Fix PR lens review publication idempotency

Author: cursoragent

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

@@ -1268,11 +1268,28 @@ def finding_inline_comment_body(finding)
     ].join("\n")
   end
 
-  # Build the review summary body. `inline_findings` are anchored to the diff as
-  # individual comments; `summary_findings` could not be anchored (their line is
-  # not part of the diff) and are listed here with their path:line instead.
-  def review_summary_body(repo:, pr:, inline_findings:, summary_findings:, comment_min_confidence:, target_url:)
-    total = inline_findings.length + summary_findings.length
+  def append_summary_findings(lines, title, findings, omission_label:)
+    return if findings.empty?
+
+    lines << title
+    findings.first(MAX_FINDINGS_PER_COMMENT).each do |finding|
+      location = finding.fetch("code_location")
+      lines << "- **P#{finding.fetch("priority")} #{format("%.2f", finding.fetch("confidence_score"))} #{finding.fetch("lens")}** `#{location.fetch("path")}:#{location.fetch("line")}`: #{finding.fetch("title")}"
+      lines << "  - #{finding.fetch("body")}"
+    end
+    if findings.length > MAX_FINDINGS_PER_COMMENT
+      lines << "- _#{findings.length - MAX_FINDINGS_PER_COMMENT} additional #{omission_label} omitted; inspect the workflow artifact for the full ledger._"
+    end
+    lines << ""
+  end
+
+  # Build the summary comment body. `inline_findings` are anchored to the diff as
+  # individual comments. If there are more anchorable findings than the inline
+  # comment cap allows, `overflow_inline_findings` are listed in the summary with
+  # their locations. `summary_findings` could not be anchored because their line
+  # is not part of the diff and are also listed here with path:line.
+  def review_summary_body(repo:, pr:, inline_findings:, overflow_inline_findings:, summary_findings:, comment_min_confidence:, target_url:)
+    total = inline_findings.length + overflow_inline_findings.length + summary_findings.length
     lines = [
       MARKER,
       "**EvalOps PR lens review**",
@@ -1287,18 +1304,18 @@ def review_summary_body(repo:, pr:, inline_findings:, summary_findings:, comment
       lines << ""
     end
 
-    unless summary_findings.empty?
-      lines << "Findings outside the diff (not inline-anchorable):"
-      summary_findings.first(MAX_FINDINGS_PER_COMMENT).each do |finding|
-        location = finding.fetch("code_location")
-        lines << "- **P#{finding.fetch("priority")} #{format("%.2f", finding.fetch("confidence_score"))} #{finding.fetch("lens")}** `#{location.fetch("path")}:#{location.fetch("line")}`: #{finding.fetch("title")}"
-        lines << "  - #{finding.fetch("body")}"
-      end
-      if summary_findings.length > MAX_FINDINGS_PER_COMMENT
-        lines << "- _#{summary_findings.length - MAX_FINDINGS_PER_COMMENT} additional finding(s) omitted; inspect the workflow artifact for the full ledger._"
-      end
-      lines << ""
-    end
+    append_summary_findings(
+      lines,
+      "Additional diff findings (not posted inline due to the #{MAX_FINDINGS_PER_COMMENT}-comment cap):",
+      overflow_inline_findings,
+      omission_label: "diff finding(s)"
+    )
+    append_summary_findings(
+      lines,
+      "Findings outside the diff (not inline-anchorable):",
+      summary_findings,
+      omission_label: "finding(s)"
+    )
 
     lines << "_Repo: #{repo} PR: ##{pr}_"
     lines.join("\n")
@@ -1339,6 +1356,27 @@ def delete_marker_review_comments(repo:, pr:)
     end
   end
 
+  def post_summary_comment(repo:, pr:, body:)
+    gh_api(
+      "--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(
+      "--method", "POST", "repos/#{repo}/pulls/#{pr}/comments",
+      input: JSON.generate(
+        commit_id: head_sha,
+        path: location.fetch("path"),
+        line: Integer(location.fetch("line")),
+        side: "RIGHT",
+        body: finding_inline_comment_body(finding)
+      )
+    )
+  end
+
   # Remove the bot's prior published artifacts for this PR (marker issue-comment
   # left by the legacy code path, and prior marker inline review comments) so the
   # publication is idempotent across re-runs on the same head.
@@ -1347,42 +1385,34 @@ def clear_prior_publication(repo:, pr:)
     delete_marker_review_comments(repo: repo, pr: pr)
   end
 
-  # Publish findings as a single PR review: a summary body plus inline comments
+  # 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 are folded into the summary body. Idempotent: prior marker
-  # comments are deleted first.
+  # not in the diff, or that overflow the inline comment cap, are folded into the
+  # summary body. Idempotent: prior marker comments are deleted first.
   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?
 
-    comments = inline_findings.first(MAX_FINDINGS_PER_COMMENT).map do |finding|
-      location = finding.fetch("code_location")
-      {
-        path: location.fetch("path"),
-        line: Integer(location.fetch("line")),
-        side: "RIGHT",
-        body: finding_inline_comment_body(finding)
-      }
-    end
+    inline_to_publish = inline_findings.first(MAX_FINDINGS_PER_COMMENT)
+    overflow_inline_findings = inline_findings.drop(MAX_FINDINGS_PER_COMMENT)
 
-    payload = {
-      commit_id: head_sha,
-      event: "COMMENT",
+    post_summary_comment(
+      repo: repo,
+      pr: pr,
       body: review_summary_body(
         repo: repo,
         pr: pr,
-        inline_findings: inline_findings,
+        inline_findings: inline_to_publish,
+        overflow_inline_findings: overflow_inline_findings,
         summary_findings: summary_findings,
         comment_min_confidence: comment_min_confidence,
         target_url: target_url
-      ),
-      comments: comments
-    }
-
-    gh_api(
-      "--method", "POST", "repos/#{repo}/pulls/#{pr}/reviews",
-      input: JSON.generate(payload)
+      )
     )
+
+    inline_to_publish.each do |finding|
+      post_inline_comment(repo: repo, pr: pr, head_sha: head_sha, finding: finding)
+    end
   end
 
   def blocking_findings(findings, block_min_confidence:)
@@ -1649,6 +1679,7 @@ def markdown_meta_report(result)
     # single knob and now maps to the *block* threshold. The comment threshold
     # defaults lower so medium-confidence findings are still shown.
     legacy_block = ENV["PR_LENS_MIN_CONFIDENCE"]
+    legacy_block = nil if legacy_block.to_s.empty?
     options = {
       comment_min_confidence: Float(
         ENV.fetch("PR_LENS_COMMENT_MIN_CONFIDENCE", EvalOpsPrLensReview::DEFAULT_COMMENT_MIN_CONFIDENCE)

test/evalops_pr_lens_review_test.rb

@@ -2,6 +2,7 @@
 
 require "json"
 require "minitest/autorun"
+require "open3"
 require "set"
 require "stringio"
 require "tmpdir"
@@ -348,6 +349,7 @@ def test_review_summary_body_lists_inline_and_off_diff_findings
       repo: "evalops/deploy",
       pr: 10,
       inline_findings: inline,
+      overflow_inline_findings: [],
       summary_findings: summary,
       comment_min_confidence: 0.55,
       target_url: "https://github.com/evalops/.github/actions/runs/1"
@@ -360,6 +362,41 @@ def test_review_summary_body_lists_inline_and_off_diff_findings
     assert_includes body, "`infra/old.tf:9`"
   end
 
+  def test_review_summary_body_lists_inline_overflow_in_summary
+    inline = Array.new(EvalOpsPrLensReview::MAX_FINDINGS_PER_COMMENT) do |index|
+      finding("Inline #{index}", 0.80, 1, "infra/main.tf", index + 1).merge(
+        "repo" => "evalops/deploy",
+        "pr" => 10,
+        "lens" => "iam-blast-radius",
+        "head_sha" => "abc123",
+        "check_id" => "evalops-pr-lens/iam-blast-radius"
+      )
+    end
+    overflow = [
+      finding("Overflow inline", 0.79, 2, "infra/main.tf", 99).merge(
+        "repo" => "evalops/deploy",
+        "pr" => 10,
+        "lens" => "iam-blast-radius",
+        "head_sha" => "abc123",
+        "check_id" => "evalops-pr-lens/iam-blast-radius"
+      )
+    ]
+
+    body = EvalOpsPrLensReview.review_summary_body(
+      repo: "evalops/deploy",
+      pr: 10,
+      inline_findings: inline,
+      overflow_inline_findings: overflow,
+      summary_findings: [],
+      comment_min_confidence: 0.55,
+      target_url: "https://github.com/evalops/.github/actions/runs/1"
+    )
+
+    assert_includes body, "#{EvalOpsPrLensReview::MAX_FINDINGS_PER_COMMENT} anchored inline below."
+    assert_includes body, "Additional diff findings"
+    assert_includes body, "`infra/main.tf:99`"
+  end
+
   def test_finding_inline_comment_body_carries_marker_and_check
     body = EvalOpsPrLensReview.finding_inline_comment_body(
       finding("Unsafe IAM expansion", 0.94, 1, "infra/main.tf", 22).merge(
@@ -859,7 +896,7 @@ def test_meta_review_green_status_states_coverage_when_only_low_confidence
     end
   end
 
-  def test_publish_review_posts_pr_review_and_is_idempotent
+  def test_publish_review_posts_summary_comment_and_inline_comments_idempotently
     api_calls = []
     fake_api = lambda do |*args, **kwargs|
       api_calls << { args: args, input: kwargs[:input] }
@@ -889,17 +926,58 @@ def test_publish_review_posts_pr_review_and_is_idempotent
 
     # Prior publication is cleared before posting (idempotency).
     assert_equal({ clear: true }, api_calls.fetch(0))
-    review_call = api_calls.find { |call| Array(call[:args]).include?("repos/evalops/deploy/pulls/10/reviews") }
-    assert review_call
-    payload = JSON.parse(review_call.fetch(:input))
-    assert_equal "COMMENT", payload.fetch("event")
-    assert_equal "abc123", payload.fetch("commit_id")
-    assert_equal 1, payload.fetch("comments").length
-    comment = payload.fetch("comments").fetch(0)
+    summary_call = api_calls.find { |call| Array(call[:args]).include?("repos/evalops/deploy/issues/10/comments") }
+    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") }
+    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
+    refute(api_calls.any? { |call| Array(call[:args]).include?("repos/evalops/deploy/pulls/10/reviews") })
+  end
+
+  def test_publish_review_moves_inline_overflow_into_summary_comment
+    api_calls = []
+    fake_api = lambda do |*args, **kwargs|
+      api_calls << { args: args, input: kwargs[:input] }
+      ""
+    end
+    inline = Array.new(EvalOpsPrLensReview::MAX_FINDINGS_PER_COMMENT + 1) 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
+
+    EvalOpsPrLensReview.stub(:clear_prior_publication, ->(**_kwargs) {}) 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
+
+    summary_call = api_calls.find { |call| Array(call[:args]).include?("repos/evalops/deploy/issues/10/comments") }
+    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") }
+    assert_equal EvalOpsPrLensReview::MAX_FINDINGS_PER_COMMENT, inline_posts
   end
 
   def test_publish_review_clears_prior_then_skips_post_when_no_findings
@@ -956,6 +1034,30 @@ def test_meta_review_back_compat_min_confidence_maps_to_block_threshold
     assert_equal "failure", EvalOpsPrLensReview.meta_state(findings, block_min_confidence: 0.65)
   end
 
+  def test_meta_review_cli_ignores_empty_legacy_block_env
+    Dir.mktmpdir do |dir|
+      output = File.join(dir, "meta-review.json")
+      script = File.expand_path("../.github/scripts/evalops-pr-lens-review.rb", __dir__)
+      stdout, stderr, status = Open3.capture3(
+        {
+          "PR_LENS_MIN_CONFIDENCE" => "",
+          "PR_LENS_BLOCK_MIN_CONFIDENCE" => nil
+        },
+        "ruby",
+        script,
+        "meta-review",
+        "--artifact-root",
+        dir,
+        "--output",
+        output
+      )
+
+      assert status.success?, "#{stdout}\n#{stderr}"
+      result = JSON.parse(File.read(output))
+      assert_equal EvalOpsPrLensReview::DEFAULT_BLOCK_MIN_CONFIDENCE, result.fetch("block_min_confidence")
+    end
+  end
+
   private
 
   def capture_warnings