From 88d4b3c3ba0cbf56a63040630f1ca9744e99ff4a Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 13 Apr 2026 14:25:48 -0400 Subject: [PATCH 01/16] fix: execute fullsend CLI and harden entrypoint output handling The run-fullsend step used `echo fullsend entrypoint` which prints the literal string instead of executing the CLI, making artifact upload non-functional. Remove `echo` so the command actually runs. Also add path validation (must be absolute) and use a heredoc delimiter for GITHUB_OUTPUT to prevent injection via newlines in CLI output. Signed-off-by: Wayne Sun --- .../.github/actions/fullsend/action.yml | 74 +++++++++---------- 1 file changed, 33 insertions(+), 41 deletions(-) diff --git a/internal/scaffold/fullsend-repo/.github/actions/fullsend/action.yml b/internal/scaffold/fullsend-repo/.github/actions/fullsend/action.yml index 8821fd4e30..784c292e24 100644 --- a/internal/scaffold/fullsend-repo/.github/actions/fullsend/action.yml +++ b/internal/scaffold/fullsend-repo/.github/actions/fullsend/action.yml @@ -1,8 +1,8 @@ name: Fullsend description: >- - Install fullsend-cli, create ${GITHUB_WORKSPACE}/output, invoke fullsend run with - --output-dir pointing at that directory, then upload it with actions/upload-artifact. Does not - read agent configuration from the repository; configure the job environment or CLI as needed. + Install fullsend-cli, run fullsend entrypoint (stdout must be the artifact directory path), then + upload that directory with actions/upload-artifact. Does not read agent configuration from the repository; + configure the job environment or CLI as needed. branding: icon: zap @@ -10,7 +10,7 @@ branding: inputs: agent: - description: Agent name passed to fullsend run (e.g. triage, code, review). + description: Agent name passed to fullsend entrypoint (e.g. triage, code, review). required: true version: description: >- @@ -18,10 +18,16 @@ inputs: names use the version without v (fullsend_0.0.1_linux_amd64.tar.gz). default: latest +outputs: + artifact_dir: + description: Absolute path printed by fullsend entrypoint on stdout (same path uploaded as an artifact). + value: ${{ steps.run-fullsend.outputs.artifact_dir }} + runs: using: composite steps: - - name: Install fullsend CLI + - id: install-fullsend + name: Install fullsend CLI shell: bash env: VERSION: ${{ inputs.version }} @@ -29,18 +35,6 @@ runs: RUNNER_ARCH: ${{ runner.arch }} run: | set -euo pipefail - - # Use vendored binary if present (placed by fullsend admin install --vendor-fullsend-binary). - # GitHub Contents API does not preserve the executable bit, so check -f not -x. - if [[ -f "${GITHUB_WORKSPACE}/bin/fullsend" ]]; then - echo "Using vendored fullsend binary from bin/fullsend" - mkdir -p "${RUNNER_TEMP}/fullsend" - cp "${GITHUB_WORKSPACE}/bin/fullsend" "${RUNNER_TEMP}/fullsend/fullsend" - chmod +x "${RUNNER_TEMP}/fullsend/fullsend" - echo "${RUNNER_TEMP}/fullsend" >> "${GITHUB_PATH}" - exit 0 - fi - VERSION="$(printf '%s' "${VERSION:-latest}" | tr -d '[:space:]')" VERSION="${VERSION:-latest}" @@ -91,36 +85,34 @@ runs: tar -xzf "/tmp/${ASSET_NAME}" -C "${RUNNER_TEMP}/fullsend" echo "${RUNNER_TEMP}/fullsend" >> "${GITHUB_PATH}" - - name: Print fullsend version - shell: bash - run: fullsend --version - - - name: Install OpenShell - shell: bash - run: | - curl -LsSf https://raw.githubusercontent.com/NVIDIA/OpenShell/v0.0.30/install.sh | sh - openshell --version - - - name: Install validation dependencies - shell: bash - run: pip install --quiet "jsonschema>=4.18.0" - - - name: Run fullsend - if: inputs.agent != '__install_only__' + - id: run-fullsend + name: Run fullsend shell: bash env: AGENT: ${{ inputs.agent }} run: | set -euo pipefail - mkdir -p "${GITHUB_WORKSPACE}/output" - fullsend run "${AGENT}" \ - --fullsend-dir "${GITHUB_WORKSPACE}" \ - --output-dir "${GITHUB_WORKSPACE}/output" \ - --target-repo "${GITHUB_WORKSPACE}/target-repo" + # fullsend must print the artifact directory path on stdout (last line wins if multiple lines). + ARTIFACT_DIR="$(fullsend entrypoint "${AGENT}" | tail -n1)" + ARTIFACT_DIR="$(printf '%s' "${ARTIFACT_DIR}" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')" + if [[ -z "${ARTIFACT_DIR}" ]]; then + echo "::error::fullsend entrypoint did not print an artifact directory on stdout" + exit 1 + fi + # Validate path is absolute and within expected boundaries. + if [[ "${ARTIFACT_DIR}" != /* ]]; then + echo "::error::fullsend entrypoint returned a non-absolute path: ${ARTIFACT_DIR}" + exit 1 + fi + # Use heredoc delimiter to prevent GITHUB_OUTPUT injection via newlines. + { + echo "artifact_dir<> "${GITHUB_OUTPUT}" - name: Upload fullsend artifacts - if: always() && inputs.agent != '__install_only__' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@v4 with: name: fullsend-${{ inputs.agent }} - path: ${{ github.workspace }}/output + path: ${{ steps.run-fullsend.outputs.artifact_dir }} From da88194d56f440642f6dbd4680a09f34e163b334 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 13 Apr 2026 14:28:40 -0400 Subject: [PATCH 02/16] fix: restore echo stub for unimplemented entrypoint command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The echo is intentional — fullsend CLI does not have an entrypoint subcommand yet. Revert the echo removal while keeping the path validation and GITHUB_OUTPUT hardening fixes. Signed-off-by: Wayne Sun --- .../scaffold/fullsend-repo/.github/actions/fullsend/action.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/scaffold/fullsend-repo/.github/actions/fullsend/action.yml b/internal/scaffold/fullsend-repo/.github/actions/fullsend/action.yml index 784c292e24..84502579fe 100644 --- a/internal/scaffold/fullsend-repo/.github/actions/fullsend/action.yml +++ b/internal/scaffold/fullsend-repo/.github/actions/fullsend/action.yml @@ -93,7 +93,7 @@ runs: run: | set -euo pipefail # fullsend must print the artifact directory path on stdout (last line wins if multiple lines). - ARTIFACT_DIR="$(fullsend entrypoint "${AGENT}" | tail -n1)" + ARTIFACT_DIR="$(echo fullsend entrypoint "${AGENT}" | tail -n1)" ARTIFACT_DIR="$(printf '%s' "${ARTIFACT_DIR}" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')" if [[ -z "${ARTIFACT_DIR}" ]]; then echo "::error::fullsend entrypoint did not print an artifact directory on stdout" From 0e1c705fdf06a4a92e03f71b63d93e0dc83af79c Mon Sep 17 00:00:00 2001 From: fullsend-code Date: Thu, 23 Apr 2026 18:33:29 +0000 Subject: [PATCH 03/16] feat: add sticky review comments and post-review CLI Add forge.Client methods for comment lifecycle management (CreateIssueComment, UpdateIssueComment, MinimizeComment) and a new `fullsend post-review` CLI subcommand that prevents review comment flooding on PR re-runs. The post-review command uses a hidden HTML marker () to identify its comment. On re-runs it finds the existing comment, collapses old content into a
block, and edits in-place. History is truncated at GitHub's 65k character limit. Changes: - forge.Client interface: add CreateIssueComment, UpdateIssueComment, MinimizeComment methods - GitHub LiveClient: implement all three (MinimizeComment uses GraphQL minimizeComment mutation via REST bridge) - FakeClient: implement all three with call recorders - internal/cli/postreview.go: new post-review subcommand with sticky comment lifecycle, dry-run mode, and stdin/file input - Unit tests for comment collapse logic, marker detection, create/update/minimize flows, truncation, and edge cases Note: pre-commit could not run (network error in sandbox). The post-script runs authoritative pre-commit on the runner. Closes #381 --- internal/cli/postreview.go | 232 +++++++++++++++++++ internal/cli/postreview_test.go | 168 ++++++++++++++ internal/cli/root.go | 1 + internal/forge/fake.go | 77 +++++- internal/forge/forge.go | 3 + internal/forge/github/github.go | 83 ++++++- internal/forge/github/github_comment_test.go | 107 +++++++++ 7 files changed, 667 insertions(+), 4 deletions(-) create mode 100644 internal/cli/postreview.go create mode 100644 internal/cli/postreview_test.go create mode 100644 internal/forge/github/github_comment_test.go diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go new file mode 100644 index 0000000000..a37383478d --- /dev/null +++ b/internal/cli/postreview.go @@ -0,0 +1,232 @@ +package cli + +import ( + "context" + "encoding/json" + "fmt" + "os" + "strconv" + "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/ui" +) + +// ReviewMarker is a hidden HTML comment used to identify fullsend review +// comments. On re-runs, the CLI searches for this marker to find and +// update the existing comment instead of creating a new one. +const ReviewMarker = "" + +// maxCommentSize is GitHub's maximum comment body size (65536 chars). +// We leave a buffer for the marker and details wrapper. +const maxCommentSize = 65000 + +func newPostReviewCmd() *cobra.Command { + var ( + repo string + pr int + result string + token 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. + +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. This prevents review comment +flooding on force-push, manual re-run, or workflow retry. + +The --result flag accepts a file path containing the review body text, +or reads from stdin if set to "-".`, + 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") + } + + 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 := readReviewBody(result) + if err != nil { + return fmt.Errorf("reading review body: %w", err) + } + + client := gh.New(token) + return postReview(cmd.Context(), client, owner, repoName, pr, body, 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 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("pr") + + return cmd +} + +// postReview implements the sticky comment lifecycle: +// 1. List existing comments to find one with ReviewMarker +// 2. If found: collapse old content into
, edit in-place +// 3. If not found: create a new comment with the marker +func postReview(ctx context.Context, client forge.Client, owner, repo string, pr int, body string, dryRun bool, printer *ui.Printer) error { + printer.Header("Post Review") + + comments, err := client.ListIssueComments(ctx, owner, repo, pr) + if err != nil { + return fmt.Errorf("listing comments: %w", err) + } + + // Find existing fullsend review comment. + var existing *forge.IssueComment + for i := range comments { + if strings.Contains(comments[i].Body, ReviewMarker) { + existing = &comments[i] + break + } + } + + markedBody := ReviewMarker + "\n" + body + + if existing != nil { + printer.StepStart("Found existing review comment, updating in-place") + + // Collapse old content into
. + newBody := buildUpdatedBody(existing.Body, markedBody) + + if 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("Review comment updated") + } else { + printer.StepStart("No existing review comment found, creating new one") + + if dryRun { + printer.StepInfo("Dry run — would create new comment") + printer.StepInfo("Body length: " + strconv.Itoa(len(markedBody))) + return nil + } + + if err := createIssueComment(ctx, client, owner, repo, pr, markedBody); err != nil { + return fmt.Errorf("creating comment: %w", err) + } + printer.StepDone("Review comment created") + } + + return nil +} + +// readReviewBody reads the review body from a file path or stdin. +func readReviewBody(path string) (string, error) { + if path == "-" { + var sb strings.Builder + buf := make([]byte, 4096) + for { + n, err := os.Stdin.Read(buf) + if n > 0 { + sb.Write(buf[:n]) + } + if err != nil { + break + } + } + return sb.String(), nil + } + + data, err := os.ReadFile(path) + if err != nil { + return "", err + } + return string(data), nil +} + +// buildUpdatedBody collapses the old comment body into a
block +// and prepends the new body. Truncates history if exceeding maxCommentSize. +func buildUpdatedBody(oldBody, newBody string) string { + // Strip the marker from the old body for the collapsed section. + oldContent := strings.Replace(oldBody, ReviewMarker+"\n", "", 1) + oldContent = strings.Replace(oldContent, ReviewMarker, "", 1) + + collapsed := fmt.Sprintf( + "\n\n
\nPrevious review\n\n%s\n\n
", + oldContent, + ) + + combined := newBody + collapsed + + // Truncate if exceeding GitHub's comment size limit. + if len(combined) > maxCommentSize { + combined = truncateBody(combined) + } + + return combined +} + +// truncateBody trims the body to fit within maxCommentSize, keeping the +// current review intact and trimming history from the end. +func truncateBody(body string) string { + if len(body) <= maxCommentSize { + return body + } + + truncationMsg := "\n\n---\n*Previous review history truncated due to comment size limits.*" + budget := maxCommentSize - len(truncationMsg) + if budget < 0 { + budget = 0 + } + + return body[:budget] + truncationMsg +} + +// createIssueComment creates a new issue comment via the forge Client. +// The forge.Client interface uses CreateIssueComment which we added +// alongside UpdateIssueComment. +func createIssueComment(ctx context.Context, client forge.Client, owner, repo string, number int, body string) error { + _, err := client.CreateIssueComment(ctx, owner, repo, number, body) + return err +} + +// ReviewResult represents a parsed review result file. +type ReviewResult struct { + Body string `json:"body"` + Action string `json:"action"` // "approve", "request-changes", "comment" +} + +// parseReviewResult attempts to parse the body as a JSON ReviewResult. +// If parsing fails, treats the entire input as a plain-text body. +func parseReviewResult(input string) ReviewResult { + var result ReviewResult + if err := json.Unmarshal([]byte(input), &result); err != nil { + return ReviewResult{Body: input, Action: "comment"} + } + if result.Body == "" { + result.Body = input + } + if result.Action == "" { + result.Action = "comment" + } + return result +} diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go new file mode 100644 index 0000000000..54798c96ca --- /dev/null +++ b/internal/cli/postreview_test.go @@ -0,0 +1,168 @@ +package cli + +import ( + "context" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/fullsend-ai/fullsend/internal/forge" + "io" + + "github.com/fullsend-ai/fullsend/internal/ui" +) + +func TestBuildUpdatedBody_CollapsesOldContent(t *testing.T) { + oldBody := ReviewMarker + "\nOld review findings here." + newBody := ReviewMarker + "\nNew review findings here." + + result := buildUpdatedBody(oldBody, newBody) + + assert.Contains(t, result, ReviewMarker) + assert.Contains(t, result, "New review findings here.") + assert.Contains(t, result, "
") + assert.Contains(t, result, "Previous review") + assert.Contains(t, result, "Old review findings here.") + assert.Contains(t, result, "
") +} + +func TestBuildUpdatedBody_PreservesNestedHistory(t *testing.T) { + // Simulate a third run where old body already has a collapsed section. + oldBody := ReviewMarker + "\nSecond review.\n\n
\nPrevious review\n\nFirst review.\n\n
" + newBody := ReviewMarker + "\nThird review." + + result := buildUpdatedBody(oldBody, newBody) + + assert.Contains(t, result, "Third review.") + assert.Contains(t, result, "Second review.") + assert.Contains(t, result, "First review.") + // Should have nested details blocks. + assert.Equal(t, 2, strings.Count(result, "
")) + assert.Equal(t, 2, strings.Count(result, "
")) +} + +func TestBuildUpdatedBody_TruncatesLargeHistory(t *testing.T) { + // Create a body that exceeds maxCommentSize. + largeOld := ReviewMarker + "\n" + strings.Repeat("x", maxCommentSize) + newBody := ReviewMarker + "\nNew review." + + result := buildUpdatedBody(largeOld, newBody) + + assert.LessOrEqual(t, len(result), maxCommentSize+100) // allow for truncation message + assert.Contains(t, result, "truncated due to comment size limits") +} + +func TestTruncateBody(t *testing.T) { + t.Run("under limit", func(t *testing.T) { + body := "short body" + assert.Equal(t, body, truncateBody(body)) + }) + + t.Run("over limit", func(t *testing.T) { + body := strings.Repeat("a", maxCommentSize+1000) + result := truncateBody(body) + assert.LessOrEqual(t, len(result), maxCommentSize+100) + assert.Contains(t, result, "truncated") + }) +} + +func TestPostReview_CreateNew(t *testing.T) { + client := forge.NewFakeClient() + client.AuthenticatedUser = "fullsend-bot" + printer := ui.New(io.Discard) + ctx := context.Background() + + err := postReview(ctx, client, "owner", "repo", 1, "Review findings.", false, printer) + require.NoError(t, err) + + // Should have created a comment. + comments := client.IssueComments["owner/repo/1"] + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, ReviewMarker) + assert.Contains(t, comments[0].Body, "Review findings.") +} + +func TestPostReview_UpdateExisting(t *testing.T) { + client := forge.NewFakeClient() + client.AuthenticatedUser = "fullsend-bot" + client.IssueComments = map[string][]forge.IssueComment{ + "owner/repo/1": { + {ID: 100, Body: ReviewMarker + "\nOld findings.", Author: "fullsend-bot"}, + }, + } + printer := ui.New(io.Discard) + ctx := context.Background() + + err := postReview(ctx, client, "owner", "repo", 1, "New findings.", false, printer) + require.NoError(t, err) + + // Should have called UpdateIssueComment. + require.Len(t, client.UpdatedComments, 1) + assert.Equal(t, 100, client.UpdatedComments[0].CommentID) + assert.Contains(t, client.UpdatedComments[0].Body, "New findings.") + assert.Contains(t, client.UpdatedComments[0].Body, "
") + assert.Contains(t, client.UpdatedComments[0].Body, "Old findings.") +} + +func TestPostReview_DryRun(t *testing.T) { + client := forge.NewFakeClient() + printer := ui.New(io.Discard) + ctx := context.Background() + + err := postReview(ctx, client, "owner", "repo", 1, "Review body.", true, printer) + require.NoError(t, err) + + // Should NOT have created any comments. + assert.Empty(t, client.IssueComments) +} + +func TestPostReview_SkipsNonMarkerComments(t *testing.T) { + client := forge.NewFakeClient() + client.AuthenticatedUser = "fullsend-bot" + client.IssueComments = map[string][]forge.IssueComment{ + "owner/repo/1": { + {ID: 50, Body: "Regular comment without marker.", Author: "human"}, + }, + } + printer := ui.New(io.Discard) + ctx := context.Background() + + err := postReview(ctx, client, "owner", "repo", 1, "Review.", false, printer) + require.NoError(t, err) + + // Should create new comment since no marker was found. + comments := client.IssueComments["owner/repo/1"] + require.Len(t, comments, 2) + assert.Contains(t, comments[1].Body, ReviewMarker) +} + +func TestParseReviewResult_JSON(t *testing.T) { + input := `{"body": "Looks good!", "action": "approve"}` + result := parseReviewResult(input) + 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 := parseReviewResult(input) + assert.Equal(t, input, result.Body) + assert.Equal(t, "comment", result.Action) +} + +func TestParseReviewResult_DefaultAction(t *testing.T) { + input := `{"body": "Some review"}` + result := parseReviewResult(input) + assert.Equal(t, "Some review", result.Body) + assert.Equal(t, "comment", result.Action) +} + +func TestReviewMarkerDetection(t *testing.T) { + body := "Some text\n" + ReviewMarker + "\nReview content" + assert.True(t, strings.Contains(body, ReviewMarker)) + + noMarker := "Some text\nReview content" + assert.False(t, strings.Contains(noMarker, ReviewMarker)) +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 4874ae196f..3991ad5c3f 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -18,6 +18,7 @@ func newRootCmd() *cobra.Command { cmd.AddCommand(newAdminCmd()) cmd.AddCommand(newRunCmd()) cmd.AddCommand(newScanCmd()) + cmd.AddCommand(newPostReviewCmd()) return cmd } diff --git a/internal/forge/fake.go b/internal/forge/fake.go index b0fd68d165..93e4f5832f 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -42,6 +42,20 @@ 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 { + Owner, Repo string + CommentID int + Reason 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 +83,9 @@ 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" + // Call recorders CreatedRepos []Repository CreatedFiles []FileRecord @@ -80,6 +97,8 @@ type FakeClient struct { Variables []VariableRecord DeletedOrgSecrets []string // "org/name" CreatedOrgSecrets []OrgSecretRecord + UpdatedComments []UpdatedCommentRecord + MinimizedComments []MinimizedCommentRecord // internal counter for change proposal numbers proposalCounter int @@ -521,15 +540,71 @@ 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 + } + comment := IssueComment{ + ID: 42, // fixed ID for test predictability + 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, + }) + return nil +} + +func (f *FakeClient) MinimizeComment(_ context.Context, owner, repo string, commentID int, 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{ + Owner: owner, + Repo: repo, + CommentID: commentID, + Reason: reason, + }) + return nil +} + func (f *FakeClient) MergeChangeProposal(_ context.Context, _, _ string, _ int) error { f.mu.Lock() defer f.mu.Unlock() diff --git a/internal/forge/forge.go b/internal/forge/forge.go index ce2cc9b78a..10e70d1200 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -133,6 +133,9 @@ 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, owner, repo string, commentID int, reason string) 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..f9b21f45f2 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -985,6 +985,83 @@ func (c *LiveClient) ListIssueComments(ctx context.Context, owner, repo string, return comments, 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("create issue comment on #%d: %w", number, err) + } + var result struct { + ID int `json:"id"` + Body string `json:"body"` + User struct { + Login string `json:"login"` + } `json:"user"` + CreatedAt string `json:"created_at"` + } + if err := decodeJSON(resp, &result); err != nil { + return nil, fmt.Errorf("decode issue comment: %w", err) + } + return &forge.IssueComment{ + ID: result.ID, + 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 reason must be one of: ABUSE, OFF_TOPIC, +// OUTDATED, RESOLVED, DUPLICATE, SPAM. +// +// This uses the REST-to-GraphQL bridge: POST /graphql with a mutation. +// The comment's GraphQL node ID is fetched first via the REST API. +func (c *LiveClient) MinimizeComment(ctx context.Context, owner, repo string, commentID int, reason string) error { + // Step 1: Get the GraphQL node ID for this comment. + resp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/issues/comments/%d", owner, repo, commentID)) + if err != nil { + return fmt.Errorf("get comment %d for minimize: %w", commentID, err) + } + var comment struct { + NodeID string `json:"node_id"` + } + if err := decodeJSON(resp, &comment); err != nil { + return fmt.Errorf("decode comment %d node_id: %w", commentID, err) + } + + // Step 2: Call the minimizeComment GraphQL mutation. + 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": comment.NodeID, + "reason": reason, + }, + } + gqlResp, err := c.post(ctx, "/graphql", gqlPayload) + if err != nil { + return fmt.Errorf("minimize comment %d: %w", commentID, err) + } + gqlResp.Body.Close() + return nil +} + // MergeChangeProposal squash-merges a pull request by number. func (c *LiveClient) MergeChangeProposal(ctx context.Context, owner, repo string, number int) error { resp, err := c.put(ctx, fmt.Sprintf("/repos/%s/%s/pulls/%d/merge", owner, repo, number), map[string]string{"merge_method": "squash"}) @@ -1183,9 +1260,9 @@ func (c *LiveClient) CreateOrgSecret(ctx context.Context, org, name, value strin selectedRepoIDs = []int64{} } payload := map[string]any{ - "encrypted_value": base64.StdEncoding.EncodeToString(encrypted), - "key_id": pubKey.KeyID, - "visibility": "selected", + "encrypted_value": base64.StdEncoding.EncodeToString(encrypted), + "key_id": pubKey.KeyID, + "visibility": "selected", "selected_repository_ids": selectedRepoIDs, } diff --git a/internal/forge/github/github_comment_test.go b/internal/forge/github/github_comment_test.go new file mode 100644 index 0000000000..4f350f3a15 --- /dev/null +++ b/internal/forge/github/github_comment_test.go @@ -0,0 +1,107 @@ +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 TestMinimizeComment(t *testing.T) { + callNum := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callNum++ + switch callNum { + case 1: + // GET comment to retrieve node_id + assert.Equal(t, "GET", r.Method) + assert.Equal(t, "/repos/owner/repo/issues/comments/789", r.URL.Path) + json.NewEncoder(w).Encode(map[string]any{ + "id": 789, + "node_id": "IC_kwDOTest", + }) + case 2: + // POST GraphQL mutation + 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(), "owner", "repo", 789, "OUTDATED") + require.NoError(t, err) + assert.Equal(t, 2, callNum) +} From 1a561355be76a3172bc4bf1ea03546743eab1c72 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 27 Apr 2026 15:08:01 -0400 Subject: [PATCH 04/16] feat: extract sticky comment package and add post-comment CLI Extract the sticky comment lifecycle (marker detection, flat history accumulation, rune-safe truncation) from post-review into a reusable internal/sticky package. Both the review and triage agents now share one codepath for find-and-update-in-place commenting. Add a generic `fullsend post-comment` CLI command that any agent script can call with --repo, --number, and --marker flags. Update post-triage.sh to use this instead of `gh issue comment`, preventing comment flooding on triage re-runs. Fix ListIssueComments to paginate (was single-page, 100 max), add GraphQL error handling to MinimizeComment, fix readReviewBody to use io.ReadAll, and revert accidental whitespace in CreateOrgSecret. Addresses #381 code quality findings and triage sticky comment request. Signed-off-by: Wayne Sun --- internal/cli/postcomment.go | 98 +++++++++ internal/cli/postcomment_test.go | 30 +++ internal/cli/postreview.go | 144 ++---------- internal/cli/postreview_test.go | 143 +----------- internal/cli/root.go | 1 + internal/forge/github/github.go | 74 ++++--- .../fullsend-repo/scripts/post-triage.sh | 6 +- internal/sticky/sticky.go | 167 ++++++++++++++ internal/sticky/sticky_test.go | 208 ++++++++++++++++++ 9 files changed, 575 insertions(+), 296 deletions(-) create mode 100644 internal/cli/postcomment.go create mode 100644 internal/cli/postcomment_test.go create mode 100644 internal/sticky/sticky.go create mode 100644 internal/sticky/sticky_test.go diff --git a/internal/cli/postcomment.go b/internal/cli/postcomment.go new file mode 100644 index 0000000000..e715814e27 --- /dev/null +++ b/internal/cli/postcomment.go @@ -0,0 +1,98 @@ +package cli + +import ( + "fmt" + "io" + "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") + } + + 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 := readCommentBody(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 +} + +func readCommentBody(path string) (string, error) { + if path == "-" { + data, err := io.ReadAll(os.Stdin) + if err != nil { + return "", fmt.Errorf("reading stdin: %w", err) + } + return string(data), nil + } + + data, err := os.ReadFile(path) + if err != nil { + return "", err + } + return string(data), nil +} 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 index a37383478d..5a484f105a 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -1,28 +1,20 @@ package cli import ( - "context" "encoding/json" "fmt" + "io" "os" - "strconv" "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" ) -// ReviewMarker is a hidden HTML comment used to identify fullsend review -// comments. On re-runs, the CLI searches for this marker to find and -// update the existing comment instead of creating a new one. -const ReviewMarker = "" - -// maxCommentSize is GitHub's maximum comment body size (65536 chars). -// We leave a buffer for the marker and details wrapper. -const maxCommentSize = 65000 +const reviewMarker = "" func newPostReviewCmd() *cobra.Command { var ( @@ -61,13 +53,21 @@ or reads from stdin if set to "-".`, } owner, repoName := parts[0], parts[1] - body, err := readReviewBody(result) + raw, err := readReviewBody(result) if err != nil { return fmt.Errorf("reading review body: %w", err) } + parsed := parseReviewResult(raw) + + printer.Header("Post Review") + client := gh.New(token) - return postReview(cmd.Context(), client, owner, repoName, pr, body, dryRun, printer) + cfg := sticky.Config{ + Marker: reviewMarker, + DryRun: dryRun, + } + return sticky.Post(cmd.Context(), client, owner, repoName, pr, parsed.Body, cfg, printer) }, } @@ -82,78 +82,14 @@ or reads from stdin if set to "-".`, return cmd } -// postReview implements the sticky comment lifecycle: -// 1. List existing comments to find one with ReviewMarker -// 2. If found: collapse old content into
, edit in-place -// 3. If not found: create a new comment with the marker -func postReview(ctx context.Context, client forge.Client, owner, repo string, pr int, body string, dryRun bool, printer *ui.Printer) error { - printer.Header("Post Review") - - comments, err := client.ListIssueComments(ctx, owner, repo, pr) - if err != nil { - return fmt.Errorf("listing comments: %w", err) - } - - // Find existing fullsend review comment. - var existing *forge.IssueComment - for i := range comments { - if strings.Contains(comments[i].Body, ReviewMarker) { - existing = &comments[i] - break - } - } - - markedBody := ReviewMarker + "\n" + body - - if existing != nil { - printer.StepStart("Found existing review comment, updating in-place") - - // Collapse old content into
. - newBody := buildUpdatedBody(existing.Body, markedBody) - - if 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("Review comment updated") - } else { - printer.StepStart("No existing review comment found, creating new one") - - if dryRun { - printer.StepInfo("Dry run — would create new comment") - printer.StepInfo("Body length: " + strconv.Itoa(len(markedBody))) - return nil - } - - if err := createIssueComment(ctx, client, owner, repo, pr, markedBody); err != nil { - return fmt.Errorf("creating comment: %w", err) - } - printer.StepDone("Review comment created") - } - - return nil -} - // readReviewBody reads the review body from a file path or stdin. func readReviewBody(path string) (string, error) { if path == "-" { - var sb strings.Builder - buf := make([]byte, 4096) - for { - n, err := os.Stdin.Read(buf) - if n > 0 { - sb.Write(buf[:n]) - } - if err != nil { - break - } + data, err := io.ReadAll(os.Stdin) + if err != nil { + return "", fmt.Errorf("reading stdin: %w", err) } - return sb.String(), nil + return string(data), nil } data, err := os.ReadFile(path) @@ -163,52 +99,6 @@ func readReviewBody(path string) (string, error) { return string(data), nil } -// buildUpdatedBody collapses the old comment body into a
block -// and prepends the new body. Truncates history if exceeding maxCommentSize. -func buildUpdatedBody(oldBody, newBody string) string { - // Strip the marker from the old body for the collapsed section. - oldContent := strings.Replace(oldBody, ReviewMarker+"\n", "", 1) - oldContent = strings.Replace(oldContent, ReviewMarker, "", 1) - - collapsed := fmt.Sprintf( - "\n\n
\nPrevious review\n\n%s\n\n
", - oldContent, - ) - - combined := newBody + collapsed - - // Truncate if exceeding GitHub's comment size limit. - if len(combined) > maxCommentSize { - combined = truncateBody(combined) - } - - return combined -} - -// truncateBody trims the body to fit within maxCommentSize, keeping the -// current review intact and trimming history from the end. -func truncateBody(body string) string { - if len(body) <= maxCommentSize { - return body - } - - truncationMsg := "\n\n---\n*Previous review history truncated due to comment size limits.*" - budget := maxCommentSize - len(truncationMsg) - if budget < 0 { - budget = 0 - } - - return body[:budget] + truncationMsg -} - -// createIssueComment creates a new issue comment via the forge Client. -// The forge.Client interface uses CreateIssueComment which we added -// alongside UpdateIssueComment. -func createIssueComment(ctx context.Context, client forge.Client, owner, repo string, number int, body string) error { - _, err := client.CreateIssueComment(ctx, owner, repo, number, body) - return err -} - // ReviewResult represents a parsed review result file. type ReviewResult struct { Body string `json:"body"` diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index 54798c96ca..79e5e49e27 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -1,143 +1,11 @@ package cli import ( - "context" - "strings" "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/fullsend-ai/fullsend/internal/forge" - "io" - - "github.com/fullsend-ai/fullsend/internal/ui" ) -func TestBuildUpdatedBody_CollapsesOldContent(t *testing.T) { - oldBody := ReviewMarker + "\nOld review findings here." - newBody := ReviewMarker + "\nNew review findings here." - - result := buildUpdatedBody(oldBody, newBody) - - assert.Contains(t, result, ReviewMarker) - assert.Contains(t, result, "New review findings here.") - assert.Contains(t, result, "
") - assert.Contains(t, result, "Previous review") - assert.Contains(t, result, "Old review findings here.") - assert.Contains(t, result, "
") -} - -func TestBuildUpdatedBody_PreservesNestedHistory(t *testing.T) { - // Simulate a third run where old body already has a collapsed section. - oldBody := ReviewMarker + "\nSecond review.\n\n
\nPrevious review\n\nFirst review.\n\n
" - newBody := ReviewMarker + "\nThird review." - - result := buildUpdatedBody(oldBody, newBody) - - assert.Contains(t, result, "Third review.") - assert.Contains(t, result, "Second review.") - assert.Contains(t, result, "First review.") - // Should have nested details blocks. - assert.Equal(t, 2, strings.Count(result, "
")) - assert.Equal(t, 2, strings.Count(result, "
")) -} - -func TestBuildUpdatedBody_TruncatesLargeHistory(t *testing.T) { - // Create a body that exceeds maxCommentSize. - largeOld := ReviewMarker + "\n" + strings.Repeat("x", maxCommentSize) - newBody := ReviewMarker + "\nNew review." - - result := buildUpdatedBody(largeOld, newBody) - - assert.LessOrEqual(t, len(result), maxCommentSize+100) // allow for truncation message - assert.Contains(t, result, "truncated due to comment size limits") -} - -func TestTruncateBody(t *testing.T) { - t.Run("under limit", func(t *testing.T) { - body := "short body" - assert.Equal(t, body, truncateBody(body)) - }) - - t.Run("over limit", func(t *testing.T) { - body := strings.Repeat("a", maxCommentSize+1000) - result := truncateBody(body) - assert.LessOrEqual(t, len(result), maxCommentSize+100) - assert.Contains(t, result, "truncated") - }) -} - -func TestPostReview_CreateNew(t *testing.T) { - client := forge.NewFakeClient() - client.AuthenticatedUser = "fullsend-bot" - printer := ui.New(io.Discard) - ctx := context.Background() - - err := postReview(ctx, client, "owner", "repo", 1, "Review findings.", false, printer) - require.NoError(t, err) - - // Should have created a comment. - comments := client.IssueComments["owner/repo/1"] - require.Len(t, comments, 1) - assert.Contains(t, comments[0].Body, ReviewMarker) - assert.Contains(t, comments[0].Body, "Review findings.") -} - -func TestPostReview_UpdateExisting(t *testing.T) { - client := forge.NewFakeClient() - client.AuthenticatedUser = "fullsend-bot" - client.IssueComments = map[string][]forge.IssueComment{ - "owner/repo/1": { - {ID: 100, Body: ReviewMarker + "\nOld findings.", Author: "fullsend-bot"}, - }, - } - printer := ui.New(io.Discard) - ctx := context.Background() - - err := postReview(ctx, client, "owner", "repo", 1, "New findings.", false, printer) - require.NoError(t, err) - - // Should have called UpdateIssueComment. - require.Len(t, client.UpdatedComments, 1) - assert.Equal(t, 100, client.UpdatedComments[0].CommentID) - assert.Contains(t, client.UpdatedComments[0].Body, "New findings.") - assert.Contains(t, client.UpdatedComments[0].Body, "
") - assert.Contains(t, client.UpdatedComments[0].Body, "Old findings.") -} - -func TestPostReview_DryRun(t *testing.T) { - client := forge.NewFakeClient() - printer := ui.New(io.Discard) - ctx := context.Background() - - err := postReview(ctx, client, "owner", "repo", 1, "Review body.", true, printer) - require.NoError(t, err) - - // Should NOT have created any comments. - assert.Empty(t, client.IssueComments) -} - -func TestPostReview_SkipsNonMarkerComments(t *testing.T) { - client := forge.NewFakeClient() - client.AuthenticatedUser = "fullsend-bot" - client.IssueComments = map[string][]forge.IssueComment{ - "owner/repo/1": { - {ID: 50, Body: "Regular comment without marker.", Author: "human"}, - }, - } - printer := ui.New(io.Discard) - ctx := context.Background() - - err := postReview(ctx, client, "owner", "repo", 1, "Review.", false, printer) - require.NoError(t, err) - - // Should create new comment since no marker was found. - comments := client.IssueComments["owner/repo/1"] - require.Len(t, comments, 2) - assert.Contains(t, comments[1].Body, ReviewMarker) -} - func TestParseReviewResult_JSON(t *testing.T) { input := `{"body": "Looks good!", "action": "approve"}` result := parseReviewResult(input) @@ -159,10 +27,9 @@ func TestParseReviewResult_DefaultAction(t *testing.T) { assert.Equal(t, "comment", result.Action) } -func TestReviewMarkerDetection(t *testing.T) { - body := "Some text\n" + ReviewMarker + "\nReview content" - assert.True(t, strings.Contains(body, ReviewMarker)) - - noMarker := "Some text\nReview content" - assert.False(t, strings.Contains(noMarker, ReviewMarker)) +func TestParseReviewResult_EmptyBody(t *testing.T) { + input := `{"action": "approve"}` + result := parseReviewResult(input) + assert.Equal(t, input, result.Body) + assert.Equal(t, "approve", result.Action) } diff --git a/internal/cli/root.go b/internal/cli/root.go index 3991ad5c3f..5bda415b6d 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -19,6 +19,7 @@ func newRootCmd() *cobra.Command { cmd.AddCommand(newRunCmd()) cmd.AddCommand(newScanCmd()) cmd.AddCommand(newPostReviewCmd()) + cmd.AddCommand(newPostCommentCmd()) return cmd } diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index f9b21f45f2..fe0f402085 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -956,33 +956,41 @@ 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)) - if err != nil { - return nil, fmt.Errorf("list issue comments: %w", err) - } - var raw []struct { - ID int `json:"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) - } - 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, + 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"` + 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, + Body: r.Body, + Author: r.User.Login, + CreatedAt: r.CreatedAt, + }) + } + + if len(raw) < 100 { + break } } - return comments, nil + return result, nil } // CreateIssueComment creates a new comment on an issue or pull request. @@ -1058,7 +1066,17 @@ func (c *LiveClient) MinimizeComment(ctx context.Context, owner, repo string, co if err != nil { return fmt.Errorf("minimize comment %d: %w", commentID, err) } - gqlResp.Body.Close() + 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 %d: %s", commentID, gqlResult.Errors[0].Message) + } return nil } @@ -1260,9 +1278,9 @@ func (c *LiveClient) CreateOrgSecret(ctx context.Context, org, name, value strin selectedRepoIDs = []int64{} } payload := map[string]any{ - "encrypted_value": base64.StdEncoding.EncodeToString(encrypted), - "key_id": pubKey.KeyID, - "visibility": "selected", + "encrypted_value": base64.StdEncoding.EncodeToString(encrypted), + "key_id": pubKey.KeyID, + "visibility": "selected", "selected_repository_ids": selectedRepoIDs, } 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..bbb1015a4d --- /dev/null +++ b/internal/sticky/sticky.go @@ -0,0 +1,167 @@ +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 { + comments, err := client.ListIssueComments(ctx, owner, repo, number) + if err != nil { + return fmt.Errorf("listing comments: %w", err) + } + + existing := FindMarkedComment(comments, cfg.Marker) + 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. +func FindMarkedComment(comments []forge.IssueComment, marker string) *forge.IssueComment { + for i := range comments { + if strings.Contains(comments[i].Body, marker) { + return &comments[i] + } + } + return nil +} + +// detailsRe matches a
...Previous ......
block. +var detailsRe = 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. + oldContent := strings.Replace(oldBody, cfg.Marker+"\n", "", 1) + oldContent = strings.Replace(oldContent, cfg.Marker, "", 1) + + // 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. + var historyBlocks []string + matches := detailsRe.FindAllStringSubmatch(oldContent, -1) + for _, m := range matches { + historyBlocks = append(historyBlocks, m[1]) + } + + // The "current" old content is everything minus the old
blocks. + currentOld := detailsRe.ReplaceAllString(oldContent, "") + currentOld = strings.TrimSpace(currentOld) + + // Build flat history: the old current content becomes the newest history + // entry, followed by any previously accumulated history entries. + var collapsed strings.Builder + if currentOld != "" { + collapsed.WriteString("\n\n
\nPrevious run\n\n") + collapsed.WriteString(currentOld) + collapsed.WriteString("\n\n
") + } + for i, block := range historyBlocks { + collapsed.WriteString(fmt.Sprintf("\n\n
\nPrevious run (%d)\n\n", i+2)) + collapsed.WriteString(strings.TrimSpace(block)) + collapsed.WriteString("\n\n
") + } + + combined := newBody + collapsed.String() + + // Re-append footer. + if footer != "" { + combined += "\n\n" + footer + } + + 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..a3c514da5d --- /dev/null +++ b/internal/sticky/sticky_test.go @@ -0,0 +1,208 @@ +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"}, + {ID: 2, Body: "\nmarked"}, + {ID: 3, Body: "another comment"}, + } + + found := FindMarkedComment(comments, "") + require.NotNil(t, found) + assert.Equal(t, 2, found.ID) + + notFound := FindMarkedComment(comments, "") + assert.Nil(t, notFound) +} + +func TestFindMarkedComment_Empty(t *testing.T) { + found := FindMarkedComment(nil, "") + 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_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 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_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) +} From 2945ca4c0f7d49f62cbab5be69ccc6ed6cfc6190 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 27 Apr 2026 15:16:25 -0400 Subject: [PATCH 05/16] test: add coverage for ListIssueComments pagination and MinimizeComment error handling Add 5 new tests covering the two zero-coverage code paths: - ListIssueComments: single page, multi-page pagination, API error - MinimizeComment: GraphQL error response, GET node_id failure Signed-off-by: Wayne Sun --- internal/forge/github/github_comment_test.go | 124 +++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/internal/forge/github/github_comment_test.go b/internal/forge/github/github_comment_test.go index 4f350f3a15..7191b8029d 100644 --- a/internal/forge/github/github_comment_test.go +++ b/internal/forge/github/github_comment_test.go @@ -60,6 +60,87 @@ func TestUpdateIssueComment(t *testing.T) { 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) { callNum := 0 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -105,3 +186,46 @@ func TestMinimizeComment(t *testing.T) { require.NoError(t, err) assert.Equal(t, 2, callNum) } + +func TestMinimizeComment_GraphQLError(t *testing.T) { + callNum := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callNum++ + switch callNum { + case 1: + // GET comment node_id — success. + json.NewEncoder(w).Encode(map[string]any{ + "id": 789, + "node_id": "IC_kwDOTest", + }) + case 2: + // POST GraphQL mutation — returns a GraphQL-level error. + 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(), "owner", "repo", 789, "OUTDATED") + require.Error(t, err) + assert.Contains(t, err.Error(), "minimize comment 789") + assert.Contains(t, err.Error(), "Could not resolve to a node") +} + +func TestMinimizeComment_GetNodeIDError(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) + err := client.MinimizeComment(context.Background(), "owner", "repo", 999, "OUTDATED") + require.Error(t, err) + assert.Contains(t, err.Error(), "get comment 999 for minimize") +} From 7e1fd4532f345079119343c1d3c0e69ac4bd77f2 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 27 Apr 2026 15:38:59 -0400 Subject: [PATCH 06/16] feat: wire formal PR reviews and fix review findings - Wire ReviewResult.Action to submit formal GitHub PR reviews (APPROVE, REQUEST_CHANGES, COMMENT) after posting sticky comment - Minimize stale reviews by the same user on re-runs - Fix parseReviewResult to return error on empty body instead of silently posting raw JSON as comment text - Deduplicate readBody helper into shared readbody.go - Add CreatePullRequestReview and ListPullRequestReviews to forge.Client interface with GitHub and FakeClient implementations - Add comprehensive tests for review dispatch, stale minimization, action-to-event mapping, and parseReviewResult error handling Signed-off-by: Wayne Sun --- internal/cli/postcomment.go | 19 +---- internal/cli/postreview.go | 121 +++++++++++++++++++++++++------- internal/cli/postreview_test.go | 111 +++++++++++++++++++++++++++-- internal/cli/readbody.go | 24 +++++++ internal/forge/fake.go | 42 +++++++++++ internal/forge/forge.go | 13 ++++ internal/forge/github/github.go | 54 ++++++++++++++ 7 files changed, 336 insertions(+), 48 deletions(-) create mode 100644 internal/cli/readbody.go diff --git a/internal/cli/postcomment.go b/internal/cli/postcomment.go index e715814e27..4f5109bb33 100644 --- a/internal/cli/postcomment.go +++ b/internal/cli/postcomment.go @@ -2,7 +2,6 @@ package cli import ( "fmt" - "io" "os" "strings" @@ -52,7 +51,7 @@ The --result flag accepts a file path or "-" for stdin.`, } owner, repoName := parts[0], parts[1] - body, err := readCommentBody(result) + body, err := readBody(result) if err != nil { return fmt.Errorf("reading comment body: %w", err) } @@ -80,19 +79,3 @@ The --result flag accepts a file path or "-" for stdin.`, return cmd } - -func readCommentBody(path string) (string, error) { - if path == "-" { - data, err := io.ReadAll(os.Stdin) - if err != nil { - return "", fmt.Errorf("reading stdin: %w", err) - } - return string(data), nil - } - - data, err := os.ReadFile(path) - if err != nil { - return "", err - } - return string(data), nil -} diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index 5a484f105a..e1fb5316da 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -1,14 +1,15 @@ package cli import ( + "context" "encoding/json" "fmt" - "io" "os" "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" @@ -53,12 +54,15 @@ or reads from stdin if set to "-".`, } owner, repoName := parts[0], parts[1] - raw, err := readReviewBody(result) + raw, err := readBody(result) if err != nil { return fmt.Errorf("reading review body: %w", err) } - parsed := parseReviewResult(raw) + parsed, err := parseReviewResult(raw) + if err != nil { + return fmt.Errorf("parsing review result: %w", err) + } printer.Header("Post Review") @@ -67,7 +71,11 @@ or reads from stdin if set to "-".`, Marker: reviewMarker, DryRun: dryRun, } - return sticky.Post(cmd.Context(), client, owner, repoName, pr, parsed.Body, 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, dryRun, printer) }, } @@ -82,41 +90,106 @@ or reads from stdin if set to "-".`, return cmd } -// readReviewBody reads the review body from a file path or stdin. -func readReviewBody(path string) (string, error) { - if path == "-" { - data, err := io.ReadAll(os.Stdin) - if err != nil { - return "", fmt.Errorf("reading stdin: %w", err) - } - return string(data), nil +// ReviewResult represents a parsed review result file. +type ReviewResult struct { + Body string `json:"body"` + Action string `json:"action"` // "approve", "request-changes", "comment" +} + +// 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 } +} - data, err := os.ReadFile(path) - if err != nil { - return "", err +// submitFormalReview submits a GitHub PR review and minimizes stale reviews +// by the same user. +func submitFormalReview(ctx context.Context, client forge.Client, owner, repo string, pr int, parsed ReviewResult, dryRun bool, printer *ui.Printer) error { + event, ok := reviewActionToEvent(parsed.Action) + if !ok { + printer.StepInfo(fmt.Sprintf("Unknown review action %q, skipping formal review", parsed.Action)) + return nil + } + + if dryRun { + printer.StepInfo(fmt.Sprintf("Dry run — would submit %s review", event)) + return nil } - return string(data), nil + + 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); err != nil { + return fmt.Errorf("submitting review: %w", err) + } + printer.StepDone("Review submitted") + + return minimizeStaleReviews(ctx, client, owner, repo, pr, printer) } -// ReviewResult represents a parsed review result file. -type ReviewResult struct { - Body string `json:"body"` - Action string `json:"action"` // "approve", "request-changes", "comment" +// minimizeStaleReviews lists all reviews on the PR, finds previous reviews +// by the authenticated user, and minimizes them to reduce noise. +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 + } + + // The most recent review is the one we just posted — skip it. + // Minimize all other reviews by the same user. + var stale []forge.PullRequestReview + for _, r := range reviews { + if r.User == user { + stale = append(stale, r) + } + } + + if len(stale) <= 1 { + return nil + } + + // Skip the last one (most recent = the one we just created). + stale = stale[:len(stale)-1] + + printer.StepStart(fmt.Sprintf("Minimizing %d stale review(s)", len(stale))) + for _, r := range stale { + if err := client.MinimizeComment(ctx, owner, repo, r.ID, "OUTDATED"); err != nil { + printer.StepInfo(fmt.Sprintf("Warning: could not minimize review %d: %v", r.ID, 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. -func parseReviewResult(input string) ReviewResult { +// Returns an error if the JSON is valid but the body field is empty. +func parseReviewResult(input string) (ReviewResult, error) { var result ReviewResult if err := json.Unmarshal([]byte(input), &result); err != nil { - return ReviewResult{Body: input, Action: "comment"} + return ReviewResult{Body: input, Action: "comment"}, nil } if result.Body == "" { - result.Body = input + return ReviewResult{}, fmt.Errorf("review result JSON has empty body field") } if result.Action == "" { result.Action = "comment" } - return result + return result, nil } diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index 79e5e49e27..8476a5f976 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -1,35 +1,134 @@ package cli import ( + "context" + "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/ui" ) func TestParseReviewResult_JSON(t *testing.T) { input := `{"body": "Looks good!", "action": "approve"}` - result := parseReviewResult(input) + 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 := parseReviewResult(input) + 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 := parseReviewResult(input) + 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"}` - result := parseReviewResult(input) - assert.Equal(t, input, result.Body) - assert.Equal(t, "approve", result.Action) + _, err := parseReviewResult(input) + assert.Error(t, err) + assert.Contains(t, err.Error(), "empty body") +} + +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 TestSubmitFormalReview_CreatesAndMinimizesStale(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-bot" + fc.PRReviews = map[string][]forge.PullRequestReview{ + "acme/repo/1": { + {ID: 100, User: "fullsend-bot", State: "COMMENTED", Body: "old review 1"}, + {ID: 200, User: "someone-else", State: "APPROVED", Body: "lgtm"}, + {ID: 300, User: "fullsend-bot", State: "APPROVED", Body: "old review 2"}, + }, + } + + printer := ui.New(io.Discard) + parsed := ReviewResult{Body: "New findings", Action: "approve"} + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, parsed, false, printer) + require.NoError(t, err) + + // Should have created one review. + require.Len(t, fc.CreatedReviews, 1) + assert.Equal(t, "APPROVE", fc.CreatedReviews[0].Event) + + // Should have minimized 2 stale reviews (IDs 100 and 300), skipping + // the one we just created (which is now the last in the list after + // ListPullRequestReviews returns the pre-populated + new one). + // But since FakeClient's ListPullRequestReviews returns the pre-populated + // reviews (3 by fullsend-bot: 100, 300, plus the newly created one is NOT + // in PRReviews), we expect 2 reviews by fullsend-bot, and we skip the last + // one (300), minimizing only 100. + require.Len(t, fc.MinimizedComments, 1) + assert.Equal(t, 100, fc.MinimizedComments[0].CommentID) + assert.Equal(t, "OUTDATED", fc.MinimizedComments[0].Reason) +} + +func TestSubmitFormalReview_DryRun(t *testing.T) { + fc := forge.NewFakeClient() + printer := ui.New(io.Discard) + parsed := ReviewResult{Body: "Findings", Action: "approve"} + + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, parsed, 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) + parsed := ReviewResult{Body: "Findings", Action: "unknown-action"} + + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, parsed, false, printer) + require.NoError(t, err) + assert.Empty(t, fc.CreatedReviews) +} + +func TestMinimizeStaleReviews_NoStaleReviews(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-bot" + fc.PRReviews = map[string][]forge.PullRequestReview{ + "acme/repo/1": { + {ID: 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) + assert.Empty(t, fc.MinimizedComments) } diff --git a/internal/cli/readbody.go b/internal/cli/readbody.go new file mode 100644 index 0000000000..83ca415de5 --- /dev/null +++ b/internal/cli/readbody.go @@ -0,0 +1,24 @@ +package cli + +import ( + "fmt" + "io" + "os" +) + +// readBody reads content from a file path or stdin (when path is "-"). +func readBody(path string) (string, error) { + if path == "-" { + data, err := io.ReadAll(os.Stdin) + if err != nil { + return "", fmt.Errorf("reading stdin: %w", err) + } + return string(data), nil + } + + data, err := os.ReadFile(path) + if err != nil { + return "", err + } + return string(data), nil +} diff --git a/internal/forge/fake.go b/internal/forge/fake.go index 93e4f5832f..bd1f646825 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -56,6 +56,13 @@ type MinimizedCommentRecord struct { Reason string } +// ReviewRecord records a pull request review creation call. +type ReviewRecord struct { + Owner, Repo string + Number int + Event, Body 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. @@ -86,6 +93,9 @@ type FakeClient struct { // Issue comments for ListIssueComments / UpdateIssueComment. IssueComments map[string][]IssueComment // key: "owner/repo/number" + // Pull request reviews for ListPullRequestReviews. + PRReviews map[string][]PullRequestReview // key: "owner/repo/number" + // Call recorders CreatedRepos []Repository CreatedFiles []FileRecord @@ -99,6 +109,7 @@ type FakeClient struct { CreatedOrgSecrets []OrgSecretRecord UpdatedComments []UpdatedCommentRecord MinimizedComments []MinimizedCommentRecord + CreatedReviews []ReviewRecord // internal counter for change proposal numbers proposalCounter int @@ -605,6 +616,37 @@ func (f *FakeClient) MinimizeComment(_ context.Context, owner, repo string, comm return nil } +func (f *FakeClient) CreatePullRequestReview(_ context.Context, owner, repo string, number int, event, body 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, + }) + 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 +} + func (f *FakeClient) MergeChangeProposal(_ context.Context, _, _ string, _ int) error { f.mu.Lock() defer f.mu.Unlock() diff --git a/internal/forge/forge.go b/internal/forge/forge.go index 10e70d1200..f797424120 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -63,6 +63,15 @@ type IssueComment struct { CreatedAt string } +// PullRequestReview represents a formal review on a pull request. +type PullRequestReview struct { + ID int + 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 @@ -137,6 +146,10 @@ type Client interface { UpdateIssueComment(ctx context.Context, owner, repo string, commentID int, body string) error MinimizeComment(ctx context.Context, owner, repo string, commentID int, reason string) error + // Pull request review operations + CreatePullRequestReview(ctx context.Context, owner, repo string, number int, event, body 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 fe0f402085..4c95d35d29 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -1080,6 +1080,60 @@ func (c *LiveClient) MinimizeComment(ctx context.Context, owner, repo string, co return nil } +// CreatePullRequestReview submits a formal review on a pull request. +// The event must be one of: APPROVE, REQUEST_CHANGES, COMMENT. +func (c *LiveClient) CreatePullRequestReview(ctx context.Context, owner, repo string, number int, event, body string) error { + payload := map[string]string{ + "event": event, + "body": body, + } + 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"` + 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, + 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. func (c *LiveClient) MergeChangeProposal(ctx context.Context, owner, repo string, number int) error { resp, err := c.put(ctx, fmt.Sprintf("/repos/%s/%s/pulls/%d/merge", owner, repo, number), map[string]string{"merge_method": "squash"}) From 42985b970f995f262e8e42a09954ed4b1cf66176 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 27 Apr 2026 16:17:25 -0400 Subject: [PATCH 07/16] fix: use NodeID for MinimizeComment and eliminate TOCTOU race MinimizeComment previously took an integer comment ID and looked it up via the issue comments REST endpoint, but minimizeStaleReviews passed PR review IDs which live on a different endpoint (wrong ID space). Change MinimizeComment to accept a GraphQL node_id string directly, which eliminates the wrong-endpoint bug and removes an extra REST round-trip. Add NodeID fields to IssueComment and PullRequestReview types so callers have the value available. Reorder submitFormalReview to minimize stale reviews BEFORE creating the new one, eliminating a TOCTOU race where the just-created review might not appear in the list due to API eventual consistency. Fix FakeClient.CreatePullRequestReview to also append to the PRReviews map so ListPullRequestReviews reflects created reviews in tests. Signed-off-by: Wayne Sun --- internal/cli/postreview.go | 27 +++--- internal/cli/postreview_test.go | 37 +++++--- internal/forge/fake.go | 26 ++++-- internal/forge/forge.go | 12 ++- internal/forge/github/github.go | 52 +++++------ internal/forge/github/github_comment_test.go | 96 ++++++-------------- 6 files changed, 111 insertions(+), 139 deletions(-) diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index e1fb5316da..bea1dcd060 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -110,8 +110,9 @@ func reviewActionToEvent(action string) (string, bool) { } } -// submitFormalReview submits a GitHub PR review and minimizes stale reviews -// by the same user. +// submitFormalReview minimizes stale reviews by the same user, then +// submits a new GitHub PR review. Minimizing first avoids a TOCTOU race +// where the just-created review might not appear in the list yet. func submitFormalReview(ctx context.Context, client forge.Client, owner, repo string, pr int, parsed ReviewResult, dryRun bool, printer *ui.Printer) error { event, ok := reviewActionToEvent(parsed.Action) if !ok { @@ -124,19 +125,22 @@ func submitFormalReview(ctx context.Context, client forge.Client, owner, repo st return nil } - printer.StepStart(fmt.Sprintf("Submitting %s review", event)) + 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); err != nil { return fmt.Errorf("submitting review: %w", err) } printer.StepDone("Review submitted") - - return minimizeStaleReviews(ctx, client, owner, repo, pr, printer) + return nil } // minimizeStaleReviews lists all reviews on the PR, finds previous reviews -// by the authenticated user, and minimizes them to reduce noise. +// 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 { @@ -150,8 +154,6 @@ func minimizeStaleReviews(ctx context.Context, client forge.Client, owner, repo return nil } - // The most recent review is the one we just posted — skip it. - // Minimize all other reviews by the same user. var stale []forge.PullRequestReview for _, r := range reviews { if r.User == user { @@ -159,17 +161,14 @@ func minimizeStaleReviews(ctx context.Context, client forge.Client, owner, repo } } - if len(stale) <= 1 { + if len(stale) == 0 { return nil } - // Skip the last one (most recent = the one we just created). - stale = stale[:len(stale)-1] - printer.StepStart(fmt.Sprintf("Minimizing %d stale review(s)", len(stale))) for _, r := range stale { - if err := client.MinimizeComment(ctx, owner, repo, r.ID, "OUTDATED"); err != nil { - printer.StepInfo(fmt.Sprintf("Warning: could not minimize review %d: %v", r.ID, err)) + 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") diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index 8476a5f976..7daafbd258 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -71,9 +71,9 @@ func TestSubmitFormalReview_CreatesAndMinimizesStale(t *testing.T) { fc.AuthenticatedUser = "fullsend-bot" fc.PRReviews = map[string][]forge.PullRequestReview{ "acme/repo/1": { - {ID: 100, User: "fullsend-bot", State: "COMMENTED", Body: "old review 1"}, - {ID: 200, User: "someone-else", State: "APPROVED", Body: "lgtm"}, - {ID: 300, User: "fullsend-bot", State: "APPROVED", Body: "old review 2"}, + {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"}, }, } @@ -86,16 +86,13 @@ func TestSubmitFormalReview_CreatesAndMinimizesStale(t *testing.T) { require.Len(t, fc.CreatedReviews, 1) assert.Equal(t, "APPROVE", fc.CreatedReviews[0].Event) - // Should have minimized 2 stale reviews (IDs 100 and 300), skipping - // the one we just created (which is now the last in the list after - // ListPullRequestReviews returns the pre-populated + new one). - // But since FakeClient's ListPullRequestReviews returns the pre-populated - // reviews (3 by fullsend-bot: 100, 300, plus the newly created one is NOT - // in PRReviews), we expect 2 reviews by fullsend-bot, and we skip the last - // one (300), minimizing only 100. - require.Len(t, fc.MinimizedComments, 1) - assert.Equal(t, 100, fc.MinimizedComments[0].CommentID) + // Stale reviews are minimized BEFORE creating the new one, so both + // existing reviews by fullsend-bot (IDs 100, 300) are minimized. + 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) { @@ -118,15 +115,27 @@ func TestSubmitFormalReview_UnknownAction(t *testing.T) { assert.Empty(t, fc.CreatedReviews) } -func TestMinimizeStaleReviews_NoStaleReviews(t *testing.T) { +func TestMinimizeStaleReviews_MinimizesAll(t *testing.T) { fc := forge.NewFakeClient() fc.AuthenticatedUser = "fullsend-bot" fc.PRReviews = map[string][]forge.PullRequestReview{ "acme/repo/1": { - {ID: 100, User: "fullsend-bot", State: "APPROVED", Body: "only review"}, + {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) + // Called before creating a new review, so the single existing review is stale. + require.Len(t, fc.MinimizedComments, 1) + assert.Equal(t, "PRR_100", fc.MinimizedComments[0].NodeID) +} + +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) diff --git a/internal/forge/fake.go b/internal/forge/fake.go index bd1f646825..c6865f5fc7 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -51,9 +51,8 @@ type UpdatedCommentRecord struct { // MinimizedCommentRecord records a comment minimize call. type MinimizedCommentRecord struct { - Owner, Repo string - CommentID int - Reason string + NodeID string + Reason string } // ReviewRecord records a pull request review creation call. @@ -601,17 +600,15 @@ func (f *FakeClient) UpdateIssueComment(_ context.Context, owner, repo string, c return nil } -func (f *FakeClient) MinimizeComment(_ context.Context, owner, repo string, commentID int, reason string) error { +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{ - Owner: owner, - Repo: repo, - CommentID: commentID, - Reason: reason, + NodeID: nodeID, + Reason: reason, }) return nil } @@ -629,6 +626,19 @@ func (f *FakeClient) CreatePullRequestReview(_ context.Context, owner, repo stri Event: event, Body: body, }) + + 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 } diff --git a/internal/forge/forge.go b/internal/forge/forge.go index f797424120..16c89b77dc 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -58,6 +58,7 @@ type Issue struct { // IssueComment represents a comment on an issue. type IssueComment struct { ID int + NodeID string Body string Author string CreatedAt string @@ -65,10 +66,11 @@ type IssueComment struct { // PullRequestReview represents a formal review on a pull request. type PullRequestReview struct { - ID int - User string - State string // "APPROVED", "CHANGES_REQUESTED", "COMMENTED", "DISMISSED" - Body string + ID int + NodeID string + User string + State string // "APPROVED", "CHANGES_REQUESTED", "COMMENTED", "DISMISSED" + Body string SubmittedAt string } @@ -144,7 +146,7 @@ type Client interface { 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, owner, repo string, commentID int, reason string) error + MinimizeComment(ctx context.Context, nodeID, reason string) error // Pull request review operations CreatePullRequestReview(ctx context.Context, owner, repo string, number int, event, body string) error diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 4c95d35d29..a742a0e254 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -966,9 +966,10 @@ func (c *LiveClient) ListIssueComments(ctx context.Context, owner, repo string, return nil, fmt.Errorf("list issue comments page %d: %w", page, err) } var raw []struct { - ID int `json:"id"` - Body string `json:"body"` - User 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"` @@ -980,6 +981,7 @@ func (c *LiveClient) ListIssueComments(ctx context.Context, owner, repo string, 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, @@ -1001,9 +1003,10 @@ func (c *LiveClient) CreateIssueComment(ctx context.Context, owner, repo string, return nil, fmt.Errorf("create issue comment on #%d: %w", number, err) } var result struct { - ID int `json:"id"` - Body string `json:"body"` - User 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"` @@ -1013,6 +1016,7 @@ func (c *LiveClient) CreateIssueComment(ctx context.Context, owner, repo string, } return &forge.IssueComment{ ID: result.ID, + NodeID: result.NodeID, Body: result.Body, Author: result.User.Login, CreatedAt: result.CreatedAt, @@ -1031,25 +1035,11 @@ func (c *LiveClient) UpdateIssueComment(ctx context.Context, owner, repo string, } // MinimizeComment minimizes (hides) an issue or review comment via the -// GitHub GraphQL API. The reason must be one of: ABUSE, OFF_TOPIC, -// OUTDATED, RESOLVED, DUPLICATE, SPAM. -// -// This uses the REST-to-GraphQL bridge: POST /graphql with a mutation. -// The comment's GraphQL node ID is fetched first via the REST API. -func (c *LiveClient) MinimizeComment(ctx context.Context, owner, repo string, commentID int, reason string) error { - // Step 1: Get the GraphQL node ID for this comment. - resp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/issues/comments/%d", owner, repo, commentID)) - if err != nil { - return fmt.Errorf("get comment %d for minimize: %w", commentID, err) - } - var comment struct { - NodeID string `json:"node_id"` - } - if err := decodeJSON(resp, &comment); err != nil { - return fmt.Errorf("decode comment %d node_id: %w", commentID, err) - } - - // Step 2: Call the minimizeComment GraphQL mutation. +// 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 { query := `mutation($id: ID!, $reason: ReportedContentClassifiers!) { minimizeComment(input: {subjectId: $id, classifier: $reason}) { minimizedComment { isMinimized } @@ -1058,13 +1048,13 @@ func (c *LiveClient) MinimizeComment(ctx context.Context, owner, repo string, co gqlPayload := map[string]any{ "query": query, "variables": map[string]string{ - "id": comment.NodeID, + "id": nodeID, "reason": reason, }, } gqlResp, err := c.post(ctx, "/graphql", gqlPayload) if err != nil { - return fmt.Errorf("minimize comment %d: %w", commentID, err) + return fmt.Errorf("minimize comment %s: %w", nodeID, err) } var gqlResult struct { Errors []struct { @@ -1075,7 +1065,7 @@ func (c *LiveClient) MinimizeComment(ctx context.Context, owner, repo string, co return fmt.Errorf("decode minimize response: %w", err) } if len(gqlResult.Errors) > 0 { - return fmt.Errorf("minimize comment %d: %s", commentID, gqlResult.Errors[0].Message) + return fmt.Errorf("minimize comment %s: %s", nodeID, gqlResult.Errors[0].Message) } return nil } @@ -1105,8 +1095,9 @@ func (c *LiveClient) ListPullRequestReviews(ctx context.Context, owner, repo str return nil, fmt.Errorf("list pull request reviews page %d: %w", page, err) } var raw []struct { - ID int `json:"id"` - User struct { + ID int `json:"id"` + NodeID string `json:"node_id"` + User struct { Login string `json:"login"` } `json:"user"` State string `json:"state"` @@ -1120,6 +1111,7 @@ func (c *LiveClient) ListPullRequestReviews(ctx context.Context, owner, repo str 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, diff --git a/internal/forge/github/github_comment_test.go b/internal/forge/github/github_comment_test.go index 7191b8029d..53e957bd69 100644 --- a/internal/forge/github/github_comment_test.go +++ b/internal/forge/github/github_comment_test.go @@ -142,90 +142,50 @@ func TestListIssueComments_APIError(t *testing.T) { } func TestMinimizeComment(t *testing.T) { - callNum := 0 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - callNum++ - switch callNum { - case 1: - // GET comment to retrieve node_id - assert.Equal(t, "GET", r.Method) - assert.Equal(t, "/repos/owner/repo/issues/comments/789", r.URL.Path) - json.NewEncoder(w).Encode(map[string]any{ - "id": 789, - "node_id": "IC_kwDOTest", - }) - case 2: - // POST GraphQL mutation - 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, - }, + 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(), "owner", "repo", 789, "OUTDATED") + err := client.MinimizeComment(context.Background(), "IC_kwDOTest", "OUTDATED") require.NoError(t, err) - assert.Equal(t, 2, callNum) } func TestMinimizeComment_GraphQLError(t *testing.T) { - callNum := 0 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - callNum++ - switch callNum { - case 1: - // GET comment node_id — success. - json.NewEncoder(w).Encode(map[string]any{ - "id": 789, - "node_id": "IC_kwDOTest", - }) - case 2: - // POST GraphQL mutation — returns a GraphQL-level error. - 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'"}, - }, - }) - } + 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(), "owner", "repo", 789, "OUTDATED") + err := client.MinimizeComment(context.Background(), "IC_kwDOTest", "OUTDATED") require.Error(t, err) - assert.Contains(t, err.Error(), "minimize comment 789") + assert.Contains(t, err.Error(), "minimize comment IC_kwDOTest") assert.Contains(t, err.Error(), "Could not resolve to a node") } - -func TestMinimizeComment_GetNodeIDError(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) - err := client.MinimizeComment(context.Background(), "owner", "repo", 999, "OUTDATED") - require.Error(t, err) - assert.Contains(t, err.Error(), "get comment 999 for minimize") -} From 6a1c3325d77a14f23f6e5d4b4f0f3e7b660b8c03 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 27 Apr 2026 16:38:59 -0400 Subject: [PATCH 08/16] fix: harden sticky comments, FakeClient fidelity, and MinimizeComment validation - Wrap sticky history blocks with sentinel comment delimiters to prevent nested
in review content from corrupting history extraction - Add legacy regex fallback for backward compatibility with existing comments - Add client-side reason validation to MinimizeComment before GraphQL call - FakeClient.CreateIssueComment now uses monotonic counter and sets NodeID - FakeClient.UpdateIssueComment now mutates backing IssueComments state - Add httptest coverage for CreatePullRequestReview and ListPullRequestReviews - Add error tolerance test for minimizeStaleReviews Signed-off-by: Wayne Sun --- internal/cli/postreview_test.go | 17 ++++++ internal/forge/fake.go | 15 +++++- internal/forge/github/github.go | 6 +++ internal/forge/github/github_comment_test.go | 57 ++++++++++++++++++++ internal/sticky/sticky.go | 27 ++++++++-- internal/sticky/sticky_test.go | 19 +++++++ 6 files changed, 136 insertions(+), 5 deletions(-) diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index 7daafbd258..84c34f595e 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -2,6 +2,7 @@ package cli import ( "context" + "fmt" "io" "testing" @@ -132,6 +133,22 @@ func TestMinimizeStaleReviews_MinimizesAll(t *testing.T) { 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" diff --git a/internal/forge/fake.go b/internal/forge/fake.go index c6865f5fc7..a438a89e8f 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -110,8 +110,9 @@ type FakeClient struct { 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. @@ -571,8 +572,10 @@ func (f *FakeClient) CreateIssueComment(_ context.Context, owner, repo string, n if e := f.err("CreateIssueComment"); e != nil { return nil, e } + f.commentCounter++ comment := IssueComment{ - ID: 42, // fixed ID for test predictability + ID: f.commentCounter, + NodeID: fmt.Sprintf("IC_fake_%d", f.commentCounter), Body: body, Author: f.AuthenticatedUser, CreatedAt: "2026-01-01T00:00:00Z", @@ -597,6 +600,14 @@ func (f *FakeClient) UpdateIssueComment(_ context.Context, owner, repo string, c 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 } diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index a742a0e254..259ec9ebc9 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -1040,6 +1040,12 @@ func (c *LiveClient) UpdateIssueComment(ctx context.Context, owner, repo string, // 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) + } + query := `mutation($id: ID!, $reason: ReportedContentClassifiers!) { minimizeComment(input: {subjectId: $id, classifier: $reason}) { minimizedComment { isMinimized } diff --git a/internal/forge/github/github_comment_test.go b/internal/forge/github/github_comment_test.go index 53e957bd69..eda858bfdd 100644 --- a/internal/forge/github/github_comment_test.go +++ b/internal/forge/github/github_comment_test.go @@ -189,3 +189,60 @@ func TestMinimizeComment_GraphQLError(t *testing.T) { 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"]) + + 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/sticky/sticky.go b/internal/sticky/sticky.go index bbb1015a4d..c67b68c8c6 100644 --- a/internal/sticky/sticky.go +++ b/internal/sticky/sticky.go @@ -85,8 +85,18 @@ func FindMarkedComment(comments []forge.IssueComment, marker string) *forge.Issu return nil } -// detailsRe matches a
...Previous ......
block. -var detailsRe = regexp.MustCompile(`(?s)
\s*Previous [^<]*\s*(.*?)\s*
`) +// 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 @@ -106,27 +116,38 @@ func BuildUpdatedBody(oldBody, newBody string, cfg Config) string { } // 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 := detailsRe.ReplaceAllString(oldContent, "") + currentOld := activeRe.ReplaceAllString(oldContent, "") currentOld = strings.TrimSpace(currentOld) // Build flat history: the old current content becomes the newest history // entry, followed by any previously accumulated history entries. + // Each block is wrapped with sentinel comments for safe extraction. var collapsed strings.Builder if currentOld != "" { collapsed.WriteString("\n\n
\nPrevious run\n\n") + collapsed.WriteString(historyStart + "\n") collapsed.WriteString(currentOld) + collapsed.WriteString("\n" + historyEnd) collapsed.WriteString("\n\n
") } for i, block := range historyBlocks { collapsed.WriteString(fmt.Sprintf("\n\n
\nPrevious run (%d)\n\n", i+2)) + collapsed.WriteString(historyStart + "\n") collapsed.WriteString(strings.TrimSpace(block)) + collapsed.WriteString("\n" + historyEnd) collapsed.WriteString("\n\n
") } diff --git a/internal/sticky/sticky_test.go b/internal/sticky/sticky_test.go index a3c514da5d..5257b9a4cc 100644 --- a/internal/sticky/sticky_test.go +++ b/internal/sticky/sticky_test.go @@ -91,6 +91,25 @@ func TestBuildUpdatedBody_FlatHistory(t *testing.T) { } } +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." From 4a1601ea4d11662214c7e31b34b837a13a88e18c Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 27 Apr 2026 16:54:05 -0400 Subject: [PATCH 09/16] fix: add input size limits, marker spoofing protection, and smart history truncation Harden sticky comment lifecycle against resource exhaustion (2MB input cap), marker spoofing by untrusted users (author-based filtering), and sentinel corruption on overflow (drop oldest history blocks before byte-level truncation). Add validation for empty body/marker inputs. Signed-off-by: Wayne Sun --- internal/cli/readbody.go | 15 ++++++- internal/sticky/sticky.go | 71 ++++++++++++++++++++--------- internal/sticky/sticky_test.go | 81 +++++++++++++++++++++++++++++++--- 3 files changed, 140 insertions(+), 27 deletions(-) diff --git a/internal/cli/readbody.go b/internal/cli/readbody.go index 83ca415de5..6a10a2bfe5 100644 --- a/internal/cli/readbody.go +++ b/internal/cli/readbody.go @@ -6,16 +6,29 @@ import ( "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) { if path == "-" { - data, err := io.ReadAll(os.Stdin) + data, err := io.ReadAll(io.LimitReader(os.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 diff --git a/internal/sticky/sticky.go b/internal/sticky/sticky.go index c67b68c8c6..c33b604636 100644 --- a/internal/sticky/sticky.go +++ b/internal/sticky/sticky.go @@ -33,12 +33,21 @@ func (c Config) maxSize() int { // 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, _ := client.GetAuthenticatedUser(ctx) + comments, err := client.ListIssueComments(ctx, owner, repo, number) if err != nil { return fmt.Errorf("listing comments: %w", err) } - existing := FindMarkedComment(comments, cfg.Marker) + existing := FindMarkedComment(comments, cfg.Marker, botUser) markedBody := cfg.Marker + "\n" + body if existing != nil { @@ -75,9 +84,14 @@ func Post(ctx context.Context, client forge.Client, owner, repo string, number i } // FindMarkedComment returns the first comment whose body contains the -// given marker string, or nil if none is found. -func FindMarkedComment(comments []forge.IssueComment, marker string) *forge.IssueComment { +// 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] } @@ -132,30 +146,47 @@ func BuildUpdatedBody(oldBody, newBody string, cfg Config) string { currentOld := activeRe.ReplaceAllString(oldContent, "") currentOld = strings.TrimSpace(currentOld) - // Build flat history: the old current content becomes the newest history - // entry, followed by any previously accumulated history entries. - // Each block is wrapped with sentinel comments for safe extraction. - var collapsed strings.Builder + // 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 != "" { - collapsed.WriteString("\n\n
\nPrevious run\n\n") - collapsed.WriteString(historyStart + "\n") - collapsed.WriteString(currentOld) - collapsed.WriteString("\n" + historyEnd) - collapsed.WriteString("\n\n
") + entries = append(entries, histEntry{summary: "Previous run", content: currentOld}) } for i, block := range historyBlocks { - collapsed.WriteString(fmt.Sprintf("\n\n
\nPrevious run (%d)\n\n", i+2)) - collapsed.WriteString(historyStart + "\n") - collapsed.WriteString(strings.TrimSpace(block)) - collapsed.WriteString("\n" + historyEnd) - collapsed.WriteString("\n\n
") + entries = append(entries, histEntry{ + summary: fmt.Sprintf("Previous run (%d)", i+2), + content: strings.TrimSpace(block), + }) } - combined := newBody + collapsed.String() + 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() + } - // Re-append footer. + footerStr := "" if footer != "" { - combined += "\n\n" + 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() { diff --git a/internal/sticky/sticky_test.go b/internal/sticky/sticky_test.go index 5257b9a4cc..c2665c37cc 100644 --- a/internal/sticky/sticky_test.go +++ b/internal/sticky/sticky_test.go @@ -21,21 +21,42 @@ var testCfg = Config{ func TestFindMarkedComment(t *testing.T) { comments := []forge.IssueComment{ - {ID: 1, Body: "regular comment"}, - {ID: 2, Body: "\nmarked"}, - {ID: 3, Body: "another comment"}, + {ID: 1, Body: "regular comment", Author: "human"}, + {ID: 2, Body: "\nmarked", Author: "bot"}, + {ID: 3, Body: "another comment", Author: "human"}, } - found := FindMarkedComment(comments, "") + found := FindMarkedComment(comments, "", "bot") require.NotNil(t, found) assert.Equal(t, 2, found.ID) - notFound := FindMarkedComment(comments, "") + 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, "") + found := FindMarkedComment(nil, "", "bot") assert.Nil(t, found) } @@ -152,6 +173,24 @@ func TestTruncateBody_OverLimit(t *testing.T) { 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. @@ -212,6 +251,36 @@ func TestPost_DryRun(t *testing.T) { 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{ From 3c96bbac430c5a3412aaada14cb7bdb08bc581ab Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Tue, 28 Apr 2026 15:33:33 -0400 Subject: [PATCH 10/16] =?UTF-8?q?fix:=20address=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20CutPrefix,=20auth=20warning,=20readBody=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Use strings.CutPrefix for marker stripping to avoid matching markers embedded in review content body - Log a warning when GetAuthenticatedUser fails so degraded spoofing protection is observable - Extract readBodyFrom for testability and add unit tests covering stdin, file, oversize, and not-found paths Signed-off-by: Wayne Sun --- internal/cli/readbody.go | 8 ++++- internal/cli/readbody_test.go | 55 +++++++++++++++++++++++++++++++++++ internal/sticky/sticky.go | 12 +++++--- 3 files changed, 70 insertions(+), 5 deletions(-) create mode 100644 internal/cli/readbody_test.go diff --git a/internal/cli/readbody.go b/internal/cli/readbody.go index 6a10a2bfe5..9bf1217edb 100644 --- a/internal/cli/readbody.go +++ b/internal/cli/readbody.go @@ -11,8 +11,14 @@ 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(os.Stdin, maxBodyBytes+1)) + data, err := io.ReadAll(io.LimitReader(stdin, maxBodyBytes+1)) if err != nil { return "", fmt.Errorf("reading stdin: %w", err) } 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/sticky/sticky.go b/internal/sticky/sticky.go index c33b604636..801183828d 100644 --- a/internal/sticky/sticky.go +++ b/internal/sticky/sticky.go @@ -40,7 +40,10 @@ func Post(ctx context.Context, client forge.Client, owner, repo string, number i return fmt.Errorf("marker is empty") } - botUser, _ := client.GetAuthenticatedUser(ctx) + 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 { @@ -116,9 +119,10 @@ var legacyDetailsRe = regexp.MustCompile(`(?s)
\s*Previous [^<] //
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. - oldContent := strings.Replace(oldBody, cfg.Marker+"\n", "", 1) - oldContent = strings.Replace(oldContent, cfg.Marker, "", 1) + // 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 From 0fbcf30c4bf1fc162c15bd0b3a5194b237678bab Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Tue, 28 Apr 2026 16:03:30 -0400 Subject: [PATCH 11/16] feat: add stale-head detection, failure handling, and migrate post-review.sh Add GetPullRequestHeadSHA to forge interface for stale-head detection. The post-review CLI now validates the PR HEAD against the reviewed SHA before posting, handles action=failure with sticky failure notices, and accepts head_sha/reason fields from the JSON result schema. Simplify post-review.sh to delegate all review posting to the CLI, retaining only result file discovery and missing-result fallback. Signed-off-by: Wayne Sun --- internal/cli/postreview.go | 138 ++++++++++++++++-- internal/cli/postreview_test.go | 98 ++++++++++++- internal/forge/fake.go | 12 ++ internal/forge/forge.go | 3 + internal/forge/github/github.go | 19 +++ .../fullsend-repo/scripts/post-review.sh | 99 +++---------- 6 files changed, 270 insertions(+), 99 deletions(-) diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index bea1dcd060..5c14cddf86 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -19,25 +19,33 @@ const reviewMarker = "" func newPostReviewCmd() *cobra.Command { var ( - repo string - pr int - result string - token string - dryRun bool + 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. + 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. This prevents review comment -flooding on force-push, manual re-run, or workflow retry. +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 the review body text, -or reads from stdin if set to "-".`, +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) @@ -64,6 +72,11 @@ or reads from stdin if set to "-".`, return fmt.Errorf("parsing review result: %w", err) } + // CLI flag takes precedence over JSON field. + if headSHA != "" { + parsed.HeadSHA = headSHA + } + printer.Header("Post Review") client := gh.New(token) @@ -71,6 +84,25 @@ or reads from stdin if set to "-".`, 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, 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, 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 } @@ -81,8 +113,9 @@ or reads from stdin if set to "-".`, 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 body file, or '-' for stdin") + 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") @@ -92,8 +125,10 @@ or reads from stdin if set to "-".`, // ReviewResult represents a parsed review result file. type ReviewResult struct { - Body string `json:"body"` - Action string `json:"action"` // "approve", "request-changes", "comment" + 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. @@ -110,6 +145,78 @@ func reviewActionToEvent(action string) (string, bool) { } } +// checkStaleHead compares the reviewed SHA against the current PR HEAD. +// Returns true if the HEAD has moved (review is stale). +func checkStaleHead(ctx context.Context, client forge.Client, owner, repo string, pr int, reviewedSHA string, dryRun bool, printer *ui.Printer) (bool, 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 currentSHA != reviewedSHA { + printer.StepInfo(fmt.Sprintf("Stale: reviewed %s but HEAD is now %s", reviewedSHA[:minLen(len(reviewedSHA), 12)], currentSHA[:minLen(len(currentSHA), 12)])) + return true, nil + } + + printer.StepDone("HEAD matches reviewed SHA") + return false, nil +} + +func minLen(a, b int) int { + if a < b { + return a + } + return b +} + +// 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 string, cfg sticky.Config, printer *ui.Printer) error { + currentSHA, _ := client.GetPullRequestHeadSHA(ctx, owner, repo, pr) + + 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") + + 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.`, parsed.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. Minimizing first avoids a TOCTOU race // where the just-created review might not appear in the list yet. @@ -178,13 +285,14 @@ func minimizeStaleReviews(ctx context.Context, client forge.Client, owner, repo // 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. +// 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 == "" { + if result.Body == "" && strings.ToLower(result.Action) != "failure" { return ReviewResult{}, fmt.Errorf("review result JSON has empty body field") } if result.Action == "" { diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index 84c34f595e..ebe9e82e2f 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -10,6 +10,7 @@ import ( "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" ) @@ -44,6 +45,22 @@ func TestParseReviewResult_EmptyBody(t *testing.T) { 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 @@ -67,6 +84,83 @@ func TestReviewActionToEvent(t *testing.T) { } } +func TestCheckStaleHead_Matches(t *testing.T) { + fc := forge.NewFakeClient() + fc.PullRequestHeadSHA = "abc1234567890" + printer := ui.New(io.Discard) + + stale, err := checkStaleHead(context.Background(), fc, "o", "r", 1, "abc1234567890", false, printer) + require.NoError(t, err) + assert.False(t, stale) +} + +func TestCheckStaleHead_Stale(t *testing.T) { + fc := forge.NewFakeClient() + fc.PullRequestHeadSHA = "new_sha_456" + printer := ui.New(io.Discard) + + stale, err := checkStaleHead(context.Background(), fc, "o", "r", 1, "old_sha_123", false, printer) + require.NoError(t, err) + assert.True(t, stale) +} + +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", 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 TestSubmitFormalReview_CreatesAndMinimizesStale(t *testing.T) { fc := forge.NewFakeClient() fc.AuthenticatedUser = "fullsend-bot" @@ -83,12 +177,9 @@ func TestSubmitFormalReview_CreatesAndMinimizesStale(t *testing.T) { err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, parsed, false, printer) require.NoError(t, err) - // Should have created one review. require.Len(t, fc.CreatedReviews, 1) assert.Equal(t, "APPROVE", fc.CreatedReviews[0].Event) - // Stale reviews are minimized BEFORE creating the new one, so both - // existing reviews by fullsend-bot (IDs 100, 300) are minimized. require.Len(t, fc.MinimizedComments, 2) assert.Equal(t, "PRR_100", fc.MinimizedComments[0].NodeID) assert.Equal(t, "OUTDATED", fc.MinimizedComments[0].Reason) @@ -128,7 +219,6 @@ func TestMinimizeStaleReviews_MinimizesAll(t *testing.T) { printer := ui.New(io.Discard) err := minimizeStaleReviews(context.Background(), fc, "acme", "repo", 1, printer) require.NoError(t, err) - // Called before creating a new review, so the single existing review is stale. require.Len(t, fc.MinimizedComments, 1) assert.Equal(t, "PRR_100", fc.MinimizedComments[0].NodeID) } diff --git a/internal/forge/fake.go b/internal/forge/fake.go index a438a89e8f..11bf3e1db3 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -92,6 +92,9 @@ type FakeClient struct { // 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" @@ -624,6 +627,15 @@ func (f *FakeClient) MinimizeComment(_ context.Context, nodeID, reason string) e 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 string) error { f.mu.Lock() defer f.mu.Unlock() diff --git a/internal/forge/forge.go b/internal/forge/forge.go index 16c89b77dc..a78b1d6add 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -148,6 +148,9 @@ type Client interface { 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 CreatePullRequestReview(ctx context.Context, owner, repo string, number int, event, body string) error ListPullRequestReviews(ctx context.Context, owner, repo string, number int) ([]PullRequestReview, error) diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 259ec9ebc9..254c4bd748 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -1076,6 +1076,25 @@ func (c *LiveClient) MinimizeComment(ctx context.Context, nodeID, reason string) 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) + } + defer resp.Body.Close() + + var pr struct { + Head struct { + SHA string `json:"sha"` + } `json:"head"` + } + if err := json.NewDecoder(resp.Body).Decode(&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. func (c *LiveClient) CreatePullRequestReview(ctx context.Context, owner, repo string, number int, event, body string) error { 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}" From 6fae50105e61bb481b5df375b894d562e1e7afa1 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Tue, 28 Apr 2026 16:10:56 -0400 Subject: [PATCH 12/16] fix: eliminate swallowed error, add input validation, and harden review API - Pass currentSHA from checkStaleHead to postStaleHeadNotice instead of making a redundant API call that silently discards errors - Add positive integer validation for --pr and --number flags - Add event allowlist validation to CreatePullRequestReview, matching the pattern already used by MinimizeComment - Route shell fallback through fullsend post-review to get sticky comment deduplication on repeated no-output failures Signed-off-by: Wayne Sun --- internal/cli/postcomment.go | 4 ++++ internal/cli/postreview.go | 26 +++++++++++++++----------- internal/cli/postreview_test.go | 10 ++++++---- internal/forge/github/github.go | 6 ++++++ 4 files changed, 31 insertions(+), 15 deletions(-) diff --git a/internal/cli/postcomment.go b/internal/cli/postcomment.go index 4f5109bb33..af745e3bf0 100644 --- a/internal/cli/postcomment.go +++ b/internal/cli/postcomment.go @@ -45,6 +45,10 @@ The --result flag accepts a file path or "-" for stdin.`, 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) diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index 5c14cddf86..f85c0d16dc 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -56,6 +56,10 @@ has moved, a stale-head failure is posted instead.`, 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) @@ -88,12 +92,12 @@ has moved, a stale-head failure is posted instead.`, // Stale-head check: refuse to post a review against code // that has changed since the agent reviewed it. if parsed.HeadSHA != "" { - stale, err := checkStaleHead(cmd.Context(), client, owner, repoName, pr, parsed.HeadSHA, dryRun, printer) + 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, cfg, printer) + return postStaleHeadNotice(cmd.Context(), client, owner, repoName, pr, parsed.HeadSHA, currentSHA, cfg, printer) } } @@ -146,27 +150,29 @@ func reviewActionToEvent(action string) (string, bool) { } // checkStaleHead compares the reviewed SHA against the current PR HEAD. -// Returns true if the HEAD has moved (review is stale). -func checkStaleHead(ctx context.Context, client forge.Client, owner, repo string, pr int, reviewedSHA string, dryRun bool, printer *ui.Printer) (bool, error) { +// 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 + return false, "", nil } currentSHA, err := client.GetPullRequestHeadSHA(ctx, owner, repo, pr) if err != nil { - return false, fmt.Errorf("fetching PR HEAD: %w", err) + return false, "", fmt.Errorf("fetching PR HEAD: %w", err) } if currentSHA != reviewedSHA { printer.StepInfo(fmt.Sprintf("Stale: reviewed %s but HEAD is now %s", reviewedSHA[:minLen(len(reviewedSHA), 12)], currentSHA[:minLen(len(currentSHA), 12)])) - return true, nil + return true, currentSHA, nil } printer.StepDone("HEAD matches reviewed SHA") - return false, nil + return false, currentSHA, nil } func minLen(a, b int) int { @@ -178,9 +184,7 @@ func minLen(a, b int) int { // 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 string, cfg sticky.Config, printer *ui.Printer) error { - currentSHA, _ := client.GetPullRequestHeadSHA(ctx, owner, repo, pr) - +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 diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index ebe9e82e2f..d7b91c7e35 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -89,9 +89,10 @@ func TestCheckStaleHead_Matches(t *testing.T) { fc.PullRequestHeadSHA = "abc1234567890" printer := ui.New(io.Discard) - stale, err := checkStaleHead(context.Background(), fc, "o", "r", 1, "abc1234567890", false, printer) + 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) { @@ -99,16 +100,17 @@ func TestCheckStaleHead_Stale(t *testing.T) { fc.PullRequestHeadSHA = "new_sha_456" printer := ui.New(io.Discard) - stale, err := checkStaleHead(context.Background(), fc, "o", "r", 1, "old_sha_123", false, printer) + 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) + 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") } @@ -120,7 +122,7 @@ func TestPostStaleHeadNotice(t *testing.T) { printer := ui.New(io.Discard) cfg := sticky.Config{Marker: ""} - err := postStaleHeadNotice(context.Background(), fc, "o", "r", 1, "old_sha_123", cfg, printer) + 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") diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 254c4bd748..be96aa48da 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -1098,6 +1098,12 @@ func (c *LiveClient) GetPullRequestHeadSHA(ctx context.Context, owner, repo stri // CreatePullRequestReview submits a formal review on a pull request. // The event must be one of: APPROVE, REQUEST_CHANGES, COMMENT. func (c *LiveClient) CreatePullRequestReview(ctx context.Context, owner, repo string, number int, event, body 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, From 28c8e7a6584fe61eebad143f028dd1f97fba1451 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Tue, 28 Apr 2026 16:17:36 -0400 Subject: [PATCH 13/16] fix: restore action.yml to main's version after rebase conflict The rebase incorrectly took the branch's stale echo-stub version of action.yml instead of main's working fullsend-run version. Signed-off-by: Wayne Sun --- internal/forge/github/github.go | 3 +- .../.github/actions/fullsend/action.yml | 74 ++++++++++--------- 2 files changed, 42 insertions(+), 35 deletions(-) diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index be96aa48da..db57624043 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -1082,14 +1082,13 @@ func (c *LiveClient) GetPullRequestHeadSHA(ctx context.Context, owner, repo stri if err != nil { return "", fmt.Errorf("get pull request #%d: %w", number, err) } - defer resp.Body.Close() var pr struct { Head struct { SHA string `json:"sha"` } `json:"head"` } - if err := json.NewDecoder(resp.Body).Decode(&pr); err != nil { + if err := decodeJSON(resp, &pr); err != nil { return "", fmt.Errorf("decode pull request #%d: %w", number, err) } return pr.Head.SHA, nil diff --git a/internal/scaffold/fullsend-repo/.github/actions/fullsend/action.yml b/internal/scaffold/fullsend-repo/.github/actions/fullsend/action.yml index 84502579fe..8821fd4e30 100644 --- a/internal/scaffold/fullsend-repo/.github/actions/fullsend/action.yml +++ b/internal/scaffold/fullsend-repo/.github/actions/fullsend/action.yml @@ -1,8 +1,8 @@ name: Fullsend description: >- - Install fullsend-cli, run fullsend entrypoint (stdout must be the artifact directory path), then - upload that directory with actions/upload-artifact. Does not read agent configuration from the repository; - configure the job environment or CLI as needed. + Install fullsend-cli, create ${GITHUB_WORKSPACE}/output, invoke fullsend run with + --output-dir pointing at that directory, then upload it with actions/upload-artifact. Does not + read agent configuration from the repository; configure the job environment or CLI as needed. branding: icon: zap @@ -10,7 +10,7 @@ branding: inputs: agent: - description: Agent name passed to fullsend entrypoint (e.g. triage, code, review). + description: Agent name passed to fullsend run (e.g. triage, code, review). required: true version: description: >- @@ -18,16 +18,10 @@ inputs: names use the version without v (fullsend_0.0.1_linux_amd64.tar.gz). default: latest -outputs: - artifact_dir: - description: Absolute path printed by fullsend entrypoint on stdout (same path uploaded as an artifact). - value: ${{ steps.run-fullsend.outputs.artifact_dir }} - runs: using: composite steps: - - id: install-fullsend - name: Install fullsend CLI + - name: Install fullsend CLI shell: bash env: VERSION: ${{ inputs.version }} @@ -35,6 +29,18 @@ runs: RUNNER_ARCH: ${{ runner.arch }} run: | set -euo pipefail + + # Use vendored binary if present (placed by fullsend admin install --vendor-fullsend-binary). + # GitHub Contents API does not preserve the executable bit, so check -f not -x. + if [[ -f "${GITHUB_WORKSPACE}/bin/fullsend" ]]; then + echo "Using vendored fullsend binary from bin/fullsend" + mkdir -p "${RUNNER_TEMP}/fullsend" + cp "${GITHUB_WORKSPACE}/bin/fullsend" "${RUNNER_TEMP}/fullsend/fullsend" + chmod +x "${RUNNER_TEMP}/fullsend/fullsend" + echo "${RUNNER_TEMP}/fullsend" >> "${GITHUB_PATH}" + exit 0 + fi + VERSION="$(printf '%s' "${VERSION:-latest}" | tr -d '[:space:]')" VERSION="${VERSION:-latest}" @@ -85,34 +91,36 @@ runs: tar -xzf "/tmp/${ASSET_NAME}" -C "${RUNNER_TEMP}/fullsend" echo "${RUNNER_TEMP}/fullsend" >> "${GITHUB_PATH}" - - id: run-fullsend - name: Run fullsend + - name: Print fullsend version + shell: bash + run: fullsend --version + + - name: Install OpenShell + shell: bash + run: | + curl -LsSf https://raw.githubusercontent.com/NVIDIA/OpenShell/v0.0.30/install.sh | sh + openshell --version + + - name: Install validation dependencies + shell: bash + run: pip install --quiet "jsonschema>=4.18.0" + + - name: Run fullsend + if: inputs.agent != '__install_only__' shell: bash env: AGENT: ${{ inputs.agent }} run: | set -euo pipefail - # fullsend must print the artifact directory path on stdout (last line wins if multiple lines). - ARTIFACT_DIR="$(echo fullsend entrypoint "${AGENT}" | tail -n1)" - ARTIFACT_DIR="$(printf '%s' "${ARTIFACT_DIR}" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')" - if [[ -z "${ARTIFACT_DIR}" ]]; then - echo "::error::fullsend entrypoint did not print an artifact directory on stdout" - exit 1 - fi - # Validate path is absolute and within expected boundaries. - if [[ "${ARTIFACT_DIR}" != /* ]]; then - echo "::error::fullsend entrypoint returned a non-absolute path: ${ARTIFACT_DIR}" - exit 1 - fi - # Use heredoc delimiter to prevent GITHUB_OUTPUT injection via newlines. - { - echo "artifact_dir<> "${GITHUB_OUTPUT}" + mkdir -p "${GITHUB_WORKSPACE}/output" + fullsend run "${AGENT}" \ + --fullsend-dir "${GITHUB_WORKSPACE}" \ + --output-dir "${GITHUB_WORKSPACE}/output" \ + --target-repo "${GITHUB_WORKSPACE}/target-repo" - name: Upload fullsend artifacts - uses: actions/upload-artifact@v4 + if: always() && inputs.agent != '__install_only__' + uses: actions/upload-artifact@v7 with: name: fullsend-${{ inputs.agent }} - path: ${{ steps.run-fullsend.outputs.artifact_dir }} + path: ${{ github.workspace }}/output From 2f24b5207a44f6077bdd7af366e3d89a86a08fe4 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Tue, 28 Apr 2026 16:37:44 -0400 Subject: [PATCH 14/16] fix: pin review to commit SHA and validate head_sha input Pass commit_id to CreatePullRequestReview API call so GitHub rejects the review if the PR HEAD has moved between the stale-head check and review submission, closing the TOCTOU gap. Add hex regex validation for head_sha to prevent injection of arbitrary content into stale-head notices. Replace custom minLen helper with Go 1.21+ builtin min. Signed-off-by: Wayne Sun --- internal/cli/postreview.go | 30 +++++----- internal/cli/postreview_test.go | 58 ++++++++++++++++++-- internal/forge/fake.go | 14 +++-- internal/forge/forge.go | 6 +- internal/forge/github/github.go | 9 ++- internal/forge/github/github_comment_test.go | 21 ++++++- 6 files changed, 107 insertions(+), 31 deletions(-) diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index f85c0d16dc..52ccefdf99 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "os" + "regexp" "strings" "github.com/spf13/cobra" @@ -17,6 +18,8 @@ import ( const reviewMarker = "" +var hexSHARe = regexp.MustCompile(`^[0-9a-fA-F]{7,64}$`) + func newPostReviewCmd() *cobra.Command { var ( repo string @@ -80,6 +83,9 @@ has moved, a stale-head failure is posted instead.`, if headSHA != "" { parsed.HeadSHA = headSHA } + if parsed.HeadSHA != "" && !hexSHARe.MatchString(parsed.HeadSHA) { + return fmt.Errorf("head SHA must be a hex string (7-64 chars), got %q", parsed.HeadSHA) + } printer.Header("Post Review") @@ -111,7 +117,7 @@ has moved, a stale-head failure is posted instead.`, return err } - return submitFormalReview(cmd.Context(), client, owner, repoName, pr, parsed, dryRun, printer) + return submitFormalReview(cmd.Context(), client, owner, repoName, pr, parsed.Action, parsed.HeadSHA, dryRun, printer) }, } @@ -167,7 +173,7 @@ func checkStaleHead(ctx context.Context, client forge.Client, owner, repo string } if currentSHA != reviewedSHA { - printer.StepInfo(fmt.Sprintf("Stale: reviewed %s but HEAD is now %s", reviewedSHA[:minLen(len(reviewedSHA), 12)], currentSHA[:minLen(len(currentSHA), 12)])) + 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 } @@ -175,13 +181,6 @@ func checkStaleHead(ctx context.Context, client forge.Client, owner, repo string return false, currentSHA, nil } -func minLen(a, b int) int { - if a < b { - return a - } - return b -} - // 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 { @@ -222,12 +221,13 @@ This PR was NOT reviewed. Do not count this as an approval.`, parsed.Reason) } // submitFormalReview minimizes stale reviews by the same user, then -// submits a new GitHub PR review. Minimizing first avoids a TOCTOU race -// where the just-created review might not appear in the list yet. -func submitFormalReview(ctx context.Context, client forge.Client, owner, repo string, pr int, parsed ReviewResult, dryRun bool, printer *ui.Printer) error { - event, ok := reviewActionToEvent(parsed.Action) +// 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", parsed.Action)) + printer.StepInfo(fmt.Sprintf("Unknown review action %q, skipping formal review", action)) return nil } @@ -242,7 +242,7 @@ func submitFormalReview(ctx context.Context, client forge.Client, owner, repo st 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); err != nil { + if err := client.CreatePullRequestReview(ctx, owner, repo, pr, event, reviewBody, commitSHA); err != nil { return fmt.Errorf("submitting review: %w", err) } printer.StepDone("Review submitted") diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index d7b91c7e35..b4a744ad5f 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -175,12 +175,12 @@ func TestSubmitFormalReview_CreatesAndMinimizesStale(t *testing.T) { } printer := ui.New(io.Discard) - parsed := ReviewResult{Body: "New findings", Action: "approve"} - err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, parsed, false, printer) + 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) @@ -192,9 +192,8 @@ func TestSubmitFormalReview_CreatesAndMinimizesStale(t *testing.T) { func TestSubmitFormalReview_DryRun(t *testing.T) { fc := forge.NewFakeClient() printer := ui.New(io.Discard) - parsed := ReviewResult{Body: "Findings", Action: "approve"} - err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, parsed, true, printer) + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, "approve", "", true, printer) require.NoError(t, err) assert.Empty(t, fc.CreatedReviews) } @@ -202,9 +201,8 @@ func TestSubmitFormalReview_DryRun(t *testing.T) { func TestSubmitFormalReview_UnknownAction(t *testing.T) { fc := forge.NewFakeClient() printer := ui.New(io.Discard) - parsed := ReviewResult{Body: "Findings", Action: "unknown-action"} - err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, parsed, false, printer) + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, "unknown-action", "", false, printer) require.NoError(t, err) assert.Empty(t, fc.CreatedReviews) } @@ -250,3 +248,51 @@ func TestMinimizeStaleReviews_NoReviews(t *testing.T) { require.NoError(t, err) assert.Empty(t, fc.MinimizedComments) } + +func TestHexSHAValidation(t *testing.T) { + tests := []struct { + sha string + valid bool + }{ + {"abc123f", true}, + {"abc123def456", true}, + {"abc123def456abc123def456abc123def456abc123", true}, + {"", true}, // empty is valid (means "no SHA provided") + {"not-hex!", false}, + {"abc 123", false}, + {"abc123`inject", false}, + {"ABC123DEF456", true}, + } + 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 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/forge/fake.go b/internal/forge/fake.go index 11bf3e1db3..7d6e3ad6d9 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -60,6 +60,7 @@ type ReviewRecord struct { Owner, Repo string Number int Event, Body string + CommitSHA string } // FakeClient is a thread-safe test double for forge.Client. @@ -636,18 +637,19 @@ func (f *FakeClient) GetPullRequestHeadSHA(_ context.Context, _, _ string, _ int return f.PullRequestHeadSHA, nil } -func (f *FakeClient) CreatePullRequestReview(_ context.Context, owner, repo string, number int, event, body string) error { +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, + Owner: owner, + Repo: repo, + Number: number, + Event: event, + Body: body, + CommitSHA: commitSHA, }) review := PullRequestReview{ diff --git a/internal/forge/forge.go b/internal/forge/forge.go index a78b1d6add..113b6a3ca4 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -151,8 +151,10 @@ type Client interface { // Pull request operations GetPullRequestHeadSHA(ctx context.Context, owner, repo string, number int) (string, error) - // Pull request review operations - CreatePullRequestReview(ctx context.Context, owner, repo string, number int, event, body 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 diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index db57624043..d316fb9434 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -1096,7 +1096,11 @@ func (c *LiveClient) GetPullRequestHeadSHA(ctx context.Context, owner, repo stri // CreatePullRequestReview submits a formal review on a pull request. // The event must be one of: APPROVE, REQUEST_CHANGES, COMMENT. -func (c *LiveClient) CreatePullRequestReview(ctx context.Context, owner, repo string, number int, event, body string) error { +// 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: @@ -1107,6 +1111,9 @@ func (c *LiveClient) CreatePullRequestReview(ctx context.Context, owner, repo st "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) diff --git a/internal/forge/github/github_comment_test.go b/internal/forge/github/github_comment_test.go index eda858bfdd..d30af855d6 100644 --- a/internal/forge/github/github_comment_test.go +++ b/internal/forge/github/github_comment_test.go @@ -206,6 +206,7 @@ func TestCreatePullRequestReview(t *testing.T) { 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}) @@ -213,7 +214,25 @@ func TestCreatePullRequestReview(t *testing.T) { defer srv.Close() client := newTestClient(t, srv) - err := client.CreatePullRequestReview(context.Background(), "owner", "repo", 7, "APPROVE", "Looks good!") + 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) } From 19f50cdfbcecd7b92088c3e104442f669a112f77 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Tue, 28 Apr 2026 16:44:29 -0400 Subject: [PATCH 15/16] fix: require full SHA for head_sha and sanitize failure reason Tighten hexSHARe regex from {7,64} to exactly 40 (SHA-1) or 64 (SHA-256) characters. Short SHAs would always mismatch the 40-char API response, causing every review to be falsely treated as stale. Validate the failure reason field against an alphanumeric+hyphen allowlist to prevent markdown injection via crafted agent result JSON. Signed-off-by: Wayne Sun --- internal/cli/postreview.go | 10 +++++++-- internal/cli/postreview_test.go | 38 ++++++++++++++++++++++++++------- 2 files changed, 38 insertions(+), 10 deletions(-) diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index 52ccefdf99..42be35d94b 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -18,7 +18,8 @@ import ( const reviewMarker = "" -var hexSHARe = regexp.MustCompile(`^[0-9a-fA-F]{7,64}$`) +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 ( @@ -201,6 +202,11 @@ The review agent reviewed commit `+"`%s`"+` but the PR HEAD is now `+"`%s`"+`. T 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 !reasonRe.MatchString(reason) { + reason = "invalid-reason" + } + var body string if parsed.Body != "" { body = parsed.Body @@ -210,7 +216,7 @@ func postFailureNotice(ctx context.Context, client forge.Client, owner, repo str **Outcome:** failure **Reason:** %s -This PR was NOT reviewed. Do not count this as an approval.`, parsed.Reason) +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 { diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index b4a744ad5f..bee3e790d1 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -254,14 +254,16 @@ func TestHexSHAValidation(t *testing.T) { sha string valid bool }{ - {"abc123f", true}, - {"abc123def456", true}, - {"abc123def456abc123def456abc123def456abc123", true}, - {"", true}, // empty is valid (means "no SHA provided") - {"not-hex!", false}, - {"abc 123", false}, - {"abc123`inject", false}, - {"ABC123DEF456", true}, + {"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) { @@ -274,6 +276,26 @@ func TestHexSHAValidation(t *testing.T) { } } +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" From 6c4a81a3f7f093b35e8f067b7201ed8f93416803 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Tue, 28 Apr 2026 16:50:44 -0400 Subject: [PATCH 16/16] fix: case-insensitive SHA comparison, empty-reason default, error message accuracy - Use strings.EqualFold for SHA comparison (defense-in-depth against mixed-case SHAs from different sources) - Default empty failure reason to "unknown" instead of passing through empty string to the notice template - Fix error message to say "40 or 64 character" matching actual regex Signed-off-by: Wayne Sun --- internal/cli/postreview.go | 8 +++++--- internal/cli/postreview_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index 42be35d94b..d360d6a862 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -85,7 +85,7 @@ has moved, a stale-head failure is posted instead.`, parsed.HeadSHA = headSHA } if parsed.HeadSHA != "" && !hexSHARe.MatchString(parsed.HeadSHA) { - return fmt.Errorf("head SHA must be a hex string (7-64 chars), got %q", parsed.HeadSHA) + return fmt.Errorf("head SHA must be a 40 or 64 character hex string, got %q", parsed.HeadSHA) } printer.Header("Post Review") @@ -173,7 +173,7 @@ func checkStaleHead(ctx context.Context, client forge.Client, owner, repo string return false, "", fmt.Errorf("fetching PR HEAD: %w", err) } - if currentSHA != reviewedSHA { + 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 } @@ -203,7 +203,9 @@ func postFailureNotice(ctx context.Context, client forge.Client, owner, repo str printer.StepStart("Review agent reported failure, posting notice") reason := parsed.Reason - if !reasonRe.MatchString(reason) { + if reason == "" { + reason = "unknown" + } else if !reasonRe.MatchString(reason) { reason = "invalid-reason" } diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index bee3e790d1..b31e3a0864 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -163,6 +163,32 @@ func TestPostFailureNotice_WithoutBody(t *testing.T) { 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"