Add diff-line index and review prompt for GitHub PR reviews

new file mode 100644 index 0000000..a285d77 --- /dev/null +++ b/app/services/github_patch_index.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +# Index of the commentable lines in a pull request's diff, built from the +# GitHub "list pull request files" response. GitHub 422s an *entire* review +# when any single comment targets a line that isn't part of the diff, so the +# review job validates every model finding here before posting. +# +# v1 comments only on the RIGHT side (the new file): a line is commentable +# when it appears in a patch hunk as an added (`+`) or context (` `) line, +# keyed by its new-file line number. Files without a `patch` (binaries, very +# large files) have no commentable lines. +class GithubPatchIndex + HUNK_HEADER = /\A@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/ + + # How far a finding's line may be snapped to the nearest commentable line + # before it is dropped (models are often off by a line or two). + CLAMP_DISTANCE = 2 + + def initialize(files) + @lines_by_path = {} + Array(files).each do |file| + path = file["filename"] + next if path.blank? + + @lines_by_path[path] = commentable_lines(file["patch"]) + end + end + + def valid_line?(path, line) + @lines_by_path[path]&.include?(line) || false + end + + # The finding's line if valid, else the nearest commentable line within + # CLAMP_DISTANCE, else nil (caller drops the finding to the summary). + def clamp(path, line) + lines = @lines_by_path[path] + return nil if lines.blank? || !line.is_a?(Integer) + return line if lines.include?(line) + + nearest = lines.min_by { |candidate| (candidate - line).abs } + (nearest - line).abs <= CLAMP_DISTANCE ? nearest : nil + end + + # The patch text with every added/context line prefixed by its new-file line + # number. Feeding the model pre-computed line numbers is what makes the + # `line` values it returns land on commentable lines. + def self.annotate_patch(patch) + return "" if patch.blank? + + new_lineno = nil + patch.each_line.map do |line| + line = line.chomp + if (match = HUNK_HEADER.match(line)) + new_lineno = Integer(match[1], 10) + line + elsif new_lineno.nil? || line.start_with?("-") + line + elsif line.start_with?("+", " ") || line.empty? + annotated = format("%5d %s", new_lineno, line) + new_lineno += 1 + annotated + else + line # "\ No newline at end of file" and other markers + end + end.join("\n") + end + + private + + def commentable_lines(patch) + lines = Set.new + return lines if patch.blank? + + new_lineno = nil + patch.each_line do |line| + line = line.chomp + if (match = HUNK_HEADER.match(line)) + new_lineno = Integer(match[1], 10) + elsif new_lineno.nil? || line.start_with?("-") + # before the first hunk, or an old-side line: no new-file number + elsif line.start_with?("+", " ") || line.empty? + lines << new_lineno + new_lineno += 1 + end + end + lines + end +end

