diff --git a/.github/scripts/evalops-pr-lens-review.rb b/.github/scripts/evalops-pr-lens-review.rb index e7aa6b2..6fddbb9 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 @@ -420,8 +429,69 @@ 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) + # 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) @@ -709,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"), @@ -963,12 +1033,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 +1081,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,29 +1264,82 @@ 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 + + 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. `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**", "", - "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._" - 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):", + 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") @@ -1207,54 +1355,172 @@ 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 }) + 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 + + # 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:, 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_json( + "--method", "POST", "repos/#{repo}/issues/#{pr}/comments", + input: JSON.generate(body: body) + ) + end + + def post_inline_comment(repo:, pr:, head_sha:, finding:) + location = finding.fetch("code_location") + gh_api_json( + "--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) ) - else - first, *stale = ids - gh_api( - "--method", "PATCH", "repos/#{repo}/issues/comments/#{first}", - input: JSON.generate({ body: body }) + ) + 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 + + 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. 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. 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) + 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 + failed_inline_findings = [] + + 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}" ) - stale.each { |id| gh_api("--method", "DELETE", "repos/#{repo}/issues/comments/#{id}") } + 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( + repo: repo, + pr: pr, + body: review_summary_body( + repo: repo, + pr: pr, + 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, + failed_inline_findings: failed_inline_findings + ) + ) + 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 delete_marker_comments(repo:, pr:) - marker_comment_ids(repo: repo, pr: pr).each do |id| - gh_api("--method", "DELETE", "repos/#{repo}/issues/comments/#{id}") + 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, coverage_incomplete: false) + 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 +1551,48 @@ 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( + 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, 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 +1603,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 +1620,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 +1639,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 +1757,41 @@ 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"] + legacy_block = nil if legacy_block.to_s.empty? 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..1465454 100644 --- a/test/evalops_pr_lens_review_test.rb +++ b/test/evalops_pr_lens_review_test.rb @@ -2,6 +2,9 @@ require "json" require "minitest/autorun" +require "open3" +require "set" +require "stringio" require "tmpdir" require_relative "../.github/scripts/evalops-pr-lens-review" @@ -111,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 @@ -154,17 +159,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 +327,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 +337,78 @@ 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, + overflow_inline_findings: [], + 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_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( + "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 +654,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 +705,513 @@ 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_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) + 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(: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 + + 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)] + } + ) + ) + + published = [] + + 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 + 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_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 = [ + 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(: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 + + 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 do |call| + Array(call[:args]).include?("--method") && Array(call[:args]).include?("repos/evalops/deploy/pulls/10/comments") + end + assert comment_call + comment = JSON.parse(comment_call.fetch(:input)) + assert_equal "abc123", comment.fetch("commit_id") + assert_equal "infra/main.tf", comment.fetch("path") + assert_equal 22, comment.fetch("line") + assert_equal "RIGHT", comment.fetch("side") + assert_includes comment.fetch("body"), EvalOpsPrLensReview::MARKER + assert_operator api_calls.index(comment_call), :<, api_calls.index(summary_call) + assert(api_calls.any? { |call| Array(call[:args]).include?("repos/evalops/deploy/issues/comments/111") }) + assert(api_calls.any? { |call| Array(call[:args]).include?("repos/evalops/deploy/pulls/comments/222") }) + assert(api_calls.any? { |call| Array(call[:args]).include?("repos/evalops/deploy/pulls/comments/333") }) + refute(api_calls.any? { |call| Array(call[:args]).include?("repos/evalops/deploy/pulls/10/reviews") }) + end + + def test_publish_review_falls_back_to_summary_when_inline_publication_fails + 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 + + warnings = capture_warnings 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 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, "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") }) + 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 + 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 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 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 + + 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 + + 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 + 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,