From b4541291936016ddc53216e98e1b69faf1a0fc0f Mon Sep 17 00:00:00 2001 From: Jonathan Haas Date: Tue, 9 Jun 2026 12:46:07 -0700 Subject: [PATCH 1/5] Fix PR lens review: inline comments + split confidence thresholds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "EvalOps PR lens review" system published 0 findings across the last 12 scheduled runs (~70 lens reviews) yet reported success every time. Three bugs combined to make it silently green: 1. One blunt threshold (DEFAULT_MIN_CONFIDENCE = 0.82) decided both what to show a human and what fails the status, so real medium-confidence findings (0.6-0.8) were silently discarded. 2. Findings posted as one issue comment at the bottom of the PR with `path:line` as plain text, despite every finding carrying an exact code_location. 3. normalize_finding silently returned nil for malformed findings. Changes: - Publish as a single PR review (POST pulls/{pr}/reviews, event COMMENT) with inline comments anchored to each finding's code_location. meta-review parses each file's patch hunks to compute the set of addable right-side line numbers; findings whose path:line is in that set are inlined, the rest are folded into the review summary body (avoids GitHub's 422 on off-diff lines). - Split the threshold in two: comment_min_confidence (default 0.55, env PR_LENS_COMMENT_MIN_CONFIDENCE) for surfacing; block_min_confidence (default 0.80, env PR_LENS_BLOCK_MIN_CONFIDENCE) for failing the status, and only on P0/P1. PR_LENS_MIN_CONFIDENCE / --min-confidence kept as a back-compat alias mapping to the block threshold. - Honest green status: "N lenses · 0 findings >= 0.55" instead of implying nothing was found. - normalize_finding now raises DroppedFinding; the caller counts and warns per dropped finding and records dropped_findings in the ledger. - Idempotency preserved: prior marker issue-comment and prior marker inline review comments are deleted before posting, so re-running on the same head replaces rather than duplicates. - Workflow gains comment_min_confidence / block_min_confidence inputs and keeps min_confidence for back-compat. Test Plan: - ruby -Itest -e 'ARGV.each { |path| require "./#{path}" }' test/*_test.rb => 125 runs, 1016 assertions, 0 failures, 0 errors, 0 skips - New coverage: diff-hunk line parsing, inline-vs-summary split, the two thresholds, idempotent replacement, dropped-finding logging. Rollback: revert this commit; behavior returns to the single-threshold issue-comment publisher. No state migration involved. Signal: 12 consecutive green scheduled runs with 0 published findings (gh run list evalops-pr-lens-review.yml, 2026-06-08..09). Co-Authored-By: Claude Fable 5 --- .github/scripts/evalops-pr-lens-review.rb | 309 ++++++++++++--- .github/workflows/evalops-pr-lens-review.yml | 35 +- test/evalops_pr_lens_review_test.rb | 394 ++++++++++++++++++- 3 files changed, 662 insertions(+), 76 deletions(-) diff --git a/.github/scripts/evalops-pr-lens-review.rb b/.github/scripts/evalops-pr-lens-review.rb index e7aa6b2..cf7f4a2 100644 --- a/.github/scripts/evalops-pr-lens-review.rb +++ b/.github/scripts/evalops-pr-lens-review.rb @@ -9,6 +9,7 @@ require "openssl" require "open3" require "optparse" +require "set" require "time" require "uri" require "yaml" @@ -123,7 +124,15 @@ module EvalOpsPrLensReview MARKER = "" REVIEW_REQUESTED_DISPATCH_EVENT = "evalopsbot-review-requested" REVIEW_REQUESTED_DISPATCH_SOURCE = "evalopsbot-review-request-dispatch" - DEFAULT_MIN_CONFIDENCE = 0.82 + # Findings at or above this confidence are surfaced to humans (inline or in the + # review summary). Lower than the historical 0.82 so real medium-confidence + # findings stop getting silently discarded. + DEFAULT_COMMENT_MIN_CONFIDENCE = 0.55 + # Only P0/P1 findings at or above this confidence flip the meta-review status to + # failure. Showing a finding and blocking on it are now separate decisions. + DEFAULT_BLOCK_MIN_CONFIDENCE = 0.80 + # Back-compat: the single legacy knob now maps to the block threshold. + DEFAULT_MIN_CONFIDENCE = DEFAULT_BLOCK_MIN_CONFIDENCE DEFAULT_MODEL = "claude-opus-4-7" DEFAULT_PROVIDER = "anthropic" DEFAULT_MAX_DIFF_BYTES = 180_000 @@ -424,6 +433,60 @@ def pr_files_metadata(repo:, pr:) gh_api_json("repos/#{repo}/pulls/#{pr}/files?per_page=100") end + # Parse a single file's unified-diff patch (as returned by the GitHub files API) + # into the set of right-side (head) line numbers that an inline review comment + # may anchor to. GitHub only accepts inline comments on added or context lines + # that are part of the diff; commenting elsewhere returns HTTP 422. + def addable_lines_from_patch(patch) + lines = Set.new + right_line = nil + patch.to_s.each_line do |raw| + line = raw.chomp + if (match = line.match(/\A@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/)) + right_line = Integer(match[1]) + next + end + next if right_line.nil? + + case line[0] + when "+" + lines << right_line + right_line += 1 + when " ", "" + # Context line counts on the right side but is not itself an addition. + right_line += 1 + when "-" + # Deletion only advances the left side. + when "\\" + # "\ No newline at end of file" marker; does not advance either side. + else + right_line += 1 + end + end + lines + end + + # Map of head-side path => Set of addable line numbers for the PR diff. Used to + # decide which findings can be posted as inline review comments versus folded + # into the review summary body. + def addable_lines_by_path(repo:, pr:, files: nil) + files ||= pr_files_metadata(repo: repo, pr: pr) + Array(files).each_with_object({}) do |file, map| + path = file["filename"].to_s + next if path.empty? + + map[path] = addable_lines_from_patch(file["patch"]) + end + end + + def finding_inline_anchorable?(finding, addable_by_path) + location = finding.fetch("code_location") + addable = addable_by_path[location.fetch("path")] + return false if addable.nil? + + addable.include?(Integer(location.fetch("line"))) + end + def discover_open_prs(repos:, pr_filter: nil, force_lenses: nil) repos.flat_map do |repo| normalized_repo = normalize_repo(repo) @@ -963,12 +1026,36 @@ def normalize_finding(finding) "line" => Integer(line) } } - rescue ArgumentError, KeyError, TypeError - nil + rescue ArgumentError, KeyError, TypeError => e + raise DroppedFinding, e.message + end + + # Raised by normalize_finding when a malformed finding cannot be normalized, + # so the caller can count and log the drop instead of silently swallowing it. + class DroppedFinding < StandardError; end + + def normalize_findings_with_drops(raw_findings, repo:, pr:, lens:) + dropped = 0 + findings = Array(raw_findings).each_with_index.filter_map do |finding, index| + normalize_finding(finding) + rescue DroppedFinding => e + dropped += 1 + warn( + "pr-lens: dropped malformed finding ##{index} from #{repo}##{pr} #{lens}: " \ + "#{e.message.lines.first.to_s.strip}" + ) + nil + end + [findings, dropped] end def normalize_lens_review(raw_review, repo:, pr:, lens:, head_sha:) - findings = Array(raw_review["findings"]).map { |finding| normalize_finding(finding) }.compact + findings, dropped = normalize_findings_with_drops( + raw_review["findings"], + repo: repo, + pr: pr, + lens: lens + ) top_confidence = findings.map { |finding| finding.fetch("confidence_score") }.max || 0.0 confidence = coerce_number( raw_review.fetch("confidence_score", top_confidence), @@ -987,6 +1074,7 @@ def normalize_lens_review(raw_review, repo:, pr:, lens:, head_sha:) "generated_at" => Time.now.utc.iso8601, "summary" => raw_review.fetch("summary", "").to_s.strip, "confidence_score" => confidence, + "dropped_findings" => dropped, "findings" => findings } end @@ -1169,27 +1257,46 @@ def run_url "#{server}/#{repo}/actions/runs/#{run_id}" end - def comment_body(repo:, pr:, findings:, min_confidence:, target_url:) + def finding_inline_comment_body(finding) + [ + MARKER, + "**P#{finding.fetch("priority")} · #{format("%.2f", finding.fetch("confidence_score"))} · #{finding.fetch("lens")}**: #{finding.fetch("title")}", + "", + finding.fetch("body"), + "", + "_Check: `#{finding.fetch("check_id")}`_" + ].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 lines = [ MARKER, "**EvalOps PR lens review**", "", - "High-confidence findings only. Threshold: #{format("%.2f", min_confidence)}.", + "#{total} finding#{total == 1 ? "" : "s"} ≥ #{format("%.2f", comment_min_confidence)} confidence.", "Run: #{target_url || "unavailable"}", "" ] - findings.first(MAX_FINDINGS_PER_COMMENT).each_with_index do |finding, index| - location = finding.fetch("code_location") - lines << "#{index + 1}. **P#{finding.fetch("priority")} #{format("%.2f", finding.fetch("confidence_score"))} #{finding.fetch("lens")}**: #{finding.fetch("title")}" - lines << " - Location: `#{location.fetch("path")}:#{location.fetch("line")}`" - lines << " - Check: `#{finding.fetch("check_id")}`" - lines << " - #{finding.fetch("body")}" + unless inline_findings.empty? + lines << "#{inline_findings.length} anchored inline below." lines << "" end - if findings.length > MAX_FINDINGS_PER_COMMENT - lines << "_#{findings.length - MAX_FINDINGS_PER_COMMENT} additional high-confidence finding(s) were omitted from the comment; inspect the workflow artifact for the full ledger._" + 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 @@ -1207,54 +1314,111 @@ def marker_comment_ids(repo:, pr:) raw.lines.map(&:strip).reject(&:empty?) end - def upsert_comment(repo:, pr:, body:) - ids = marker_comment_ids(repo: repo, pr: pr) - if ids.empty? - gh_api( - "--method", "POST", "repos/#{repo}/issues/#{pr}/comments", - input: JSON.generate({ body: body }) - ) - else - first, *stale = ids - gh_api( - "--method", "PATCH", "repos/#{repo}/issues/comments/#{first}", - input: JSON.generate({ body: body }) - ) - stale.each { |id| gh_api("--method", "DELETE", "repos/#{repo}/issues/comments/#{id}") } - end - end - def delete_marker_comments(repo:, pr:) marker_comment_ids(repo: repo, pr: pr).each do |id| gh_api("--method", "DELETE", "repos/#{repo}/issues/comments/#{id}") end end - def meta_state(findings, coverage_incomplete: false) + # Prior bot inline review comments carrying the marker, on this PR. Deleted + # before a fresh review is posted so re-running on the same head replaces + # rather than duplicates. + def marker_review_comment_ids(repo:, pr:) + raw = gh_api( + "--paginate", + "repos/#{repo}/pulls/#{pr}/comments", + "--jq", + ".[] | select(.body | contains(\"#{MARKER}\")) | .id" + ) + 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| + gh_api("--method", "DELETE", "repos/#{repo}/pulls/comments/#{id}") + end + 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. + def clear_prior_publication(repo:, pr:) + delete_marker_comments(repo: repo, pr: pr) + delete_marker_review_comments(repo: repo, pr: pr) + end + + # Publish findings as a single PR review: a summary body plus inline 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. + 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 + + payload = { + commit_id: head_sha, + event: "COMMENT", + body: review_summary_body( + repo: repo, + pr: pr, + inline_findings: 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) + ) + end + + def blocking_findings(findings, block_min_confidence:) + findings.select do |finding| + finding.fetch("priority") <= 1 && + finding.fetch("confidence_score") >= block_min_confidence + end + end + + def meta_state(findings, block_min_confidence:, coverage_incomplete: false) return "error" if coverage_incomplete - findings.any? { |finding| finding.fetch("priority") <= 1 } ? "failure" : "success" + blocking_findings(findings, block_min_confidence: block_min_confidence).any? ? "failure" : "success" end - def meta_description(findings, missing_count: 0, skipped_count: 0) + def meta_description(findings, lens_count:, comment_min_confidence:, missing_count: 0, skipped_count: 0) + confidence_label = format("%.2f", comment_min_confidence) + lens_label = "#{lens_count} lens#{lens_count == 1 ? "" : "es"}" + if missing_count.positive? "PR lens coverage incomplete: #{missing_count} missing" - elsif findings.empty? && skipped_count.positive? - "No findings; #{skipped_count} lens review#{skipped_count == 1 ? "" : "s"} skipped" elsif findings.empty? - "No high-confidence PR lens findings" + base = "#{lens_label} · 0 findings ≥ #{confidence_label}" + skipped_count.positive? ? "#{base} (#{skipped_count} skipped)" : base else - "#{findings.length} high-confidence finding#{findings.length == 1 ? "" : "s"}" + "#{lens_label} · #{findings.length} finding#{findings.length == 1 ? "" : "s"} ≥ #{confidence_label}" end end - def meta_review(artifact_root:, min_confidence:, output:) + def meta_review(artifact_root:, comment_min_confidence:, block_min_confidence:, output:) reviews = read_lens_reviews(artifact_root) expected_reviews = read_expected_reviews(artifact_root) reviews_by_key = reviews.each_with_object({}) do |review, hash| hash[[review.fetch("repo"), Integer(review.fetch("pr")), review.fetch("lens"), review.fetch("head_sha")]] = review end - ranked = dedupe_and_rank(high_confidence_findings(reviews, min_confidence: min_confidence)) + ranked = dedupe_and_rank(high_confidence_findings(reviews, min_confidence: comment_min_confidence)) grouped = grouped_by_pr(ranked) target_url = run_url coverage_by_pr = {} @@ -1285,22 +1449,38 @@ def meta_review(artifact_root:, min_confidence:, output:) "lenses" => reviews.select { |review| review.fetch("repo") == repo && review.fetch("pr") == pr && review.fetch("head_sha") == head_sha }.map { |review| review.fetch("lens") }.sort } ) + if findings.empty? - delete_marker_comments(repo: repo, pr: pr) + clear_prior_publication(repo: repo, pr: pr) else - upsert_comment( + addable_by_path = addable_lines_by_path(repo: repo, pr: pr) + inline_findings, summary_findings = findings.partition do |finding| + finding_inline_anchorable?(finding, addable_by_path) + end + publish_review( repo: repo, pr: pr, - body: comment_body(repo: repo, pr: pr, findings: findings, min_confidence: min_confidence, target_url: target_url) + head_sha: head_sha, + inline_findings: inline_findings, + summary_findings: summary_findings, + comment_min_confidence: comment_min_confidence, + target_url: target_url ) end + post_status( repo: repo, sha: head_sha, context: meta_context, - state: meta_state(findings, coverage_incomplete: coverage.fetch("missing").positive?), + state: meta_state( + findings, + block_min_confidence: block_min_confidence, + coverage_incomplete: coverage.fetch("missing").positive? + ), description: meta_description( findings, + lens_count: Array(coverage.fetch("lenses", [])).length, + comment_min_confidence: comment_min_confidence, missing_count: coverage.fetch("missing"), skipped_count: coverage.fetch("skipped") ), @@ -1311,13 +1491,16 @@ def meta_review(artifact_root:, min_confidence:, output:) result = { "schema_version" => 1, "generated_at" => Time.now.utc.iso8601, - "min_confidence" => min_confidence, + "comment_min_confidence" => comment_min_confidence, + "block_min_confidence" => block_min_confidence, "reviews" => reviews.length, "expected_reviews" => expected_reviews.length, + "dropped_findings" => reviews.sum { |review| Integer(review.fetch("dropped_findings", 0)) }, "coverage" => coverage_by_pr.map do |(repo, pr, head_sha), coverage| coverage.merge("repo" => repo, "pr" => pr, "head_sha" => head_sha) end, "published_findings" => ranked, + "blocking_findings" => blocking_findings(ranked, block_min_confidence: block_min_confidence), "run_url" => target_url } File.write(output, JSON.pretty_generate(result)) @@ -1325,12 +1508,16 @@ def meta_review(artifact_root:, min_confidence:, output:) end def markdown_meta_report(result) + comment_threshold = format("%.2f", result.fetch("comment_min_confidence", DEFAULT_COMMENT_MIN_CONFIDENCE)) + block_threshold = format("%.2f", result.fetch("block_min_confidence", DEFAULT_BLOCK_MIN_CONFIDENCE)) lines = [ "## EvalOps PR Lens Review", "", "- Reviews: #{result.fetch("reviews")}", "- Expected reviews: #{result.fetch("expected_reviews")}", - "- Published findings: #{result.fetch("published_findings").length}", + "- Comment threshold: #{comment_threshold} · Block threshold: #{block_threshold}", + "- Published findings: #{result.fetch("published_findings").length} (#{result.fetch("blocking_findings", []).length} blocking)", + "- Dropped malformed findings: #{result.fetch("dropped_findings", 0)}", "- Run: #{result.fetch("run_url") || "unavailable"}", "", "### Coverage" @@ -1340,7 +1527,7 @@ def markdown_meta_report(result) end if result.fetch("published_findings", []).empty? lines << "" - lines << "No high-confidence findings cleared the publication threshold." + lines << "No findings cleared the comment threshold (#{comment_threshold})." else lines << "" lines << "### Findings" @@ -1458,22 +1645,40 @@ def markdown_meta_report(result) review = JSON.parse(File.read(options.fetch(:review_json))) puts EvalOpsPrLensReview.lens_status_description(review) when "meta-review" + # Back-compat: PR_LENS_MIN_CONFIDENCE (and --min-confidence) is the legacy + # 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"] options = { - min_confidence: Float(ENV.fetch("PR_LENS_MIN_CONFIDENCE", EvalOpsPrLensReview::DEFAULT_MIN_CONFIDENCE)), + comment_min_confidence: Float( + ENV.fetch("PR_LENS_COMMENT_MIN_CONFIDENCE", EvalOpsPrLensReview::DEFAULT_COMMENT_MIN_CONFIDENCE) + ), + block_min_confidence: Float( + ENV.fetch( + "PR_LENS_BLOCK_MIN_CONFIDENCE", + legacy_block || EvalOpsPrLensReview::DEFAULT_BLOCK_MIN_CONFIDENCE + ) + ), output: "meta-review.json" } + markdown_output = nil OptionParser.new do |parser| parser.on("--artifact-root PATH") { |value| options[:artifact_root] = value } - parser.on("--min-confidence NUMBER", Float) { |value| options[:min_confidence] = value } + parser.on("--comment-min-confidence NUMBER", Float) { |value| options[:comment_min_confidence] = value } + parser.on("--block-min-confidence NUMBER", Float) { |value| options[:block_min_confidence] = value } + # Legacy alias: sets the block threshold. + parser.on("--min-confidence NUMBER", Float) { |value| options[:block_min_confidence] = value } parser.on("--output PATH") { |value| options[:output] = value } - parser.on("--markdown-output PATH") { |value| options[:markdown_output] = value } + parser.on("--markdown-output PATH") { |value| markdown_output = value } end.parse! raise OptionParser::MissingArgument, "artifact-root" if options[:artifact_root].to_s.empty? - markdown_output = options.delete(:markdown_output) result = EvalOpsPrLensReview.meta_review(**options) File.write(markdown_output, EvalOpsPrLensReview.markdown_meta_report(result)) if markdown_output - puts "Published #{result.fetch("published_findings").length} high-confidence finding(s)." + puts( + "Published #{result.fetch("published_findings").length} finding(s) " \ + "(#{result.fetch("blocking_findings").length} blocking)." + ) when "dispatch-review-requests" options = { owner: "evalops", diff --git a/.github/workflows/evalops-pr-lens-review.yml b/.github/workflows/evalops-pr-lens-review.yml index e36e6fd..fa9287c 100644 --- a/.github/workflows/evalops-pr-lens-review.yml +++ b/.github/workflows/evalops-pr-lens-review.yml @@ -16,9 +16,17 @@ on: required: false default: "" min_confidence: - description: "Minimum confidence for PR comment publication" + description: "Deprecated alias for block_min_confidence (kept for back-compat)" required: false - default: "0.82" + default: "" + comment_min_confidence: + description: "Minimum confidence for surfacing a finding (inline or summary)" + required: false + default: "0.55" + block_min_confidence: + description: "Minimum confidence for a P0/P1 finding to fail the meta-review status" + required: false + default: "0.80" model: description: "Model for lens reviewers" required: false @@ -309,7 +317,10 @@ jobs: timeout-minutes: 10 env: GH_TOKEN: ${{ secrets.EVALOPS_PR_LENS_TOKEN || secrets.EVALOPS_REVIEW_GUARD_TOKEN }} - PR_LENS_MIN_CONFIDENCE: ${{ github.event_name == 'repository_dispatch' && github.event.client_payload.min_confidence || inputs.min_confidence || '0.82' }} + # Back-compat alias: PR_LENS_MIN_CONFIDENCE maps to the block threshold. + PR_LENS_MIN_CONFIDENCE: ${{ github.event_name == 'repository_dispatch' && github.event.client_payload.min_confidence || inputs.min_confidence || '' }} + PR_LENS_COMMENT_MIN_CONFIDENCE: ${{ github.event_name == 'repository_dispatch' && github.event.client_payload.comment_min_confidence || inputs.comment_min_confidence || '0.55' }} + PR_LENS_BLOCK_MIN_CONFIDENCE: ${{ github.event_name == 'repository_dispatch' && github.event.client_payload.block_min_confidence || inputs.block_min_confidence || '0.80' }} PR_LENS_APP_REPOSITORIES: ".github,platform,deploy,maestro-internal,maestro,ensemble,diffscope,chat,cerebro" RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} steps: @@ -351,15 +362,23 @@ jobs: exit 2 fi - - name: Publish high-confidence findings + - name: Publish findings as inline PR review shell: bash run: | set -euo pipefail - ruby .github/scripts/evalops-pr-lens-review.rb meta-review \ - --artifact-root artifacts \ - --min-confidence "${PR_LENS_MIN_CONFIDENCE}" \ - --output meta-review.json \ + args=( + meta-review + --artifact-root artifacts + --comment-min-confidence "${PR_LENS_COMMENT_MIN_CONFIDENCE}" + --block-min-confidence "${PR_LENS_BLOCK_MIN_CONFIDENCE}" + --output meta-review.json --markdown-output "${GITHUB_STEP_SUMMARY}" + ) + # Back-compat: a non-empty legacy min_confidence overrides the block threshold. + if [ -n "${PR_LENS_MIN_CONFIDENCE}" ]; then + args+=(--min-confidence "${PR_LENS_MIN_CONFIDENCE}") + fi + ruby .github/scripts/evalops-pr-lens-review.rb "${args[@]}" - name: Upload meta review ledger uses: actions/upload-artifact@v4 diff --git a/test/evalops_pr_lens_review_test.rb b/test/evalops_pr_lens_review_test.rb index 11ae040..695afa0 100644 --- a/test/evalops_pr_lens_review_test.rb +++ b/test/evalops_pr_lens_review_test.rb @@ -2,6 +2,8 @@ require "json" require "minitest/autorun" +require "set" +require "stringio" require "tmpdir" require_relative "../.github/scripts/evalops-pr-lens-review" @@ -154,17 +156,52 @@ def test_normalize_lens_review_drops_invalid_findings ] } - review = EvalOpsPrLensReview.normalize_lens_review( - raw, - repo: "evalops/platform", - pr: 2023, - lens: "migration-safety", - head_sha: "abc123" - ) + review = nil + warnings = capture_warnings do + review = EvalOpsPrLensReview.normalize_lens_review( + raw, + repo: "evalops/platform", + pr: 2023, + lens: "migration-safety", + head_sha: "abc123" + ) + end + assert_equal 1, warnings.grep(/dropped malformed finding/).length assert_equal "evalops-pr-lens/migration-safety", review.fetch("check_id") assert_equal 1, review.fetch("findings").length assert_equal "db/migrations/001.sql", review.fetch("findings").fetch(0).dig("code_location", "path") + assert_equal 1, review.fetch("dropped_findings") + end + + def test_normalize_findings_with_drops_counts_and_warns_malformed_findings + raw_findings = [ + { + "title" => "Valid", + "body" => "A real defect.", + "confidence_score" => 0.7, + "priority" => 2, + "code_location" => { "path" => "a.rb", "line" => 5 } + }, + { "title" => "missing body" }, + { "body" => "missing title" } + ] + + findings = nil + dropped = nil + warnings = capture_warnings do + findings, dropped = EvalOpsPrLensReview.normalize_findings_with_drops( + raw_findings, + repo: "evalops/platform", + pr: 7, + lens: "migration-safety" + ) + end + + assert_equal 1, findings.length + assert_equal 2, dropped + assert_equal 2, warnings.grep(/dropped malformed finding/).length + assert(warnings.any? { |line| line.include?("evalops/platform#7 migration-safety") }) end def test_high_confidence_findings_filters_and_ranks_by_confidence @@ -287,8 +324,8 @@ def test_lens_routing_config_overrides_default_review_options end end - def test_comment_body_contains_only_ranked_findings - findings = [ + def test_review_summary_body_lists_inline_and_off_diff_findings + inline = [ finding("Unsafe IAM expansion", 0.94, 1, "infra/main.tf", 22).merge( "repo" => "evalops/deploy", "pr" => 10, @@ -297,18 +334,42 @@ def test_comment_body_contains_only_ranked_findings "check_id" => "evalops-pr-lens/iam-blast-radius" ) ] + summary = [ + finding("Drift outside the diff", 0.61, 2, "infra/old.tf", 9).merge( + "repo" => "evalops/deploy", + "pr" => 10, + "lens" => "argo-manifest-skew", + "head_sha" => "abc123", + "check_id" => "evalops-pr-lens/argo-manifest-skew" + ) + ] - body = EvalOpsPrLensReview.comment_body( + body = EvalOpsPrLensReview.review_summary_body( repo: "evalops/deploy", pr: 10, - findings: findings, - min_confidence: 0.82, + inline_findings: inline, + summary_findings: summary, + comment_min_confidence: 0.55, target_url: "https://github.com/evalops/.github/actions/runs/1" ) assert_includes body, EvalOpsPrLensReview::MARKER - assert_includes body, "High-confidence findings only" - assert_includes body, "`infra/main.tf:22`" + assert_includes body, "2 findings ≥ 0.55 confidence." + assert_includes body, "1 anchored inline below." + assert_includes body, "Findings outside the diff" + assert_includes body, "`infra/old.tf:9`" + 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( + "lens" => "iam-blast-radius", + "check_id" => "evalops-pr-lens/iam-blast-radius" + ) + ) + + assert_includes body, EvalOpsPrLensReview::MARKER + assert_includes body, "P1 · 0.94 · iam-blast-radius" assert_includes body, "`evalops-pr-lens/iam-blast-radius`" end @@ -554,11 +615,12 @@ def test_meta_review_marks_incomplete_coverage_when_expected_lens_artifact_is_mi statuses = [] EvalOpsPrLensReview.stub(:run_url, "https://github.com/evalops/.github/actions/runs/1") do - EvalOpsPrLensReview.stub(:delete_marker_comments, ->(**_kwargs) {}) do + EvalOpsPrLensReview.stub(:clear_prior_publication, ->(**_kwargs) {}) do EvalOpsPrLensReview.stub(:post_status, ->(**kwargs) { statuses << kwargs }) do result = EvalOpsPrLensReview.meta_review( artifact_root: dir, - min_confidence: 0.82, + comment_min_confidence: 0.55, + block_min_confidence: 0.80, output: File.join(dir, "meta-review.json") ) @@ -604,8 +666,308 @@ def test_migration_safety_lens_covers_stateful_infra_rollouts assert_includes prompt, "destructive filesystem or cloud-resource cleanup" end + def test_addable_lines_from_patch_maps_right_side_added_lines + patch = <<~PATCH.chomp + @@ -1,4 +1,6 @@ + context line one + -removed line + +added line ten + +added line eleven + context line two + @@ -20,2 +22,3 @@ + another context + +tail addition + PATCH + + lines = EvalOpsPrLensReview.addable_lines_from_patch(patch) + + # First hunk starts at right-side line 1 (context), additions at 2 and 3. + assert_includes lines, 2 + assert_includes lines, 3 + # Second hunk starts at 22 (context "another context"), addition lands at 23. + assert_includes lines, 23 + # Context-only lines are not addable anchors. + refute_includes lines, 1 + refute_includes lines, 22 + end + + def test_addable_lines_from_patch_handles_no_newline_marker + patch = <<~PATCH.chomp + @@ -1 +1,2 @@ + -old + +new line one + +new line two + \\ No newline at end of file + PATCH + + lines = EvalOpsPrLensReview.addable_lines_from_patch(patch) + + assert_equal [1, 2].to_set, lines + end + + def test_addable_lines_by_path_indexes_each_file_patch + files = [ + { "filename" => "infra/main.tf", "patch" => "@@ -1 +1,2 @@\n+new one\n+new two" }, + { "filename" => "infra/no_patch.bin" } + ] + + map = EvalOpsPrLensReview.addable_lines_by_path(repo: "evalops/deploy", pr: 1, files: files) + + assert_equal [1, 2].to_set, map.fetch("infra/main.tf") + assert_empty map.fetch("infra/no_patch.bin") + 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) + off_diff_line = finding("Wrong line", 0.9, 1, "infra/main.tf", 99) + off_diff_path = finding("Unknown file", 0.9, 1, "infra/other.tf", 22) + + assert EvalOpsPrLensReview.finding_inline_anchorable?(in_diff, addable) + refute EvalOpsPrLensReview.finding_inline_anchorable?(off_diff_line, addable) + refute EvalOpsPrLensReview.finding_inline_anchorable?(off_diff_path, addable) + end + + def test_meta_state_blocks_only_on_p0_p1_above_block_threshold + high_p1 = finding("Blocking", 0.85, 1, "a.rb", 1) + low_conf_p1 = finding("Below block", 0.6, 1, "b.rb", 2) + high_p2 = finding("Not blocking priority", 0.99, 2, "c.rb", 3) + + assert_equal "failure", EvalOpsPrLensReview.meta_state([high_p1], block_min_confidence: 0.80) + assert_equal "success", EvalOpsPrLensReview.meta_state([low_conf_p1], block_min_confidence: 0.80) + assert_equal "success", EvalOpsPrLensReview.meta_state([high_p2], block_min_confidence: 0.80) + assert_equal "error", EvalOpsPrLensReview.meta_state([high_p1], block_min_confidence: 0.80, coverage_incomplete: true) + end + + def test_meta_description_reports_honest_coverage_when_no_findings + description = EvalOpsPrLensReview.meta_description( + [], + lens_count: 6, + comment_min_confidence: 0.55 + ) + + assert_equal "6 lenses · 0 findings ≥ 0.55", description + end + + def test_meta_description_counts_findings_above_comment_threshold + findings = [finding("One", 0.7, 1, "a.rb", 1), finding("Two", 0.6, 2, "b.rb", 2)] + description = EvalOpsPrLensReview.meta_description( + findings, + lens_count: 1, + comment_min_confidence: 0.55 + ) + + assert_equal "1 lens · 2 findings ≥ 0.55", description + end + + def test_meta_review_splits_inline_and_summary_and_blocks_on_high_confidence + Dir.mktmpdir do |dir| + review_dir = File.join(dir, "pr-lens-evalops-deploy-10-iam-blast-radius") + FileUtils.mkdir_p(review_dir) + File.write( + File.join(review_dir, "lens-review.json"), + JSON.pretty_generate( + { + "schema_version" => 1, + "repo" => "evalops/deploy", + "pr" => 10, + "lens" => "iam-blast-radius", + "check_id" => "evalops-pr-lens/iam-blast-radius", + "head_sha" => "abc123", + "dropped_findings" => 0, + "findings" => [ + finding("Inline blocking defect", 0.91, 1, "infra/main.tf", 22), + finding("Off-diff medium defect", 0.60, 2, "infra/old.tf", 9) + ] + } + ) + ) + + published = [] + statuses = [] + addable = { "infra/main.tf" => [22].to_set, "infra/old.tf" => [].to_set } + + EvalOpsPrLensReview.stub(:run_url, "https://github.com/evalops/.github/actions/runs/1") do + EvalOpsPrLensReview.stub(:addable_lines_by_path, ->(**_kwargs) { addable }) do + EvalOpsPrLensReview.stub(:publish_review, ->(**kwargs) { published << kwargs }) do + EvalOpsPrLensReview.stub(:post_status, ->(**kwargs) { statuses << kwargs }) do + result = EvalOpsPrLensReview.meta_review( + artifact_root: dir, + comment_min_confidence: 0.55, + block_min_confidence: 0.80, + output: File.join(dir, "meta-review.json") + ) + + # Both findings clear the 0.55 comment threshold. + assert_equal 2, result.fetch("published_findings").length + # Only the P1 @ 0.91 clears the 0.80 block threshold. + assert_equal 1, result.fetch("blocking_findings").length + + call = published.fetch(0) + assert_equal ["Inline blocking defect"], call.fetch(:inline_findings).map { |f| f.fetch("title") } + assert_equal ["Off-diff medium defect"], call.fetch(:summary_findings).map { |f| f.fetch("title") } + assert_equal "abc123", call.fetch(:head_sha) + + assert_equal "failure", statuses.fetch(0).fetch(:state) + end + end + end + end + end + end + + def test_meta_review_green_status_states_coverage_when_only_low_confidence + Dir.mktmpdir do |dir| + review_dir = File.join(dir, "pr-lens-evalops-deploy-11-migration-safety") + FileUtils.mkdir_p(review_dir) + File.write( + File.join(review_dir, "lens-review.json"), + JSON.pretty_generate( + { + "schema_version" => 1, + "repo" => "evalops/deploy", + "pr" => 11, + "lens" => "migration-safety", + "check_id" => "evalops-pr-lens/migration-safety", + "head_sha" => "def456", + "dropped_findings" => 0, + "findings" => [finding("Too speculative", 0.40, 1, "db/001.sql", 3)] + } + ) + ) + + statuses = [] + cleared = [] + + EvalOpsPrLensReview.stub(:run_url, "https://github.com/evalops/.github/actions/runs/1") do + EvalOpsPrLensReview.stub(:clear_prior_publication, ->(**kwargs) { cleared << kwargs }) do + EvalOpsPrLensReview.stub(:post_status, ->(**kwargs) { statuses << kwargs }) do + EvalOpsPrLensReview.meta_review( + artifact_root: dir, + comment_min_confidence: 0.55, + block_min_confidence: 0.80, + output: File.join(dir, "meta-review.json") + ) + + assert_equal "success", statuses.fetch(0).fetch(:state) + assert_equal "1 lens · 0 findings ≥ 0.55", statuses.fetch(0).fetch(:description) + # Nothing cleared the comment threshold, so prior publication is cleared. + refute_empty cleared + end + end + end + end + end + + def test_publish_review_posts_pr_review_and_is_idempotent + api_calls = [] + fake_api = lambda do |*args, **kwargs| + api_calls << { args: args, input: kwargs[:input] } + "" + end + + inline = [ + finding("Inline defect", 0.9, 1, "infra/main.tf", 22).merge( + "lens" => "iam-blast-radius", + "check_id" => "evalops-pr-lens/iam-blast-radius" + ) + ] + + 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 + end + + # 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) + 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 + end + + def test_publish_review_clears_prior_then_skips_post_when_no_findings + api_calls = [] + + EvalOpsPrLensReview.stub(:clear_prior_publication, ->(**_kwargs) { api_calls << :cleared }) do + EvalOpsPrLensReview.stub(:gh_api, ->(*_args, **_kwargs) { api_calls << :posted; "" }) do + EvalOpsPrLensReview.publish_review( + repo: "evalops/deploy", + pr: 10, + head_sha: "abc123", + inline_findings: [], + summary_findings: [], + comment_min_confidence: 0.55, + target_url: nil + ) + end + end + + assert_equal [:cleared], api_calls + end + + def test_clear_prior_publication_deletes_issue_and_review_marker_comments + deletions = [] + list = lambda do |*args, **_kwargs| + if args.include?("repos/evalops/deploy/issues/10/comments") + "111\n" + elsif args.include?("repos/evalops/deploy/pulls/10/comments") + "222\n333\n" + elsif args.include?("--method") + deletions << args + "" + else + "" + end + end + + EvalOpsPrLensReview.stub(:gh_api, list) do + EvalOpsPrLensReview.clear_prior_publication(repo: "evalops/deploy", pr: 10) + end + + assert(deletions.any? { |args| args.include?("repos/evalops/deploy/issues/comments/111") }) + assert(deletions.any? { |args| args.include?("repos/evalops/deploy/pulls/comments/222") }) + assert(deletions.any? { |args| args.include?("repos/evalops/deploy/pulls/comments/333") }) + end + + def test_meta_review_back_compat_min_confidence_maps_to_block_threshold + # Documents that the legacy single knob (env PR_LENS_MIN_CONFIDENCE / + # --min-confidence) now governs blocking, not comment publication. + findings = [finding("P1 finding", 0.7, 1, "a.rb", 1)] + + # At the legacy 0.82 it would not block; the new 0.55 comment default still shows it. + assert_equal "success", EvalOpsPrLensReview.meta_state(findings, block_min_confidence: 0.82) + assert_equal "failure", EvalOpsPrLensReview.meta_state(findings, block_min_confidence: 0.65) + end + private + def capture_warnings + original = $stderr + buffer = StringIO.new + $stderr = buffer + yield + buffer.string.lines.map(&:chomp) + ensure + $stderr = original + end + def finding(title, confidence, priority, path, line) { "title" => title, From 124e5e47171cee145b62be4013b67c6d9288bc5f Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 9 Jun 2026 19:52:27 +0000 Subject: [PATCH 2/5] Fix PR lens review publication idempotency --- .github/scripts/evalops-pr-lens-review.rb | 111 ++++++++++++-------- test/evalops_pr_lens_review_test.rb | 118 ++++++++++++++++++++-- 2 files changed, 181 insertions(+), 48 deletions(-) diff --git a/.github/scripts/evalops-pr-lens-review.rb b/.github/scripts/evalops-pr-lens-review.rb index cf7f4a2..2c9545f 100644 --- a/.github/scripts/evalops-pr-lens-review.rb +++ b/.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) diff --git a/test/evalops_pr_lens_review_test.rb b/test/evalops_pr_lens_review_test.rb index 695afa0..4a40d5f 100644 --- a/test/evalops_pr_lens_review_test.rb +++ b/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 From 65e21d9ebe72f2bcbdc7e9cd881d851c71f1241b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 9 Jun 2026 19:58:58 +0000 Subject: [PATCH 3/5] Fix PR lens publication edge cases --- .github/scripts/evalops-pr-lens-review.rb | 66 +++++++++--- test/evalops_pr_lens_review_test.rb | 125 ++++++++++++++++++---- 2 files changed, 155 insertions(+), 36 deletions(-) diff --git a/.github/scripts/evalops-pr-lens-review.rb b/.github/scripts/evalops-pr-lens-review.rb index 2c9545f..4ec4f45 100644 --- a/.github/scripts/evalops-pr-lens-review.rb +++ b/.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,14 +1357,14 @@ 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) ) @@ -1365,7 +1372,7 @@ def post_summary_comment(repo:, pr:, body:) 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:) diff --git a/test/evalops_pr_lens_review_test.rb b/test/evalops_pr_lens_review_test.rb index 4a40d5f..d87ce8a 100644 --- a/test/evalops_pr_lens_review_test.rb +++ b/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,28 +936,28 @@ 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") @@ -939,9 +965,64 @@ def test_publish_review_posts_summary_comment_and_inline_comments_idempotently 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 From f0c265aaff53cd622bef68f1d4e2eb0f2be4a9d6 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 9 Jun 2026 20:06:18 +0000 Subject: [PATCH 4/5] Fix PR lens inline publication fallback --- .github/scripts/evalops-pr-lens-review.rb | 72 ++++++++++++--- test/evalops_pr_lens_review_test.rb | 105 ++++++++++++++++------ 2 files changed, 140 insertions(+), 37 deletions(-) diff --git a/.github/scripts/evalops-pr-lens-review.rb b/.github/scripts/evalops-pr-lens-review.rb index 4ec4f45..6fddbb9 100644 --- a/.github/scripts/evalops-pr-lens-review.rb +++ b/.github/scripts/evalops-pr-lens-review.rb @@ -1293,10 +1293,21 @@ def append_summary_findings(lines, title, findings, omission_label:) # 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 + # their locations. `failed_inline_findings` are diff findings listed here when + # inline publication fails after selection. `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:, + failed_inline_findings: [] + ) + total = inline_findings.length + overflow_inline_findings.length + summary_findings.length + failed_inline_findings.length lines = [ MARKER, "**EvalOps PR lens review**", @@ -1311,6 +1322,12 @@ def review_summary_body(repo:, pr:, inline_findings:, overflow_inline_findings:, lines << "" end + append_summary_findings( + lines, + "Diff findings (inline publication failed, so listed here):", + failed_inline_findings, + omission_label: "diff finding(s)" + ) append_summary_findings( lines, "Additional diff findings (not posted inline due to the #{MAX_FINDINGS_PER_COMMENT}-comment cap):", @@ -1404,7 +1421,9 @@ def rollback_publication(repo:, pr:, summary_comment_id:, inline_comment_ids:) # not in the diff, or that overflow the inline comment cap, are folded into the # 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. + # summary or erase the prior review. If inline publication fails after findings + # are selected, the script falls back to a summary-only publication so off-diff + # and overflow findings are still published for the run. def publish_review(repo:, pr:, head_sha:, inline_findings:, summary_findings:, comment_min_confidence:, target_url:) if inline_findings.empty? && summary_findings.empty? clear_prior_publication(repo: repo, pr: pr) @@ -1417,10 +1436,28 @@ def publish_review(repo:, pr:, head_sha:, inline_findings:, summary_findings:, c prior_inline_ids = marker_review_comment_ids(repo: repo, pr: pr) published_inline_ids = [] published_summary_id = nil + failed_inline_findings = [] - 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"] + begin + 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 + rescue StandardError => e + rollback_publication( + repo: repo, + pr: pr, + summary_comment_id: nil, + inline_comment_ids: published_inline_ids + ) + warn( + "pr-lens: inline publication failed for #{repo}##{pr} @ #{head_sha}; " \ + "publishing summary only: #{e.message.lines.first.to_s.strip}" + ) + failed_inline_findings = inline_to_publish + overflow_inline_findings + inline_to_publish = [] + overflow_inline_findings = [] + published_inline_ids = [] end published_summary_id = post_summary_comment( @@ -1433,7 +1470,8 @@ def publish_review(repo:, pr:, head_sha:, inline_findings:, summary_findings:, c overflow_inline_findings: overflow_inline_findings, summary_findings: summary_findings, comment_min_confidence: comment_min_confidence, - target_url: target_url + target_url: target_url, + failed_inline_findings: failed_inline_findings ) ) published_summary_id = published_summary_id["id"] if published_summary_id @@ -1517,9 +1555,19 @@ def meta_review(artifact_root:, comment_min_confidence:, block_min_confidence:, if findings.empty? clear_prior_publication(repo: repo, pr: pr) else - addable_by_path = addable_lines_by_path(repo: repo, pr: pr) - inline_findings, summary_findings = findings.partition do |finding| - finding_inline_anchorable?(finding, addable_by_path) + current_head_sha = pr_head_sha(repo: repo, pr: pr) + if current_head_sha != head_sha + warn( + "pr-lens: skipping inline publication for #{repo}##{pr} because PR head advanced " \ + "from #{head_sha} to #{current_head_sha}" + ) + inline_findings = [] + summary_findings = findings + else + addable_by_path = addable_lines_by_path(repo: repo, pr: pr) + inline_findings, summary_findings = findings.partition do |finding| + finding_inline_anchorable?(finding, addable_by_path) + end end publish_review( repo: repo, diff --git a/test/evalops_pr_lens_review_test.rb b/test/evalops_pr_lens_review_test.rb index d87ce8a..5dd572e 100644 --- a/test/evalops_pr_lens_review_test.rb +++ b/test/evalops_pr_lens_review_test.rb @@ -841,27 +841,77 @@ def test_meta_review_splits_inline_and_summary_and_blocks_on_high_confidence addable = { "infra/main.tf" => [22].to_set, "infra/old.tf" => [].to_set } EvalOpsPrLensReview.stub(:run_url, "https://github.com/evalops/.github/actions/runs/1") do - EvalOpsPrLensReview.stub(:addable_lines_by_path, ->(**_kwargs) { addable }) do - EvalOpsPrLensReview.stub(:publish_review, ->(**kwargs) { published << kwargs }) do - EvalOpsPrLensReview.stub(:post_status, ->(**kwargs) { statuses << kwargs }) do - result = EvalOpsPrLensReview.meta_review( - artifact_root: dir, - comment_min_confidence: 0.55, - block_min_confidence: 0.80, - output: File.join(dir, "meta-review.json") - ) + EvalOpsPrLensReview.stub(:pr_head_sha, ->(**_kwargs) { "abc123" }) do + EvalOpsPrLensReview.stub(:addable_lines_by_path, ->(**_kwargs) { addable }) do + EvalOpsPrLensReview.stub(:publish_review, ->(**kwargs) { published << kwargs }) do + EvalOpsPrLensReview.stub(:post_status, ->(**kwargs) { statuses << kwargs }) do + result = EvalOpsPrLensReview.meta_review( + artifact_root: dir, + comment_min_confidence: 0.55, + block_min_confidence: 0.80, + output: File.join(dir, "meta-review.json") + ) + + # Both findings clear the 0.55 comment threshold. + assert_equal 2, result.fetch("published_findings").length + # Only the P1 @ 0.91 clears the 0.80 block threshold. + assert_equal 1, result.fetch("blocking_findings").length + + call = published.fetch(0) + assert_equal ["Inline blocking defect"], call.fetch(:inline_findings).map { |f| f.fetch("title") } + assert_equal ["Off-diff medium defect"], call.fetch(:summary_findings).map { |f| f.fetch("title") } + assert_equal "abc123", call.fetch(:head_sha) + + assert_equal "failure", statuses.fetch(0).fetch(:state) + end + end + end + end + end + end + end - # Both findings clear the 0.55 comment threshold. - assert_equal 2, result.fetch("published_findings").length - # Only the P1 @ 0.91 clears the 0.80 block threshold. - assert_equal 1, result.fetch("blocking_findings").length + def test_meta_review_disables_inline_publication_when_pr_head_has_advanced + Dir.mktmpdir do |dir| + review_dir = File.join(dir, "pr-lens-evalops-deploy-10-iam-blast-radius") + FileUtils.mkdir_p(review_dir) + File.write( + File.join(review_dir, "lens-review.json"), + JSON.pretty_generate( + { + "schema_version" => 1, + "repo" => "evalops/deploy", + "pr" => 10, + "lens" => "iam-blast-radius", + "check_id" => "evalops-pr-lens/iam-blast-radius", + "head_sha" => "abc123", + "dropped_findings" => 0, + "findings" => [finding("Inline defect", 0.91, 1, "infra/main.tf", 22)] + } + ) + ) - call = published.fetch(0) - assert_equal ["Inline blocking defect"], call.fetch(:inline_findings).map { |f| f.fetch("title") } - assert_equal ["Off-diff medium defect"], call.fetch(:summary_findings).map { |f| f.fetch("title") } - assert_equal "abc123", call.fetch(:head_sha) + published = [] - assert_equal "failure", statuses.fetch(0).fetch(:state) + EvalOpsPrLensReview.stub(:run_url, "https://github.com/evalops/.github/actions/runs/1") do + EvalOpsPrLensReview.stub(:pr_head_sha, ->(**_kwargs) { "def456" }) do + EvalOpsPrLensReview.stub(:addable_lines_by_path, ->(**_kwargs) { flunk "unexpected live diff lookup" }) do + EvalOpsPrLensReview.stub(:publish_review, ->(**kwargs) { published << kwargs }) do + EvalOpsPrLensReview.stub(:post_status, ->(**_kwargs) {}) do + warnings = capture_warnings do + EvalOpsPrLensReview.meta_review( + artifact_root: dir, + comment_min_confidence: 0.55, + block_min_confidence: 0.80, + output: File.join(dir, "meta-review.json") + ) + end + + call = published.fetch(0) + assert_empty call.fetch(:inline_findings) + assert_equal ["Inline defect"], call.fetch(:summary_findings).map { |finding| finding.fetch("title") } + assert(warnings.any? { |line| line.include?("skipping inline publication") }) + end end end end @@ -972,7 +1022,7 @@ def test_publish_review_posts_inline_comments_before_summary_and_clears_prior_on 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 + def test_publish_review_falls_back_to_summary_when_inline_publication_fails api_calls = [] inline_posts = 0 inline = 2.times.map do |index| @@ -1000,7 +1050,7 @@ def test_publish_review_rolls_back_partial_inline_publication_without_deleting_p end end - error = assert_raises(RuntimeError) do + warnings = capture_warnings do EvalOpsPrLensReview.stub(:gh_api, fake_api) do EvalOpsPrLensReview.publish_review( repo: "evalops/deploy", @@ -1014,13 +1064,18 @@ def test_publish_review_rolls_back_partial_inline_publication_without_deleting_p end end - assert_equal "inline publish failed", error.message - refute(api_calls.any? do |call| + summary_call = api_calls.find do |call| Array(call[:args]).include?("--method") && Array(call[:args]).include?("repos/evalops/deploy/issues/10/comments") - end) + end + assert summary_call + summary_body = JSON.parse(summary_call.fetch(:input)).fetch("body") + assert_includes summary_body, "Diff findings (inline publication failed, so listed here):" + assert_includes summary_body, "`infra/main.tf:1`" + assert_includes summary_body, "`infra/main.tf:2`" + assert(warnings.any? { |line| line.include?("publishing summary only") }) 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") }) + 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") }) end def test_publish_review_moves_inline_overflow_into_summary_comment From 85ae9c06d57b4257338f66e2d0c9d6acebddf6e9 Mon Sep 17 00:00:00 2001 From: Jonathan Haas <15969068+haasonsaas@users.noreply.github.com> Date: Tue, 9 Jun 2026 13:26:51 -0700 Subject: [PATCH 5/5] test: stub gh_api_paginated_json so discover_open_prs test makes no live API call #147 changed pr_files_metadata to the paginated helper (gh api --paginate --slurp), which the existing gh_api_json stub no longer intercepts; the test then hit the live API and failed in CI (no GH_TOKEN). Stub gh_api_paginated_json so the test is hermetic. Verified passing under a no-auth gh environment. Co-Authored-By: Claude Fable 5 --- test/evalops_pr_lens_review_test.rb | 34 +++++++++++++++-------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/test/evalops_pr_lens_review_test.rb b/test/evalops_pr_lens_review_test.rb index 5dd572e..1465454 100644 --- a/test/evalops_pr_lens_review_test.rb +++ b/test/evalops_pr_lens_review_test.rb @@ -114,25 +114,27 @@ def test_discover_open_prs_can_force_lenses_for_explicit_review_requests "head" => { "sha" => "head", "ref" => "evalopsbot-review-canary" }, "base" => { "sha" => "base", "ref" => "main" } } - api = lambda do |path, **_kwargs| - case path - when "repos/evalops/.github/pulls?state=open&per_page=100" - [pr] - when "repos/evalops/.github/pulls/103/files?per_page=100" - [{ "filename" => ".github/evalopsbot-canary/review-request.md" }] - else - flunk "unexpected gh api path #{path}" - end + list_api = lambda do |path, **_kwargs| + flunk "unexpected gh api path #{path}" unless path == "repos/evalops/.github/pulls?state=open&per_page=100" + [pr] + end + # pr_files_metadata reads files via gh_api_paginated_json (gh api --paginate --slurp), + # which returns an array of pages; stub it so this test never makes a live API call. + files_api = lambda do |path, **_kwargs| + flunk "unexpected paginated path #{path}" unless path == "repos/evalops/.github/pulls/103/files?per_page=100" + [[{ "filename" => ".github/evalopsbot-canary/review-request.md" }]] end - EvalOpsPrLensReview.stub(:gh_api_json, api) do - prs = EvalOpsPrLensReview.discover_open_prs( - repos: ["evalops/.github"], - pr_filter: { "evalops/.github" => [103] }, - force_lenses: %w[migration-safety iam-blast-radius] - ) + EvalOpsPrLensReview.stub(:gh_api_json, list_api) do + EvalOpsPrLensReview.stub(:gh_api_paginated_json, files_api) do + prs = EvalOpsPrLensReview.discover_open_prs( + repos: ["evalops/.github"], + pr_filter: { "evalops/.github" => [103] }, + force_lenses: %w[migration-safety iam-blast-radius] + ) - assert_equal %w[migration-safety iam-blast-radius], prs.fetch(0).fetch("lenses") + assert_equal %w[migration-safety iam-blast-radius], prs.fetch(0).fetch("lenses") + end end end