new file mode 100644 index 0000000..93ea27c --- /dev/null +++ b/app/services/github_review_prompt.rb @@ -0,0 +1,218 @@ +# frozen_string_literal: true + +require "json" + +# Builds the chat-completions payload for a GitHub PR review and parses the +# model's reply. Pure functions — all state comes in as arguments — so the +# budgets, exclusions, and parsing are unit-testable without HTTP. +# +# The model's output is untrusted and the comments it produces are posted +# publicly on github.com, so two rules hold throughout: the system prompt +# carries the same identity/non-disclosure constraints as the cloud +# assistant (see ChatCompletionsController::SIGIT_IDENTITY_PROMPT), and +# nothing is ever posted that didn't survive strict JSON parsing. +class GithubReviewPrompt + class ParseError < StandardError; end + + MAX_FILES = 50 + MAX_TOTAL_PATCH_BYTES = 80_000 + MAX_FILE_PATCH_BYTES = 12_000 + MAX_FINDINGS = 12 + MAX_PR_BODY_CHARS = 4_000 + + SEVERITIES = %w[critical warning nit].freeze + + # Generated/vendored artifacts that add bulk but no review signal. Excluded + # files still show up in the walkthrough's file list with a note. + EXCLUDED_PATTERNS = [ + %r{\A(?:.*/)?(?:Gemfile\.lock|package-lock\.json|pnpm-lock\.yaml|yarn\.lock|Cargo\.lock|composer\.lock|poetry\.lock)\z}, + /\.lock\z/, + %r{(?:\A|/)(?:vendor|node_modules|dist|build|target)/}, + /\.min\.(?:js|css)\z/, + %r{\Adb/schema\.rb\z}, + /\.(?:map|svg\.gz|woff2?)\z/ + ].freeze + + SYSTEM_PROMPT = <<~PROMPT + You are siGit Code, the automated code-review assistant from siGit (sigit.si). + If you are asked what model you are, who built, made, or trained you, or what + you are based on, answer only that you are siGit Code. Never state or imply + that you are any third-party model, and never name or hint at the underlying + model, provider, vendor, or infrastructure. + + You are reviewing a GitHub pull request. Each changed file's diff is shown + with the new-file line number prefixed to every added (+) and unchanged + ( ) line. Review only what changed. + + Rules: + - Report real problems: bugs, correctness risks, security issues, broken + edge cases, misleading names or comments. No praise-only comments, no + restating the diff, no style nitpicks a formatter would catch. + - Each finding must point at an added or context line of the diff, using the + prefixed new-file line number of that file. + - Severity is one of: "critical" (likely bug or security problem), + "warning" (probable problem or risky pattern), "nit" (minor, take or leave). + - At most #{MAX_FINDINGS} findings. Fewer, well-chosen findings beat many + shallow ones. Zero findings is a valid answer. + - Keep every comment terse: one or two sentences, no headings or lists. + - The summary is a short markdown walkthrough of the change: what it does, + how the pieces fit, anything reviewers should look at first. No preamble. + + Respond with exactly one JSON object and nothing else — no markdown fences, + no prose before or after — in this shape: + {"summary": "markdown walkthrough of the change", + "findings": [{"path": "path/to/file", "line": 42, "severity": "warning", "comment": "…"}]} + PROMPT + + class << self + # Splits the PR's files into the set to review and the set to mention as + # skipped: hard exclusions first, then a greedy per-file/total byte budget + # (smallest patches first, so one huge file can't crowd out ten small ones). + # Each entry in `skipped` is [file, reason]. + def select_files(files) + candidates, skipped = Array(files).partition { |f| reviewable?(f) } + skipped = skipped.map { |f| [ f, skip_reason(f) ] } + + selected = [] + budget = MAX_TOTAL_PATCH_BYTES + candidates.sort_by { |f| f["patch"].bytesize }.each do |file| + if file["patch"].bytesize <= budget + selected << file + budget -= file["patch"].bytesize + else + skipped << [ file, "over the total diff budget" ] + end + end + + # Preserve GitHub's file ordering for the prompt and the walkthrough. + selected = files.select { |f| selected.include?(f) } + [ selected, skipped ] + end + + # True when the PR can't be reviewed at all (the job posts a summary-only + # note and marks the review skipped/too_large). + def too_large?(files, selected) + Array(files).size > MAX_FILES || (files.present? && selected.empty?) + end + + def payload(pull_request:, files:, skipped:, model:) + { + "model" => model, + "messages" => [ + { "role" => "system", "content" => SYSTEM_PROMPT }, + { "role" => "user", "content" => user_message(pull_request, files, skipped) } + ], + "stream" => false, + "temperature" => 0.2, + "max_tokens" => 4_096, + # Sent but not depended on — parse_response handles fenced/wrapped JSON. + "response_format" => { "type" => "json_object" } + } + end + + # Parses the model reply into {summary:, findings:}. Malformed individual + # findings are dropped; an unparseable reply raises ParseError (the job + # posts nothing rather than garbled output). + def parse_response(content) + data = lenient_json_parse(content.to_s) + summary = data["summary"] + raise ParseError, "missing summary" unless summary.is_a?(String) && summary.present? + + findings = Array(data["findings"]).filter_map { |finding| normalize_finding(finding) } + { summary: summary, findings: findings.first(MAX_FINDINGS) } + end + + def summary_footer(head_sha) + "\n\n---\n*Automated review by [siGit Code](https://sigit.si/code) · commit #{head_sha.to_s.first(7)}*" + end + + # The summary-only comment for PRs over budget. + def too_large_body(files_count) + "This pull request changes #{files_count} files, which is over the size " \ + "limit for an automated review, so siGit Code skipped it. Splitting " \ + "the change into smaller pull requests will get each part a review." + end + + # ── internals ────────────────────────────────────────────────────────────── + + def reviewable?(file) + file["patch"].present? && !excluded_path?(file["filename"]) && + file["patch"].bytesize <= MAX_FILE_PATCH_BYTES + end + + def excluded_path?(path) + EXCLUDED_PATTERNS.any? { |pattern| pattern.match?(path.to_s) } + end + + def skip_reason(file) + if file["patch"].blank? + "no text diff" + elsif excluded_path?(file["filename"]) + "generated or vendored" + else + "diff too large" + end + end + + def user_message(pull_request, files, skipped) + parts = [] + parts << "Pull request: #{pull_request["title"]}" + parts << "Branches: #{pull_request.dig("base", "ref")} <- #{pull_request.dig("head", "ref")}" + body = pull_request["body"].to_s.strip + parts << "Description:\n#{body.truncate(MAX_PR_BODY_CHARS)}" if body.present? + + if skipped.any? + notes = skipped.map { |file, reason| "- #{file["filename"]} (#{reason})" } + parts << "Changed but not shown:\n#{notes.join("\n")}" + end + + diffs = files.map do |file| + header = "#{file["filename"]} (#{file["status"]}, +#{file["additions"]}/-#{file["deletions"]})" + "### #{header}\n```diff\n#{GithubPatchIndex.annotate_patch(file["patch"])}\n```" + end + parts << "Changed files:\n\n#{diffs.join("\n\n")}" + + parts.join("\n\n") + end + + def lenient_json_parse(content) + text = content.strip + text = text.delete_prefix("```json").delete_prefix("```").delete_suffix("```").strip + + begin + data = JSON.parse(text) + rescue JSON::ParserError + start, finish = text.index("{"), text.rindex("}") + raise ParseError, "no JSON object in model reply" if start.nil? || finish.nil? || finish <= start + + begin + data = JSON.parse(text[start..finish]) + rescue JSON::ParserError => e + raise ParseError, "invalid JSON in model reply: #{e.message}" + end + end + + raise ParseError, "model reply is not a JSON object" unless data.is_a?(Hash) + + data + end + + def normalize_finding(finding) + return nil unless finding.is_a?(Hash) + + path = finding["path"] + comment = finding["comment"] + line = finding["line"] + line = Integer(line, exception: false) if line.is_a?(String) + return nil unless path.is_a?(String) && path.present? + return nil unless comment.is_a?(String) && comment.present? + return nil unless line.is_a?(Integer) && line.positive? + + severity = SEVERITIES.include?(finding["severity"]) ? finding["severity"] : "warning" + { path: path, line: line, severity: severity, comment: comment } + end + + private :reviewable?, :excluded_path?, :skip_reason, :user_message, + :lenient_json_parse, :normalize_finding + end +end