From 4bdc67ba35e2d96d2d8ffc88397e74902d0348ca Mon Sep 17 00:00:00 2001 From: Leonardo Araujo Date: Thu, 28 May 2026 23:50:00 -0300 Subject: [PATCH] feat(native): dedupe PR review comments with replace strategy Issue comments from pull_request.post_comment now default to comment_strategy replace: list issue comments, PATCH the body containing , or POST once with the marker embedded. Supports append, upsert alias, and comment_id for direct updates. Example workflows and GITHUB_ACTIONS docs updated. Closes #94 Co-authored-by: Cursor --- docs/GITHUB_ACTIONS.md | 14 +- examples/pr-review-github-actions/README.md | 2 +- .../workflows/pr-review-github.yaml | 5 +- examples/pr-review-github/README.md | 11 +- .../workflows/pr-review-github.yaml | 6 +- internal/tools/native/doc.go | 4 +- internal/tools/native/github.go | 62 +--- internal/tools/native/github_comments.go | 276 ++++++++++++++++++ internal/tools/native/github_test.go | 224 +++++++++++++- internal/tools/native/registry.go | 16 +- test/integration/cli_flow_test.go | 3 + 11 files changed, 542 insertions(+), 81 deletions(-) create mode 100644 internal/tools/native/github_comments.go diff --git a/docs/GITHUB_ACTIONS.md b/docs/GITHUB_ACTIONS.md index d4db608..68ce17c 100644 --- a/docs/GITHUB_ACTIONS.md +++ b/docs/GITHUB_ACTIONS.md @@ -50,8 +50,9 @@ Configure repository secret **`OPENAI_API_KEY`** — the example project’s rev you need: - **`contents: read`** — checkout. - **`pull-requests: write`** — required for the bundled **`review`** job: it runs - **`agentctl run … --approve tool.github.pull_request.post_comment`**, which posts a real issue - comment on the PR after the model review. + **`agentctl run … --approve tool.github.pull_request.post_comment`**, which creates or updates a + single issue comment on the PR after the model review (default **`comment_strategy: replace`** + updates a comment containing **``** instead of posting anew on every push). - The optional **`post-pointer`** job (**`gh pr comment`**) also needs **`pull-requests: write`** when **`AGENTIC_GH_PR_COMMENT: "true"`** (short pointer to the Actions run). @@ -83,6 +84,15 @@ job without failing CI. Hard failures use **1**, **2**, **3**, **4** (see **section 11.2** in **`DESIGN_DOC.md`**). +### Sticky review comment (`synchronize` / re-runs) + +The example workflow sets **`comment_strategy: replace`** on **`pull_request.post_comment`**. On each +approved run the native tool lists issue comments on the PR, finds one whose body contains the HTML +marker **``**, and **`PATCH`**es it; if none exists it **`POST`**s once and +embeds the marker. That avoids a new full review comment on every push. Use **`comment_strategy: append`** +in your workflow YAML if you prefer a new comment per run, or **`comment_id`** to update a specific +comment. **`upsert: true`** is an alias for **`replace`**. + --- ## Installing `agentctl` in Actions diff --git a/examples/pr-review-github-actions/README.md b/examples/pr-review-github-actions/README.md index 9d4203a..f63cc7b 100644 --- a/examples/pr-review-github-actions/README.md +++ b/examples/pr-review-github-actions/README.md @@ -13,7 +13,7 @@ For the **mock-only** live GitHub path (no OpenAI key, good for CI and integrati |------|---------| | `project.yaml` | Imports policies, tools, agent, workflow; **`defaults.model: openai/gpt-4o-mini`**; **`OPENAI_API_KEY`** via `apiKeyFrom` | | `agents/reviewer.yaml` | **`spec.model: openai/gpt-4o-mini`**, structured JSON output | -| `workflows/pr-review-github.yaml` | GitHub REST read → reviewer → `post_comment` (CI passes `--approve`) | +| `workflows/pr-review-github.yaml` | GitHub REST read → reviewer → `post_comment` with **`comment_strategy: replace`** (one sticky comment per PR) | | [`.github/workflows/agentctl-pr-review.yml`](../../.github/workflows/agentctl-pr-review.yml) | Runs on PRs; **`AGENTIC_PROJECT`** = **`examples/pr-review-github-actions`** | | [`.github/workflows/agentctl-pr-review-publish.yml`](../../.github/workflows/agentctl-pr-review-publish.yml) | Optional manual **`workflow_dispatch`** to post an approved PR comment | diff --git a/examples/pr-review-github-actions/workflows/pr-review-github.yaml b/examples/pr-review-github-actions/workflows/pr-review-github.yaml index fa6c895..af0e49b 100644 --- a/examples/pr-review-github-actions/workflows/pr-review-github.yaml +++ b/examples/pr-review-github-actions/workflows/pr-review-github.yaml @@ -5,8 +5,8 @@ metadata: spec: description: | Live GitHub path: fetch PR JSON and unified diff via the GitHub REST API, run a structured review - with the default project model (OpenAI gpt-4o-mini), then post an issue comment on the PR (real - POST when owner/repo/number/body are set and GITHUB_TOKEN is present; otherwise simulated). + with the default project model (OpenAI gpt-4o-mini), then post or update one issue comment on the PR + (comment_strategy replace by default: PATCH existing comment or POST once). The comment step is policy-gated unless you pass --approve for tool.github.pull_request.post_comment (the bundled Actions PR job passes --approve so a real comment is posted). Requires GITHUB_TOKEN, OPENAI_API_KEY (for the reviewer agent), and network access to GitHub/OpenAI. @@ -37,6 +37,7 @@ spec: owner: ${input.owner} repo: ${input.repo} number: ${input.number} + comment_strategy: replace body: | ## Automated review diff --git a/examples/pr-review-github/README.md b/examples/pr-review-github/README.md index aaa1426..6fae293 100644 --- a/examples/pr-review-github/README.md +++ b/examples/pr-review-github/README.md @@ -6,10 +6,11 @@ This example wires **Phase B + C** of the GitHub integration: - **Read:** `pull_request.get` and `pull_request.diff` against the GitHub REST API. - **Review:** structured **mock** model output validated by JSON Schema. -- **Write:** `pull_request.post_comment` performs a **real** `POST …/issues/{n}/comments` when - `owner`, `repo`, `number`, and `body` are set and **`GITHUB_TOKEN` is present**; otherwise it stays - **simulated** (as in `examples/pr-review-demo`). The comment step remains **policy-gated** unless - you pass `--approve tool.github.pull_request.post_comment`. +- **Write:** `pull_request.post_comment` creates or updates an issue comment when `owner`, `repo`, + `number`, and `body` are set and **`GITHUB_TOKEN` is present** (default **`comment_strategy: replace`** + patches a comment containing **``**; use **`append`** for a new comment each run). + Without repo context it stays **simulated** (as in `examples/pr-review-demo`). The step is **policy-gated** + unless you pass `--approve tool.github.pull_request.post_comment`. ## Prerequisites @@ -61,7 +62,7 @@ agentctl run workflow/pr-review-github \ ## CI / tests `go test ./test/integration/...` starts an HTTP stub and sets `GITHUB_API_URL` so the workflow runs -without touching GitHub, including an **approved** run that exercises the live comment `POST` path. +without touching GitHub, including an **approved** run that exercises the live comment list + `POST` path. ## GitHub Actions diff --git a/examples/pr-review-github/workflows/pr-review-github.yaml b/examples/pr-review-github/workflows/pr-review-github.yaml index 36b5437..1d4ec0e 100644 --- a/examples/pr-review-github/workflows/pr-review-github.yaml +++ b/examples/pr-review-github/workflows/pr-review-github.yaml @@ -5,9 +5,8 @@ metadata: spec: description: | Live GitHub path: fetch PR JSON and unified diff via the GitHub REST API, run a structured review, - then post an issue comment on the PR (real POST when owner/repo/number/body are set and - GITHUB_TOKEN is present; otherwise simulated). The comment step is policy-gated unless you pass - --approve for tool.github.pull_request.post_comment. + then post or update one issue comment (comment_strategy replace: PATCH or POST once). + Policy-gated unless you pass --approve for tool.github.pull_request.post_comment. Requires GITHUB_TOKEN and network access to GITHUB_API_URL (default https://api.github.com). policy: guarded-writes input: @@ -36,6 +35,7 @@ spec: owner: ${input.owner} repo: ${input.repo} number: ${input.number} + comment_strategy: replace body: | ## Automated review diff --git a/internal/tools/native/doc.go b/internal/tools/native/doc.go index 9cdca17..a3170cd 100644 --- a/internal/tools/native/doc.go +++ b/internal/tools/native/doc.go @@ -3,7 +3,9 @@ // // Live reads: pull_request.get, pull_request.diff, check_runs.list. // pull_request.post_comment is simulated unless owner, repo, number, and body are all set, in which -// case it POSTs to the issue comments API (PRs use the same issue number). +// case it writes to the issue comments API (PRs use the same issue number). By default comment_strategy +// is replace: find a comment containing and PATCH it, or POST once. Use +// comment_strategy append to always create a new comment. Optional comment_id forces PATCH on that id. // // GITHUB_API_URL overrides the REST base URL (default https://api.github.com), e.g. for tests. package native diff --git a/internal/tools/native/github.go b/internal/tools/native/github.go index 30c35d8..b9a8b08 100644 --- a/internal/tools/native/github.go +++ b/internal/tools/native/github.go @@ -1,7 +1,6 @@ package native import ( - "bytes" "context" "encoding/json" "fmt" @@ -70,24 +69,6 @@ func githubPullRequestDiff(ctx context.Context, with map[string]any) (map[string return map[string]any{"diff": text}, nil } -func githubPullRequestPostComment(ctx context.Context, owner, repo, number, body string) (map[string]any, error) { - path := fmt.Sprintf("/repos/%s/%s/issues/%s/comments", url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(number)) - payload := map[string]string{"body": body} - b, err := githubPOSTJSON(ctx, path, payload, maxGitHubJSONBody) - if err != nil { - return nil, err - } - var out map[string]any - if err := json.Unmarshal(b, &out); err != nil { - return nil, fmt.Errorf("native: pull_request.post_comment decode: %w", err) - } - if out == nil { - out = map[string]any{} - } - out["simulated"] = false - return out, nil -} - // githubLivePostCommentContext reports whether step inputs request a real GitHub issue comment: // non-empty owner, repo, number (or pull_number), and body. When true, post_comment uses the REST // API if GITHUB_TOKEN is set; otherwise the demo stays fully offline (simulated). @@ -118,32 +99,14 @@ func tryStringFromWith(with map[string]any, keys ...string) (string, bool) { } func githubPOSTJSON(ctx context.Context, path string, payload any, maxResp int64) ([]byte, error) { - token, err := githubToken() - if err != nil { - return nil, err - } - bodyBytes, err := json.Marshal(payload) - if err != nil { - return nil, fmt.Errorf("native: github encode body: %w", err) - } - fullURL := strings.TrimSuffix(githubAPIBase(), "/") + path - req, err := http.NewRequestWithContext(ctx, http.MethodPost, fullURL, bytes.NewReader(bodyBytes)) - if err != nil { - return nil, err - } - req.Header.Set("Authorization", "Bearer "+token) - req.Header.Set("User-Agent", githubUserAgent) - req.Header.Set("Accept", githubAcceptJSON) - req.Header.Set("Content-Type", githubAcceptJSON) - req.Header.Set("X-GitHub-Api-Version", githubAPIVersion) + return githubJSONRequest(ctx, http.MethodPost, path, payload, maxResp) +} - cli := &http.Client{Timeout: 60 * time.Second} - resp, err := cli.Do(req) - if err != nil { - return nil, fmt.Errorf("native: github request: %w", err) - } - defer resp.Body.Close() +func defaultGitHubHTTPClient() *http.Client { + return &http.Client{Timeout: 60 * time.Second} +} +func readGitHubResponseBody(resp *http.Response, maxResp int64) ([]byte, error) { limited := io.LimitReader(resp.Body, maxResp+1) b, err := io.ReadAll(limited) if err != nil { @@ -152,9 +115,6 @@ func githubPOSTJSON(ctx context.Context, path string, payload any, maxResp int64 if int64(len(b)) > maxResp { return nil, fmt.Errorf("native: github response body exceeds limit (%d bytes)", maxResp) } - if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return nil, fmt.Errorf("native: github HTTP %s: %s", resp.Status, truncateRunes(string(b), 512)) - } return b, nil } @@ -269,20 +229,16 @@ func githubRequestBody(ctx context.Context, method, path, accept string, maxBody req.Header.Set("X-GitHub-Api-Version", githubAPIVersion) } - cli := &http.Client{Timeout: 60 * time.Second} + cli := defaultGitHubHTTPClient() resp, err := cli.Do(req) if err != nil { return nil, fmt.Errorf("native: github request: %w", err) } defer resp.Body.Close() - limited := io.LimitReader(resp.Body, maxBody+1) - b, err := io.ReadAll(limited) + b, err := readGitHubResponseBody(resp, maxBody) if err != nil { - return nil, fmt.Errorf("native: github read body: %w", err) - } - if int64(len(b)) > maxBody { - return nil, fmt.Errorf("native: github response body exceeds limit (%d bytes)", maxBody) + return nil, err } if resp.StatusCode < 200 || resp.StatusCode >= 300 { return nil, fmt.Errorf("native: github HTTP %s: %s", resp.Status, truncateRunes(string(b), 512)) diff --git a/internal/tools/native/github_comments.go b/internal/tools/native/github_comments.go new file mode 100644 index 0000000..32c7c36 --- /dev/null +++ b/internal/tools/native/github_comments.go @@ -0,0 +1,276 @@ +package native + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strconv" + "strings" +) + +// AgenticReviewMarker is embedded in automated PR review issue comments so synchronize +// runs can find and update the same comment instead of posting anew. +const AgenticReviewMarker = "" + +const ( + commentStrategyAppend = "append" + commentStrategyReplace = "replace" + + githubCommentsPerPage = 100 + githubCommentsMaxPages = 10 +) + +// githubCommentStrategy returns append or replace for live post_comment calls. +// Default is replace so PR synchronize events do not spam issue comments. +// upsert: true is an alias for replace. +func githubCommentStrategy(with map[string]any) (string, error) { + if upsert, ok := with["upsert"]; ok { + b, err := scalarToBool(upsert) + if err != nil { + return "", fmt.Errorf("native: post_comment upsert: %w", err) + } + if b { + return commentStrategyReplace, nil + } + } + if s, ok := tryStringFromWith(with, "comment_strategy"); ok { + switch strings.ToLower(s) { + case commentStrategyAppend: + return commentStrategyAppend, nil + case commentStrategyReplace: + return commentStrategyReplace, nil + default: + return "", fmt.Errorf("native: post_comment comment_strategy must be %q or %q, got %q", + commentStrategyAppend, commentStrategyReplace, s) + } + } + return commentStrategyReplace, nil +} + +func scalarToBool(v any) (bool, error) { + switch x := v.(type) { + case bool: + return x, nil + case string: + s := strings.TrimSpace(strings.ToLower(x)) + switch s { + case "true", "1", "yes": + return true, nil + case "false", "0", "no": + return false, nil + default: + return false, fmt.Errorf("unsupported bool string %q", x) + } + default: + return false, fmt.Errorf("unsupported type %T", v) + } +} + +func ensureAgenticReviewMarker(body string) string { + if strings.Contains(body, AgenticReviewMarker) { + return body + } + body = strings.TrimRight(body, "\n") + if body == "" { + return AgenticReviewMarker + } + return body + "\n\n" + AgenticReviewMarker +} + +// githubPullRequestPostComment posts or updates an issue comment on a PR. +// strategy is append (always POST) or replace (PATCH existing marker comment or POST once). +func githubPullRequestPostComment(ctx context.Context, owner, repo, number, body, strategy string) (map[string]any, error) { + switch strategy { + case commentStrategyAppend: + return githubPullRequestCreateComment(ctx, owner, repo, number, body) + case commentStrategyReplace: + return githubPullRequestReplaceComment(ctx, owner, repo, number, body) + default: + return nil, fmt.Errorf("native: post_comment unknown strategy %q", strategy) + } +} + +func githubPullRequestCreateComment(ctx context.Context, owner, repo, number, body string) (map[string]any, error) { + path := fmt.Sprintf("/repos/%s/%s/issues/%s/comments", url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(number)) + payload := map[string]string{"body": body} + b, err := githubPOSTJSON(ctx, path, payload, maxGitHubJSONBody) + if err != nil { + return nil, err + } + return decodeGitHubCommentResponse(b, false) +} + +func githubPullRequestReplaceComment(ctx context.Context, owner, repo, number, body string) (map[string]any, error) { + body = ensureAgenticReviewMarker(body) + existingID, err := githubFindAgenticReviewCommentID(ctx, owner, repo, number) + if err != nil { + return nil, err + } + if existingID != "" { + return githubUpdateIssueComment(ctx, owner, repo, existingID, body) + } + out, err := githubPullRequestCreateComment(ctx, owner, repo, number, body) + if err != nil { + return nil, err + } + out["created"] = true + out["updated"] = false + return out, nil +} + +func githubFindAgenticReviewCommentID(ctx context.Context, owner, repo, number string) (string, error) { + for page := 1; page <= githubCommentsMaxPages; page++ { + comments, err := githubListIssueCommentsPage(ctx, owner, repo, number, page) + if err != nil { + return "", err + } + for _, c := range comments { + id, body, ok := commentIDAndBody(c) + if !ok { + continue + } + if strings.Contains(body, AgenticReviewMarker) { + return id, nil + } + } + if len(comments) < githubCommentsPerPage { + break + } + } + return "", nil +} + +func commentIDAndBody(c map[string]any) (id, body string, ok bool) { + rawID, okID := c["id"] + if !okID || rawID == nil { + return "", "", false + } + idStr, err := scalarToString(rawID) + if err != nil || idStr == "" { + return "", "", false + } + rawBody, okBody := c["body"] + if !okBody || rawBody == nil { + return "", "", false + } + bodyStr, err := scalarToString(rawBody) + if err != nil { + return "", "", false + } + return idStr, bodyStr, true +} + +func githubListIssueCommentsPage(ctx context.Context, owner, repo, number string, page int) ([]map[string]any, error) { + path := fmt.Sprintf("/repos/%s/%s/issues/%s/comments?per_page=%d&page=%d", + url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(number), + githubCommentsPerPage, page) + b, err := githubGET(ctx, path, githubAcceptJSON, maxGitHubJSONBody) + if err != nil { + return nil, err + } + var comments []map[string]any + if err := json.Unmarshal(b, &comments); err != nil { + return nil, fmt.Errorf("native: list issue comments decode: %w", err) + } + return comments, nil +} + +func githubUpdateIssueComment(ctx context.Context, owner, repo, commentID, body string) (map[string]any, error) { + path := fmt.Sprintf("/repos/%s/%s/issues/comments/%s", + url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(commentID)) + payload := map[string]string{"body": body} + b, err := githubPATCHJSON(ctx, path, payload, maxGitHubJSONBody) + if err != nil { + return nil, err + } + out, err := decodeGitHubCommentResponse(b, true) + if err != nil { + return nil, err + } + out["updated"] = true + out["created"] = false + return out, nil +} + +func decodeGitHubCommentResponse(b []byte, updated bool) (map[string]any, error) { + var out map[string]any + if err := json.Unmarshal(b, &out); err != nil { + return nil, fmt.Errorf("native: pull_request.post_comment decode: %w", err) + } + if out == nil { + out = map[string]any{} + } + out["simulated"] = false + out["updated"] = updated + out["created"] = !updated + return out, nil +} + +func githubPATCHJSON(ctx context.Context, path string, payload any, maxResp int64) ([]byte, error) { + return githubJSONRequest(ctx, http.MethodPatch, path, payload, maxResp) +} + +func githubJSONRequest(ctx context.Context, method, path string, payload any, maxResp int64) ([]byte, error) { + token, err := githubToken() + if err != nil { + return nil, err + } + var body io.Reader + if payload != nil { + bodyBytes, err := json.Marshal(payload) + if err != nil { + return nil, fmt.Errorf("native: github encode body: %w", err) + } + body = bytes.NewReader(bodyBytes) + } + fullURL := strings.TrimSuffix(githubAPIBase(), "/") + path + req, err := http.NewRequestWithContext(ctx, method, fullURL, body) + if err != nil { + return nil, err + } + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("User-Agent", githubUserAgent) + req.Header.Set("Accept", githubAcceptJSON) + if body != nil { + req.Header.Set("Content-Type", githubAcceptJSON) + } + req.Header.Set("X-GitHub-Api-Version", githubAPIVersion) + + cli := defaultGitHubHTTPClient() + resp, err := cli.Do(req) + if err != nil { + return nil, fmt.Errorf("native: github request: %w", err) + } + defer resp.Body.Close() + + b, err := readGitHubResponseBody(resp, maxResp) + if err != nil { + return nil, err + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return nil, fmt.Errorf("native: github HTTP %s: %s", resp.Status, truncateRunes(string(b), 512)) + } + return b, nil +} + +// commentIDFromWith returns an explicit issue comment id for replace strategy. +func commentIDFromWith(with map[string]any) (string, bool) { + return tryStringFromWith(with, "comment_id") +} + +func githubPullRequestReplaceCommentByID(ctx context.Context, owner, repo, commentID, body string) (map[string]any, error) { + body = ensureAgenticReviewMarker(body) + return githubUpdateIssueComment(ctx, owner, repo, commentID, body) +} + +// parseCommentID validates a numeric GitHub comment id string. +func parseCommentID(id string) error { + if _, err := strconv.ParseInt(strings.TrimSpace(id), 10, 64); err != nil { + return fmt.Errorf("native: post_comment comment_id must be a numeric id: %w", err) + } + return nil +} diff --git a/internal/tools/native/github_test.go b/internal/tools/native/github_test.go index ed6cb5c..43fd4cb 100644 --- a/internal/tools/native/github_test.go +++ b/internal/tools/native/github_test.go @@ -138,21 +138,24 @@ func TestGithubPostComment_simulatedWhenRepoContextMissing(t *testing.T) { func TestGithubPostComment_liveCreatesIssueComment(t *testing.T) { t.Setenv("GITHUB_TOKEN", "tok") + var postBodies []string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/repos/o/r/issues/3/comments" || r.Method != http.MethodPost { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/repos/o/r/issues/3/comments": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[]`)) + case r.Method == http.MethodPost && r.URL.Path == "/repos/o/r/issues/3/comments": + b, err := io.ReadAll(r.Body) + if err != nil { + t.Fatal(err) + } + postBodies = append(postBodies, string(b)) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":42,"html_url":"https://github.com/o/r/issues/3#issuecomment-42"}`)) + default: http.NotFound(w, r) - return - } - b, err := io.ReadAll(r.Body) - if err != nil { - t.Fatal(err) } - if !strings.Contains(string(b), "hi") { - t.Fatalf("body json: %s", string(b)) - } - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusCreated) - _, _ = w.Write([]byte(`{"id":42,"html_url":"https://github.com/o/r/issues/3#issuecomment-42"}`)) })) t.Cleanup(srv.Close) t.Setenv("GITHUB_API_URL", srv.URL) @@ -170,6 +173,203 @@ func TestGithubPostComment_liveCreatesIssueComment(t *testing.T) { if out["id"].(float64) != 42 { t.Fatalf("id %#v", out["id"]) } + if out["created"] != true { + t.Fatalf("created %#v", out["created"]) + } + if len(postBodies) != 1 || !strings.Contains(postBodies[0], "agentic-review") { + t.Fatalf("POST body should include marker: %v", postBodies) + } +} + +func TestGithubPostComment_appendSkipsListAndMarker(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "tok") + var gotGET bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + gotGET = true + } + if r.Method == http.MethodPost && r.URL.Path == "/repos/o/r/issues/1/comments" { + b, _ := io.ReadAll(r.Body) + if strings.Contains(string(b), AgenticReviewMarker) { + t.Fatal("append strategy should not inject marker") + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":1}`)) + return + } + http.NotFound(w, r) + })) + t.Cleanup(srv.Close) + t.Setenv("GITHUB_API_URL", srv.URL) + + reg := NewRegistry() + _, _, err := reg.Dispatch(context.Background(), "pull_request.post_comment", map[string]any{ + "owner": "o", "repo": "r", "number": "1", "body": "plain", + "comment_strategy": "append", + }) + if err != nil { + t.Fatal(err) + } + if gotGET { + t.Fatal("append should not list comments") + } +} + +func TestGithubPostComment_replaceUpdatesExisting(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "tok") + var methods []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + methods = append(methods, r.Method+" "+r.URL.Path) + switch { + case r.Method == http.MethodGet && r.URL.Path == "/repos/o/r/issues/5/comments": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[{"id":99,"body":"old\n\n` + AgenticReviewMarker + `"}]`)) + case r.Method == http.MethodPatch && r.URL.Path == "/repos/o/r/issues/comments/99": + b, _ := io.ReadAll(r.Body) + if !strings.Contains(string(b), "updated review") { + t.Fatalf("patch body %s", string(b)) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":99,"html_url":"https://example/99"}`)) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(srv.Close) + t.Setenv("GITHUB_API_URL", srv.URL) + + reg := NewRegistry() + out, _, err := reg.Dispatch(context.Background(), "pull_request.post_comment", map[string]any{ + "owner": "o", "repo": "r", "number": "5", "body": "updated review", + }) + if err != nil { + t.Fatal(err) + } + if out["updated"] != true || out["created"] != false { + t.Fatalf("updated/created %#v %#v", out["updated"], out["created"]) + } + if !strings.Contains(strings.Join(methods, ";"), "PATCH") { + t.Fatalf("methods %v", methods) + } +} + +func TestGithubPostComment_replaceCreatesWhenNoMarker(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "tok") + posted := false + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/comments") { + _, _ = w.Write([]byte(`[{"id":1,"body":"unrelated"}]`)) + return + } + if r.Method == http.MethodPost { + posted = true + _, _ = w.Write([]byte(`{"id":2}`)) + return + } + http.NotFound(w, r) + })) + t.Cleanup(srv.Close) + t.Setenv("GITHUB_API_URL", srv.URL) + + reg := NewRegistry() + out, _, err := reg.Dispatch(context.Background(), "pull_request.post_comment", map[string]any{ + "owner": "o", "repo": "r", "number": "2", "body": "first", + }) + if err != nil { + t.Fatal(err) + } + if !posted { + t.Fatal("expected POST") + } + if out["created"] != true { + t.Fatalf("created %#v", out["created"]) + } +} + +func TestGithubPostComment_commentIDPatchesDirectly(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "tok") + patched := false + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPatch && r.URL.Path == "/repos/a/b/issues/comments/777" { + patched = true + _, _ = w.Write([]byte(`{"id":777}`)) + return + } + if r.Method == http.MethodGet { + t.Fatal("comment_id should skip list") + } + http.NotFound(w, r) + })) + t.Cleanup(srv.Close) + t.Setenv("GITHUB_API_URL", srv.URL) + + reg := NewRegistry() + _, _, err := reg.Dispatch(context.Background(), "pull_request.post_comment", map[string]any{ + "owner": "a", "repo": "b", "number": "1", "body": "x", "comment_id": "777", + }) + if err != nil || !patched { + t.Fatalf("err=%v patched=%v", err, patched) + } +} + +func TestGithubPostComment_invalidStrategy(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "tok") + reg := NewRegistry() + _, _, err := reg.Dispatch(context.Background(), "pull_request.post_comment", map[string]any{ + "owner": "o", "repo": "r", "number": "1", "body": "x", + "comment_strategy": "merge", + }) + if err == nil || !strings.Contains(err.Error(), "comment_strategy") { + t.Fatalf("err=%v", err) + } +} + +func TestGithubPostComment_upsertAlias(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "tok") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`[{"id":5,"body":"` + AgenticReviewMarker + `"}]`)) + return + } + if r.Method == http.MethodPatch { + _, _ = w.Write([]byte(`{"id":5}`)) + return + } + http.NotFound(w, r) + })) + t.Cleanup(srv.Close) + t.Setenv("GITHUB_API_URL", srv.URL) + + reg := NewRegistry() + out, _, err := reg.Dispatch(context.Background(), "pull_request.post_comment", map[string]any{ + "owner": "o", "repo": "r", "number": "1", "body": "via upsert", "upsert": true, + }) + if err != nil { + t.Fatal(err) + } + if out["updated"] != true { + t.Fatalf("updated %#v", out["updated"]) + } +} + +func TestEnsureAgenticReviewMarker(t *testing.T) { + got := ensureAgenticReviewMarker("hello") + if !strings.Contains(got, AgenticReviewMarker) { + t.Fatalf("got %q", got) + } + if strings.Count(got, AgenticReviewMarker) != 1 { + t.Fatalf("duplicate marker: %q", got) + } + if ensureAgenticReviewMarker(got) != got { + t.Fatal("idempotent") + } +} + +func TestGithubCommentStrategy_defaultsReplace(t *testing.T) { + s, err := githubCommentStrategy(map[string]any{}) + if err != nil || s != commentStrategyReplace { + t.Fatalf("s=%q err=%v", s, err) + } } func TestGithubPullRequestGet_httpError(t *testing.T) { diff --git a/internal/tools/native/registry.go b/internal/tools/native/registry.go index 7fcae57..9a9f3c9 100644 --- a/internal/tools/native/registry.go +++ b/internal/tools/native/registry.go @@ -53,7 +53,7 @@ func (r *Registry) Dispatch(ctx context.Context, operation string, with map[stri return map[string]any{"pull_request": obj}, meta, nil case "pull_request.post_comment": // Offline: body only (e.g. examples/pr-review-demo). Live: owner, repo, number, body + GITHUB_TOKEN - // creates an issue comment on the PR (same uses string; policy can still gate). + // creates or updates an issue comment on the PR (comment_strategy replace by default). meta.DurationMs = time.Since(start).Milliseconds() owner, repo, num, bodyText, wantLive := githubLivePostCommentContext(with) if !wantLive { @@ -63,7 +63,19 @@ func (r *Registry) Dispatch(ctx context.Context, operation string, with map[stri "body_preview": truncateRunes(body, 240), }, meta, nil } - out, err := githubPullRequestPostComment(ctx, owner, repo, num, bodyText) + strategy, err := githubCommentStrategy(with) + if err != nil { + return nil, meta, err + } + var out map[string]any + if commentID, ok := commentIDFromWith(with); ok { + if err := parseCommentID(commentID); err != nil { + return nil, meta, err + } + out, err = githubPullRequestReplaceCommentByID(ctx, owner, repo, commentID, bodyText) + } else { + out, err = githubPullRequestPostComment(ctx, owner, repo, num, bodyText, strategy) + } if err != nil { return nil, meta, err } diff --git a/test/integration/cli_flow_test.go b/test/integration/cli_flow_test.go index 1f7ab5f..c6f7cda 100644 --- a/test/integration/cli_flow_test.go +++ b/test/integration/cli_flow_test.go @@ -361,6 +361,9 @@ func TestCLI_PrReviewGithubApprovedLiveComment(t *testing.T) { } w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"number":7,"title":"Stub PR","head":{"sha":"abc123"}}`)) + case r.Method == http.MethodGet && r.URL.Path == "/repos/testorg/testrepo/issues/7/comments": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[]`)) case r.Method == http.MethodPost && r.URL.Path == "/repos/testorg/testrepo/issues/7/comments": w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusCreated)