Fix PR lens publication edge cases

Author: cursoragent

.github/scripts/evalops-pr-lens-review.rb

@@ -429,8 +429,15 @@ def dispatch_requested_reviews(owner:, reviewer:, limit:, dry_run:, target_url:,
     summary
   end
 
+  def gh_api_paginated_json(*args, input: nil, token: ENV["GH_TOKEN"])
+    raw = gh_api("--paginate", "--slurp", *args, input: input, token: token)
+    return [] if raw.strip.empty?
+
+    JSON.parse(raw)
+  end
+
   def pr_files_metadata(repo:, pr:)
-    gh_api_json("repos/#{repo}/pulls/#{pr}/files?per_page=100")
+    Array(gh_api_paginated_json("repos/#{repo}/pulls/#{pr}/files?per_page=100")).flat_map { |page| Array(page) }
   end
 
   # Parse a single file's unified-diff patch (as returned by the GitHub files API)
@@ -772,7 +779,7 @@ def pr_metadata(repo:, pr:)
   end
 
   def pr_file_summary(repo:, pr:)
-    files = gh_api_json("repos/#{repo}/pulls/#{pr}/files?per_page=100")
+    files = pr_files_metadata(repo: repo, pr: pr)
     files.map do |file|
       [
         file.fetch("status"),
@@ -1331,8 +1338,8 @@ def marker_comment_ids(repo:, pr:)
     raw.lines.map(&:strip).reject(&:empty?)
   end
 
-  def delete_marker_comments(repo:, pr:)
-    marker_comment_ids(repo: repo, pr: pr).each do |id|
+  def delete_marker_comments(repo:, pr:, ids: nil)
+    Array(ids || marker_comment_ids(repo: repo, pr: pr)).each do |id|
       gh_api("--method", "DELETE", "repos/#{repo}/issues/comments/#{id}")
     end
   end
@@ -1350,22 +1357,22 @@ def marker_review_comment_ids(repo:, pr:)
     raw.lines.map(&:strip).reject(&:empty?)
   end
 
-  def delete_marker_review_comments(repo:, pr:)
-    marker_review_comment_ids(repo: repo, pr: pr).each do |id|
+  def delete_marker_review_comments(repo:, pr:, ids: nil)
+    Array(ids || marker_review_comment_ids(repo: repo, pr: pr)).each do |id|
       gh_api("--method", "DELETE", "repos/#{repo}/pulls/comments/#{id}")
     end
   end
 
   def post_summary_comment(repo:, pr:, body:)
-    gh_api(
+    gh_api_json(
       "--method", "POST", "repos/#{repo}/issues/#{pr}/comments",
       input: JSON.generate(body: body)
     )
   end
 
   def post_inline_comment(repo:, pr:, head_sha:, finding:)
     location = finding.fetch("code_location")
-    gh_api(
+    gh_api_json(
       "--method", "POST", "repos/#{repo}/pulls/#{pr}/comments",
       input: JSON.generate(
         commit_id: head_sha,
@@ -1385,18 +1392,38 @@ def clear_prior_publication(repo:, pr:)
     delete_marker_review_comments(repo: repo, pr: pr)
   end
 
+  def rollback_publication(repo:, pr:, summary_comment_id:, inline_comment_ids:)
+    delete_marker_review_comments(repo: repo, pr: pr, ids: inline_comment_ids.reverse)
+    delete_marker_comments(repo: repo, pr: pr, ids: [summary_comment_id].compact)
+  rescue StandardError => e
+    warn "pr-lens: failed to roll back partial publication for #{repo}##{pr}: #{e.message.lines.first.to_s.strip}"
+  end
+
   # Publish findings as a marker summary issue comment plus inline PR comments
   # anchored to each anchorable finding's code_location. Findings whose line is
   # not in the diff, or that overflow the inline comment cap, are folded into the
-  # summary body. Idempotent: prior marker comments are deleted first.
+  # summary body. Prior marker comments are deleted only after the replacement
+  # publication succeeds so a partial API failure does not leave a misleading
+  # summary or erase the prior review.
   def publish_review(repo:, pr:, head_sha:, inline_findings:, summary_findings:, comment_min_confidence:, target_url:)
-    clear_prior_publication(repo: repo, pr: pr)
-    return if inline_findings.empty? && summary_findings.empty?
+    if inline_findings.empty? && summary_findings.empty?
+      clear_prior_publication(repo: repo, pr: pr)
+      return
+    end
 
     inline_to_publish = inline_findings.first(MAX_FINDINGS_PER_COMMENT)
     overflow_inline_findings = inline_findings.drop(MAX_FINDINGS_PER_COMMENT)
+    prior_summary_ids = marker_comment_ids(repo: repo, pr: pr)
+    prior_inline_ids = marker_review_comment_ids(repo: repo, pr: pr)
+    published_inline_ids = []
+    published_summary_id = nil
 
-    post_summary_comment(
+    inline_to_publish.each do |finding|
+      response = post_inline_comment(repo: repo, pr: pr, head_sha: head_sha, finding: finding)
+      published_inline_ids << response["id"] if response && response["id"]
+    end
+
+    published_summary_id = post_summary_comment(
       repo: repo,
       pr: pr,
       body: review_summary_body(
@@ -1409,10 +1436,17 @@ def publish_review(repo:, pr:, head_sha:, inline_findings:, summary_findings:, c
         target_url: target_url
       )
     )
-
-    inline_to_publish.each do |finding|
-      post_inline_comment(repo: repo, pr: pr, head_sha: head_sha, finding: finding)
-    end
+    published_summary_id = published_summary_id["id"] if published_summary_id
+    delete_marker_comments(repo: repo, pr: pr, ids: prior_summary_ids)
+    delete_marker_review_comments(repo: repo, pr: pr, ids: prior_inline_ids)
+  rescue StandardError
+    rollback_publication(
+      repo: repo,
+      pr: pr,
+      summary_comment_id: published_summary_id,
+      inline_comment_ids: published_inline_ids
+    )
+    raise
   end
 
   def blocking_findings(findings, block_min_confidence:)

test/evalops_pr_lens_review_test.rb

@@ -754,6 +754,22 @@ def test_addable_lines_by_path_indexes_each_file_patch
     assert_empty map.fetch("infra/no_patch.bin")
   end
 
+  def test_pr_files_metadata_paginates_all_pages
+    calls = []
+    pages = [
+      [{ "filename" => "infra/main.tf" }],
+      [{ "filename" => "infra/extra.tf" }]
+    ]
+
+    EvalOpsPrLensReview.stub(:gh_api, ->(*args, **_kwargs) { calls << args; JSON.generate(pages) }) do
+      files = EvalOpsPrLensReview.pr_files_metadata(repo: "evalops/deploy", pr: 1)
+
+      assert_equal ["infra/main.tf", "infra/extra.tf"], files.map { |file| file.fetch("filename") }
+      assert_includes calls.fetch(0), "--paginate"
+      assert_includes calls.fetch(0), "--slurp"
+    end
+  end
+
   def test_finding_inline_anchorable_only_when_line_in_diff
     addable = { "infra/main.tf" => [22, 23].to_set }
     in_diff = finding("Anchorable", 0.9, 1, "infra/main.tf", 22)
@@ -896,11 +912,21 @@ def test_meta_review_green_status_states_coverage_when_only_low_confidence
     end
   end
 
-  def test_publish_review_posts_summary_comment_and_inline_comments_idempotently
+  def test_publish_review_posts_inline_comments_before_summary_and_clears_prior_on_success
     api_calls = []
     fake_api = lambda do |*args, **kwargs|
       api_calls << { args: args, input: kwargs[:input] }
-      ""
+      if args.include?("repos/evalops/deploy/issues/10/comments") && !args.include?("--method")
+        "111\n"
+      elsif args.include?("repos/evalops/deploy/pulls/10/comments") && !args.include?("--method")
+        "222\n333\n"
+      elsif args.include?("repos/evalops/deploy/pulls/10/comments")
+        JSON.generate("id" => 444)
+      elsif args.include?("repos/evalops/deploy/issues/10/comments")
+        JSON.generate("id" => 555)
+      else
+        ""
+      end
     end
 
     inline = [
@@ -910,38 +936,93 @@ def test_publish_review_posts_summary_comment_and_inline_comments_idempotently
       )
     ]
 
-    EvalOpsPrLensReview.stub(:clear_prior_publication, ->(**_kwargs) { api_calls << { clear: true } }) do
-      EvalOpsPrLensReview.stub(:gh_api, fake_api) do
-        EvalOpsPrLensReview.publish_review(
-          repo: "evalops/deploy",
-          pr: 10,
-          head_sha: "abc123",
-          inline_findings: inline,
-          summary_findings: [],
-          comment_min_confidence: 0.55,
-          target_url: "https://github.com/evalops/.github/actions/runs/1"
-        )
-      end
+    EvalOpsPrLensReview.stub(:gh_api, fake_api) do
+      EvalOpsPrLensReview.publish_review(
+        repo: "evalops/deploy",
+        pr: 10,
+        head_sha: "abc123",
+        inline_findings: inline,
+        summary_findings: [],
+        comment_min_confidence: 0.55,
+        target_url: "https://github.com/evalops/.github/actions/runs/1"
+      )
     end
 
-    # Prior publication is cleared before posting (idempotency).
-    assert_equal({ clear: true }, api_calls.fetch(0))
-    summary_call = api_calls.find { |call| Array(call[:args]).include?("repos/evalops/deploy/issues/10/comments") }
+    summary_call = api_calls.find do |call|
+      Array(call[:args]).include?("--method") && Array(call[:args]).include?("repos/evalops/deploy/issues/10/comments")
+    end
     assert summary_call
     summary_payload = JSON.parse(summary_call.fetch(:input))
     assert_includes summary_payload.fetch("body"), EvalOpsPrLensReview::MARKER
 
-    comment_call = api_calls.find { |call| Array(call[:args]).include?("repos/evalops/deploy/pulls/10/comments") }
+    comment_call = api_calls.find do |call|
+      Array(call[:args]).include?("--method") && Array(call[:args]).include?("repos/evalops/deploy/pulls/10/comments")
+    end
     assert comment_call
     comment = JSON.parse(comment_call.fetch(:input))
     assert_equal "abc123", comment.fetch("commit_id")
     assert_equal "infra/main.tf", comment.fetch("path")
     assert_equal 22, comment.fetch("line")
     assert_equal "RIGHT", comment.fetch("side")
     assert_includes comment.fetch("body"), EvalOpsPrLensReview::MARKER
+    assert_operator api_calls.index(comment_call), :<, api_calls.index(summary_call)
+    assert(api_calls.any? { |call| Array(call[:args]).include?("repos/evalops/deploy/issues/comments/111") })
+    assert(api_calls.any? { |call| Array(call[:args]).include?("repos/evalops/deploy/pulls/comments/222") })
+    assert(api_calls.any? { |call| Array(call[:args]).include?("repos/evalops/deploy/pulls/comments/333") })
     refute(api_calls.any? { |call| Array(call[:args]).include?("repos/evalops/deploy/pulls/10/reviews") })
   end
 
+  def test_publish_review_rolls_back_partial_inline_publication_without_deleting_prior_comments
+    api_calls = []
+    inline_posts = 0
+    inline = 2.times.map do |index|
+      finding("Inline #{index}", 0.9, 1, "infra/main.tf", index + 1).merge(
+        "lens" => "iam-blast-radius",
+        "check_id" => "evalops-pr-lens/iam-blast-radius"
+      )
+    end
+
+    fake_api = lambda do |*args, **kwargs|
+      api_calls << { args: args, input: kwargs[:input] }
+      if args.include?("repos/evalops/deploy/issues/10/comments") && !args.include?("--method")
+        "111\n"
+      elsif args.include?("repos/evalops/deploy/pulls/10/comments") && !args.include?("--method")
+        "222\n"
+      elsif args.include?("repos/evalops/deploy/pulls/10/comments")
+        inline_posts += 1
+        raise "inline publish failed" if inline_posts == 2
+
+        JSON.generate("id" => 444)
+      elsif args.include?("repos/evalops/deploy/issues/10/comments")
+        JSON.generate("id" => 555)
+      else
+        ""
+      end
+    end
+
+    error = assert_raises(RuntimeError) do
+      EvalOpsPrLensReview.stub(:gh_api, fake_api) do
+        EvalOpsPrLensReview.publish_review(
+          repo: "evalops/deploy",
+          pr: 10,
+          head_sha: "abc123",
+          inline_findings: inline,
+          summary_findings: [],
+          comment_min_confidence: 0.55,
+          target_url: "https://github.com/evalops/.github/actions/runs/1"
+        )
+      end
+    end
+
+    assert_equal "inline publish failed", error.message
+    refute(api_calls.any? do |call|
+      Array(call[:args]).include?("--method") && Array(call[:args]).include?("repos/evalops/deploy/issues/10/comments")
+    end)
+    assert(api_calls.any? { |call| Array(call[:args]).include?("repos/evalops/deploy/pulls/comments/444") })
+    refute(api_calls.any? { |call| Array(call[:args]).include?("repos/evalops/deploy/issues/comments/111") })
+    refute(api_calls.any? { |call| Array(call[:args]).include?("repos/evalops/deploy/pulls/comments/222") })
+  end
+
   def test_publish_review_moves_inline_overflow_into_summary_comment
     api_calls = []
     fake_api = lambda do |*args, **kwargs|
@@ -969,14 +1050,18 @@ def test_publish_review_moves_inline_overflow_into_summary_comment
       end
     end
 
-    summary_call = api_calls.find { |call| Array(call[:args]).include?("repos/evalops/deploy/issues/10/comments") }
+    summary_call = api_calls.find do |call|
+      Array(call[:args]).include?("--method") && Array(call[:args]).include?("repos/evalops/deploy/issues/10/comments")
+    end
     assert summary_call
     summary_body = JSON.parse(summary_call.fetch(:input)).fetch("body")
     assert_includes summary_body, "#{EvalOpsPrLensReview::MAX_FINDINGS_PER_COMMENT} anchored inline below."
     assert_includes summary_body, "Additional diff findings"
     assert_includes summary_body, "`infra/main.tf:13`"
 
-    inline_posts = api_calls.count { |call| Array(call[:args]).include?("repos/evalops/deploy/pulls/10/comments") }
+    inline_posts = api_calls.count do |call|
+      Array(call[:args]).include?("--method") && Array(call[:args]).include?("repos/evalops/deploy/pulls/10/comments")
+    end
     assert_equal EvalOpsPrLensReview::MAX_FINDINGS_PER_COMMENT, inline_posts
   end