diff --git a/.github/workflows/ai-code-reviewer.yml b/.github/workflows/ai-code-reviewer.yml index 5221244..45d1b61 100644 --- a/.github/workflows/ai-code-reviewer.yml +++ b/.github/workflows/ai-code-reviewer.yml @@ -32,13 +32,29 @@ jobs: - name: AI Code Review env: + # The || fallbacks deliberately mirror the script's built-in + # defaults so this block documents every knob at the call site — + # keep them in sync when changing a script default. GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} OPENROUTER_API_KEY: ${{ secrets.OPENROUTER_API_KEY }} - AI_MODEL: ${{ vars.AI_MODEL || 'minimax/minimax-m2.5' }} + AI_MODEL: ${{ vars.AI_MODEL || 'z-ai/glm-5.3' }} AI_TEMPERATURE: ${{ vars.AI_TEMPERATURE || '0.1' }} AI_MAX_TOKENS: ${{ vars.AI_MAX_TOKENS || '64000' }} MAX_DIFF_SIZE: ${{ vars.MAX_DIFF_SIZE || '800000' }} EXCLUDE_FILE_PATTERNS: ${{ vars.EXCLUDE_FILE_PATTERNS || '*.lock,*.min.js,*.min.css,package-lock.json,yarn.lock' }} + MAX_SUMMARY_COMMITS: ${{ vars.MAX_SUMMARY_COMMITS || '15' }} + MAX_COMMIT_MESSAGES: ${{ vars.MAX_COMMIT_MESSAGES || '3' }} + INCLUDE_COMMIT_SUMMARY: ${{ vars.INCLUDE_COMMIT_SUMMARY || 'true' }} + INCLUDE_PREVIOUS_REVIEWS: ${{ vars.INCLUDE_PREVIOUS_REVIEWS || 'true' }} + INCLUDE_HUMAN_COMMENTS: ${{ vars.INCLUDE_HUMAN_COMMENTS || 'true' }} + INCLUDE_CHECK_RUNS: ${{ vars.INCLUDE_CHECK_RUNS || 'true' }} + INCLUDE_LABELS: ${{ vars.INCLUDE_LABELS || 'true' }} + INCLUDE_PR_DESCRIPTION: ${{ vars.INCLUDE_PR_DESCRIPTION || 'true' }} + INCLUDE_COMMIT_MESSAGES: ${{ vars.INCLUDE_COMMIT_MESSAGES || 'true' }} + MAX_HUMAN_COMMENTS: ${{ vars.MAX_HUMAN_COMMENTS || '100' }} + MAX_HUMAN_COMMENT_LENGTH: ${{ vars.MAX_HUMAN_COMMENT_LENGTH || '4000' }} + MAX_HUMAN_COMMENTS_TOTAL: ${{ vars.MAX_HUMAN_COMMENTS_TOTAL || '20000' }} + STRUCTURED_OUTPUT: ${{ vars.STRUCTURED_OUTPUT || 'true' }} PR_NUMBER: ${{ github.event.pull_request.number }} REPO_FULL_NAME: ${{ github.repository }} FAIL_ON_REQUESTED_CHANGES: ${{ vars.FAIL_ON_REQUESTED_CHANGES || 'false' }} diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..7a60b85 --- /dev/null +++ b/.gitignore @@ -0,0 +1,2 @@ +__pycache__/ +*.pyc diff --git a/README.md b/README.md index e4ca42a..38af4a7 100644 --- a/README.md +++ b/README.md @@ -19,6 +19,7 @@ This guide explains how to set up the automated AI PR review system using OpenRo **Latest Updates:** - **Thinking Model Support**: Now supports advanced reasoning models like Kimi K2 that use `` tags - **Rich Context**: Includes PR descriptions, commit messages, and human comments for comprehensive reviews +- **Commit Overview**: Tells the model how many commits are already on a PR, who authored them, and how many lines each author changed — with caps configured separately from the (token-heavy) fully quoted messages - **Higher Token Limits**: Default 64k tokens for complete reviews without truncation - **Smart Context Management**: Only fetches most recent AI review to save tokens - **Enhanced Error Handling**: Robust parsing of various AI response formats @@ -63,7 +64,7 @@ The review is posted as a single concise comment on your pull request with appro The workflow is pre-configured with sensible defaults, but you can customize it by setting repository variables in **Settings** → **Secrets and variables** → **Actions** → **Variables**: -- **AI_MODEL**: Change the AI model (default: `moonshotai/kimi-k2-thinking`) +- **AI_MODEL**: Change the AI model (default: `z-ai/glm-5.3`) - See [OpenRouter models](https://openrouter.ai/models) for options - Recommended: Models with reasoning capabilities (Kimi K2, o1, etc.) - **AI_TEMPERATURE**: Adjust randomness (default: `0.1` for consistent reviews) @@ -72,9 +73,15 @@ The workflow is pre-configured with sensible defaults, but you can customize it - For large PRs with thinking models, this prevents cut-off responses - Adjust lower for cost savings on smaller PRs - **MAX_DIFF_SIZE**: Maximum diff size in bytes (default: `800000` / 800KB) +- **MAX_SUMMARY_COMMITS**: How many of the PR's most recent commits the commit overview reads (default: `15`; `0` shows the commit count only). The overview tells the model how many commits are already on the PR, who made them, and each author's added/removed line totals. Each summarized commit costs one extra GitHub API call, but only a handful of numbers enter the prompt, so this cap can stay generous. +- **MAX_COMMIT_MESSAGES**: How many commit messages are fully quoted in the prompt (default: `3`). Fully quoted messages are the token-expensive part of the commit history, hence the separate, smaller cap — the overview (above) still covers many more commits. +- **INCLUDE_COMMIT_SUMMARY**: Include the "There are X commits already on this PR" overview with per-author counts and line totals (default: `true`) +- **MAX_HUMAN_COMMENTS**: How many of the newest human comments are included (default: `100`; `0` includes none at all). Comments are presented newest-first, so when this or the overall budget clips, the oldest go first — the latest feedback always survives. +- **MAX_HUMAN_COMMENT_LENGTH**: Maximum characters per human comment; longer comments are clipped and marked " […truncated]" (default: `4000`) +- **MAX_HUMAN_COMMENTS_TOTAL**: Overall byte budget for the human-comments block (`head -c`); when exceeded, the block is cut and marked (default: `20000`; `0` omits the block entirely) - **STRUCTURED_OUTPUT**: Enforce a JSON Schema on the model's output via OpenRouter structured outputs (default: `true`) - Makes the provider emit valid, correctly-escaped JSON instead of the model hand-writing it — the main cause of "Invalid JSON response from AI model" - - Requires a model/provider that supports `response_format` json_schema (most modern models do; e.g. Kimi K2, MiniMax M2.5) + - Requires a model/provider that supports `response_format` json_schema (most modern models do; e.g. GLM 5.3, Kimi K2, MiniMax M2.5) - Set to `false` only if your chosen model doesn't support structured outputs - **DEBUG_MODE**: Enable debug logging (default: `false`) - ⚠️ Warning: Exposes code diff in workflow logs when enabled @@ -103,11 +110,11 @@ This will generate a fresh review of the current PR state. ## Review Results -The AI reviews your code across all focus areas and reports actionable findings ordered by severity, with a location, failure scenario, impact, and suggested fix. It omits praise, change summaries, and empty sections. A clean review says "No actionable findings." followed by the verdict. Concise output does not lower the token budget available for reasoning and findings. The review is meant to assist human reviewers, not replace them. +The AI reviews your code across all focus areas and reports actionable findings as bullets tagged **must fix**, **should fix**, or **nit** (in that order), each with a location, failure scenario, impact, and suggested fix. Inferences are highlighted with an explicit "Inference (not verified):" label so they are never mistaken for verified facts, and anything that cannot be verified from the diff but is worth a human look is collected in a final "Should be checked" section. The review omits praise, change summaries, and empty sections; a clean review says "No actionable findings." followed by the verdict. Concise output does not lower the token budget available for reasoning and findings. The review is meant to assist human reviewers, not replace them. ## Cost Estimation -Costs with the default Kimi K2 thinking model are very affordable. Based on real usage data: +Costs are very affordable. The ranges below are estimates carried over from real usage with the previous default model (Kimi K2) — re-check against current [OpenRouter pricing](https://openrouter.ai/models) for GLM 5.3: **Typical Costs:** - Small PR (< 1000 lines): $0.01 - $0.02 @@ -120,7 +127,7 @@ Costs with the default Kimi K2 thinking model are very affordable. Based on real - **Total cost: $0.01 - $0.05 per review** **Why So Affordable:** -- Kimi K2 has competitive pricing (~$0.001-$0.003 per 1k tokens) +- GLM 5.3 has competitive pricing (see OpenRouter) - Smart context management (only most recent AI review, limited commit history) - Most PRs are smaller than you think in token count - The 64k token limit is a ceiling, not typical usage @@ -131,7 +138,7 @@ Costs with the default Kimi K2 thinking model are very affordable. Based on real - Number of human comments and commit messages included - OpenRouter provider routing (prices vary slightly by provider) -Check [OpenRouter pricing](https://openrouter.ai/models) for current Kimi K2 rates. +Check [OpenRouter pricing](https://openrouter.ai/models) for current GLM 5.3 rates. ## Customization @@ -187,11 +194,11 @@ If you get a "Diff is too large" error: The workflow fetches and sends these repository elements to the AI: 1. **Code Changes**: Full diff of modified files 2. **PR Description**: Title and description text from the pull request -3. **Commit Messages**: Up to 15 most recent commit messages (excluding merges) -4. **Human Comments**: Comments from human reviewers on the PR; bot comments are excluded, while human comments quoting a review header or marker are retained -5. **Labels**: All repository labels with descriptions and colors -6. **Previous AI Review**: Most recent bot-authored AI review comment only (limited to 10k chars), identified by its review header or `` marker -7. **CI/CD Status**: GitHub Actions check runs and build statuses +3. **Commit Messages**: Up to `MAX_COMMIT_MESSAGES` most recent commit messages (default 3, excluding merges), plus a compact overview stating how many commits are already on the PR, the per-author commit counts, and each author's added/removed line totals (covering up to `MAX_SUMMARY_COMMITS` most recent commits, default 15) +4. **Human Comments**: Comments from human reviewers on the PR, fetched across all pages (not just the first 30), newest first; bot comments are excluded, while human comments quoting a review header or marker are retained. Caps (`MAX_HUMAN_COMMENTS`, `MAX_HUMAN_COMMENT_LENGTH`, `MAX_HUMAN_COMMENTS_TOTAL`) clip the oldest first and mark any truncation. +5. **Labels**: All repository labels with descriptions and colors (kept complete on purpose; the prompt instructs the model to only apply genuinely useful ones) +6. **Previous AI Review**: Most recent bot-authored AI review comment only (limited to 10,000 bytes, marked when truncated), identified by its review header or `` marker +7. **CI/CD Status**: A one-line summary of GitHub Actions check runs ("N of M checks passed") plus only the non-passing runs — failures, skipped, cancelled, timed out, or still running — listed individually (capped at 20 lines with a "+K more" line); fully green matrix shards no longer flood the prompt 8. **PR Metadata**: Pull request details, head SHA, repository information 9. **Files**: May include sensitive configuration files, keys, or credentials @@ -219,4 +226,4 @@ For issues with: ## Development Tests -Run `python3 -m unittest discover -s tests -v` to check the generated request and comment context filters. The tests use local substitutes for GitHub and OpenRouter, with no network requests or model calls. They require Python 3 and the script dependencies (Bash, jq, and Perl). +Run `python3 -B -m unittest discover -s tests -v` to check the generated request and comment context filters. The tests use local substitutes for GitHub and OpenRouter, with no network requests or model calls. They require Python 3 and the script dependencies (Bash, jq, and Perl). The `-B` flag keeps Python from writing `__pycache__` into the tree (CI uses it for the same reason; `.gitignore` covers it as a backstop). diff --git a/ai-reviewer.sh b/ai-reviewer.sh index 98c1062..f19d2d7 100644 --- a/ai-reviewer.sh +++ b/ai-reviewer.sh @@ -26,7 +26,7 @@ if [ -z "$API_KEY" ]; then fi # Configuration with defaults -AI_MODEL="${AI_MODEL:-minimax/minimax-m2.5}" +AI_MODEL="${AI_MODEL:-z-ai/glm-5.3}" AI_TEMPERATURE="${AI_TEMPERATURE:-0.1}" AI_MAX_TOKENS="${AI_MAX_TOKENS:-64000}" MAX_DIFF_SIZE="${MAX_DIFF_SIZE:-5000000}" # 5MB default limit (allows large PRs while preventing excessive API usage) @@ -48,6 +48,48 @@ INCLUDE_LABELS="${INCLUDE_LABELS:-true}" INCLUDE_PR_DESCRIPTION="${INCLUDE_PR_DESCRIPTION:-true}" INCLUDE_COMMIT_MESSAGES="${INCLUDE_COMMIT_MESSAGES:-true}" +# Commit-history context is split by cost: the overview statistic (how many +# commits are on the PR, who made them, added/removed line totals) is cheap in +# tokens and read for many commits, while the fully quoted commit messages are +# token-heavy and therefore capped separately. +# - MAX_SUMMARY_COMMITS: how many past commits the overview statistic reads +# (per-commit line stats cost one GitHub API call each). 0 keeps the count/ +# author line only. +# - MAX_COMMIT_MESSAGES: how many commit messages are fully quoted in the +# prompt. 0 lists no messages. +# Non-numeric values fall back to the defaults. +MAX_SUMMARY_COMMITS="${MAX_SUMMARY_COMMITS:-15}" +MAX_COMMIT_MESSAGES="${MAX_COMMIT_MESSAGES:-3}" +if ! [[ "$MAX_SUMMARY_COMMITS" =~ ^[0-9]+$ ]]; then + MAX_SUMMARY_COMMITS=15 +fi +if ! [[ "$MAX_COMMIT_MESSAGES" =~ ^[0-9]+$ ]]; then + MAX_COMMIT_MESSAGES=3 +fi + +# Include a short "X commits already on this PR" overview in the prompt, with +# per-author commit counts and added/deleted line totals (see +# MAX_SUMMARY_COMMITS for how many commits those cover). +INCLUDE_COMMIT_SUMMARY="${INCLUDE_COMMIT_SUMMARY:-true}" + +# Human comments are high-value context, so these caps are deliberately +# generous and each is configurable: how many of the newest comments are +# kept, how long each may be, and the overall character budget for the +# block. When a cap clips, the oldest of the selected comments go first and +# the clip is marked so the model knows context was cut. +MAX_HUMAN_COMMENTS="${MAX_HUMAN_COMMENTS:-100}" +MAX_HUMAN_COMMENT_LENGTH="${MAX_HUMAN_COMMENT_LENGTH:-4000}" +MAX_HUMAN_COMMENTS_TOTAL="${MAX_HUMAN_COMMENTS_TOTAL:-20000}" +if ! [[ "$MAX_HUMAN_COMMENTS" =~ ^[0-9]+$ ]]; then + MAX_HUMAN_COMMENTS=100 +fi +if ! [[ "$MAX_HUMAN_COMMENT_LENGTH" =~ ^[0-9]+$ ]]; then + MAX_HUMAN_COMMENT_LENGTH=4000 +fi +if ! [[ "$MAX_HUMAN_COMMENTS_TOTAL" =~ ^[0-9]+$ ]]; then + MAX_HUMAN_COMMENTS_TOTAL=20000 +fi + # Read diff content from stdin DIFF_CONTENT=$(cat) @@ -88,32 +130,148 @@ def is_ai_review: startswith("## AI Code Review") or contains("")); ' -# Fetch previous AI review (only the most recent one) for context +# Exact merge-commit predicate for the commit list, shared by the message +# selection and the overview counts: a commit with more than one parent IS a +# merge. Message-prefix heuristics ("Merge...") misfire on subjects like +# "MergeableHashMap: fix iteration". +COMMIT_CLASSIFIERS=' +def is_merge: + (.parents | length) > 1; +' + +# head -c cuts at a byte boundary, which can split a multibyte UTF-8 +# character and leave an invalid sequence at the end of a block. Older jq +# (which builds the request payload) rejects invalid UTF-8 outright; jq 1.7 +# silently substitutes U+FFFD, corrupting the clipped text. Either way the +# trailing incomplete sequence is pure damage — strip it; complete +# characters are never touched. +strip_partial_utf8() { + perl -pe 's/(?:[\xF0-\xF4][\x80-\xBF]{0,2}|[\xE0-\xEF][\x80-\xBF]?|[\xC2-\xDF])$//' +} + +# Fetch the PR's comment list once, shared by the previous-AI-review and +# human-comment context. --paginate is required: a plain gh api call returns +# only the first page (GitHub default: 30 comments, oldest first), which on +# busy PRs would silently hide the newest feedback. --paginate emits one JSON +# array per page back to back, so slurp with jq -s and 'add' to merge them +# into a single array before any slicing happens (the slice must NOT run +# inside a gh api --jq filter — that would apply it per page). +COMMENTS_JSON="[]" +if { [ "$INCLUDE_PREVIOUS_REVIEWS" = "true" ] || [ "$INCLUDE_HUMAN_COMMENTS" = "true" ]; } && [ -n "$PR_NUMBER" ] && [ -n "$REPO_FULL_NAME" ] && [ -n "$GITHUB_TOKEN" ]; then + # Capture gh's exit separately from the merge: the pipeline's status + # would be jq's, so a mid-pagination failure (rate limit, transient + # 5xx) after valid pages would otherwise be silent and leave a stale + # prefix posing as the full list. Treat any fetch failure as no data + # rather than a quietly truncated context. + if COMMENTS_RAW=$(gh api "repos/$REPO_FULL_NAME/issues/$PR_NUMBER/comments" --paginate 2>/dev/null); then + COMMENTS_JSON=$(printf '%s' "$COMMENTS_RAW" | jq -s 'add // []') + else + echo "⚠️ Comment list fetch failed; continuing without comment context" >&2 + COMMENTS_JSON="[]" + fi +fi + +# Previous AI review (only the most recent one) for context. Selected from the +# full merged comment list, so a sticky review beyond page one is still found. PREVIOUS_REVIEWS="" -if [ "$INCLUDE_PREVIOUS_REVIEWS" = "true" ] && [ -n "$PR_NUMBER" ] && [ -n "$REPO_FULL_NAME" ] && [ -n "$GITHUB_TOKEN" ]; then - # Fetch only the most recent AI review comment - PREVIOUS_REVIEWS=$(gh api "repos/$REPO_FULL_NAME/issues/$PR_NUMBER/comments" \ - --jq "$COMMENT_CLASSIFIERS"'[.[] | select(is_ai_review)] | last | if . then "### Previous AI Review (" + .created_at + "):\n" + .body + "\n---\n" else "" end' 2>/dev/null | head -c 10000 || echo "") +if [ "$INCLUDE_PREVIOUS_REVIEWS" = "true" ] && [ "$COMMENTS_JSON" != "[]" ]; then + PREVIOUS_REVIEWS_FULL=$(echo "$COMMENTS_JSON" | jq -r "$COMMENT_CLASSIFIERS"'[.[] | select(is_ai_review)] | last | if . then "### Previous AI Review (" + .created_at + "):\n" + .body + "\n---\n" else "" end' 2>/dev/null || echo "") + PREVIOUS_REVIEWS=$(printf '%s' "$PREVIOUS_REVIEWS_FULL" | head -c 10000 | strip_partial_utf8) + # Same truncation contract as every other budget: detect from the source + # length and mark, so a cut-off prior review (verdict, "Should be checked" + # items) is never mistaken for a complete one. + if [ "$(printf '%s' "$PREVIOUS_REVIEWS_FULL" | wc -c)" -gt 10000 ]; then + PREVIOUS_REVIEWS="$PREVIOUS_REVIEWS +[…truncated at 10000 bytes]" + fi fi -# Fetch human comments for context +# Human comments for context. Human comments are valuable, so the defaults +# are generous and configurable (see MAX_HUMAN_COMMENTS et al.): the newest +# MAX_HUMAN_COMMENTS comments are kept out of the FULL paginated list, +# presented newest-first so an overall-budget clip drops the oldest of the +# selected — never the latest feedback. Per-comment and overall clipping are +# marked as truncated. Exclude all bot comments; previous AI reviews have +# their own context block. HUMAN_COMMENTS="" -if [ "$INCLUDE_HUMAN_COMMENTS" = "true" ] && [ -n "$PR_NUMBER" ] && [ -n "$REPO_FULL_NAME" ] && [ -n "$GITHUB_TOKEN" ]; then - # Exclude all bot comments; previous AI reviews have their own context block. - HUMAN_COMMENTS=$(gh api "repos/$REPO_FULL_NAME/issues/$PR_NUMBER/comments" \ - --jq "$COMMENT_CLASSIFIERS"'[.[] | select(is_bot | not)] | map("**" + .user.login + "** (" + .created_at + "):\n" + .body) | join("\n\n---\n\n")' 2>/dev/null | head -c 20000 || echo "") +if [ "$INCLUDE_HUMAN_COMMENTS" = "true" ] && [ "$COMMENTS_JSON" != "[]" ] && [ "$MAX_HUMAN_COMMENTS_TOTAL" -gt 0 ]; then + HUMAN_COMMENTS_FULL=$(echo "$COMMENTS_JSON" | jq -r \ + --argjson n "$MAX_HUMAN_COMMENTS" --argjson c "$MAX_HUMAN_COMMENT_LENGTH" \ + "$COMMENT_CLASSIFIERS"'[.[] | select(is_bot | not)] + | if $n > 0 then .[-$n:] else [] end + | reverse + | map("**" + (.user.login // "unknown") + "** (" + .created_at + "):\n" + + (if ((.body // "") | length) > $c + then ((.body // "")[0:$c] + " […truncated]") + else (.body // "") end)) + | join("\n\n---\n\n")' 2>/dev/null || echo "") + HUMAN_COMMENTS=$(printf '%s' "$HUMAN_COMMENTS_FULL" | head -c "$MAX_HUMAN_COMMENTS_TOTAL" | strip_partial_utf8) + # Detect clipping from the source length, not the result's byte count: + # command substitution strips trailing newlines, so a comment ending in + # blank lines could otherwise shrink the clipped result below the budget + # and hide a real cut. + if [ "$(printf '%s' "$HUMAN_COMMENTS_FULL" | wc -c)" -gt "$MAX_HUMAN_COMMENTS_TOTAL" ]; then + HUMAN_COMMENTS="$HUMAN_COMMENTS +[…truncated at $MAX_HUMAN_COMMENTS_TOTAL bytes]" + fi fi -# Fetch GitHub Actions check runs status (if PR_NUMBER and REPO_FULL_NAME are set) +# Fetch the PR object once, shared by the check-runs context (head SHA) and +# the PR-description context — both default-on, so a per-feature fetch would +# hit the same endpoint twice on every review. +PR_JSON="" +if { [ "$INCLUDE_CHECK_RUNS" = "true" ] || [ "$INCLUDE_PR_DESCRIPTION" = "true" ]; } && [ -n "$PR_NUMBER" ] && [ -n "$REPO_FULL_NAME" ] && [ -n "$GITHUB_TOKEN" ]; then + if ! PR_JSON=$(gh api "repos/$REPO_FULL_NAME/pulls/$PR_NUMBER" 2>/dev/null); then + echo "⚠️ PR object fetch failed; continuing without PR description and CI status" >&2 + PR_JSON="" + fi +fi + +# Fetch GitHub Actions check runs status (if PR_NUMBER and REPO_FULL_NAME are set). +# Successful checks are collapsed into a one-line count; every non-passing run +# (failure, skipped, cancelled, timed out, still running) is listed +# individually — green matrix shards must not flood the prompt, but skipped +# runs can matter, so they stay visible. The non-passing list is capped at 20 +# lines with a "+K more" line, so a broadly red matrix (shared dependency +# failure, mass cancellation) cannot trade the green-shard flood for a +# red-shard flood exactly when the diff context is largest. CHECK_RUNS_STATUS="" -if [ "$INCLUDE_CHECK_RUNS" = "true" ] && [ -n "$PR_NUMBER" ] && [ -n "$REPO_FULL_NAME" ] && [ -n "$GITHUB_TOKEN" ]; then - # Get the head SHA of the PR - HEAD_SHA=$(gh api "repos/$REPO_FULL_NAME/pulls/$PR_NUMBER" --jq '.head.sha' 2>/dev/null || echo "") +if [ "$INCLUDE_CHECK_RUNS" = "true" ] && [ -n "$PR_JSON" ]; then + # Get the head SHA from the shared PR object + HEAD_SHA=$(echo "$PR_JSON" | jq -r '.head.sha // empty' 2>/dev/null) if [ -n "$HEAD_SHA" ]; then - # Fetch check runs for this commit - CHECK_RUNS_STATUS=$(gh api "repos/$REPO_FULL_NAME/commits/$HEAD_SHA/check-runs" \ - --jq '.check_runs // [] | .[] | "- **\(.name)**: \(.status)\(if .conclusion then " (\(.conclusion))" else "" end)"' 2>/dev/null || echo "") + # Paginate (the endpoint returns 30 runs per page by default — big + # matrix repos exceed that) and merge the pages locally; the summary + # must see the full list, not page one. A failed fetch yields no + # CI context rather than an undercounted summary. + if CHECK_RUNS_RAW=$(gh api "repos/$REPO_FULL_NAME/commits/$HEAD_SHA/check-runs" --paginate 2>/dev/null); then + CHECK_RUNS_JSON=$(printf '%s' "$CHECK_RUNS_RAW" | jq -s 'map(.check_runs // []) | add // []') + else + echo "⚠️ Check-run fetch failed; continuing without CI status" >&2 + CHECK_RUNS_JSON="[]" + fi + CHECK_RUNS_SUMMARY=$(echo "$CHECK_RUNS_JSON" | jq \ + '{total: length, + passed: [.[] | select(.conclusion == "success")] | length, + other: ([.[] | select(.conclusion != "success") + | "- **\(.name)**: \(.status)\(if .conclusion then " (\(.conclusion))" else "" end)"] + | if length > 20 then .[0:20] + ["+ \(length - 20) more non-passing run(s) not listed"] else . end)}' 2>/dev/null || echo "") + + if [ -n "$CHECK_RUNS_SUMMARY" ] && [ "$CHECK_RUNS_SUMMARY" != "null" ]; then + TOTAL_CHECKS=$(echo "$CHECK_RUNS_SUMMARY" | jq -r '.total // 0') + PASSED_CHECKS=$(echo "$CHECK_RUNS_SUMMARY" | jq -r '.passed // 0') + OTHER_CHECKS=$(echo "$CHECK_RUNS_SUMMARY" | jq -r 'if .other then .other | join("\n") else "" end') + + if [ "$TOTAL_CHECKS" -gt 0 ]; then + if [ -n "$OTHER_CHECKS" ]; then + CHECK_RUNS_STATUS="$PASSED_CHECKS of $TOTAL_CHECKS checks passed. Non-passing checks: +$OTHER_CHECKS" + else + CHECK_RUNS_STATUS="All $TOTAL_CHECKS checks passed." + fi + fi + fi fi fi @@ -124,8 +282,12 @@ if [ "$INCLUDE_LABELS" = "true" ] && [ -n "$PR_NUMBER" ] && [ -n "$REPO_FULL_NAM if [ "$DEBUG_MODE" = "true" ]; then echo "🔍 Fetching available labels from repository..." >&2 fi - AVAILABLE_LABELS=$(gh api "repos/$REPO_FULL_NAME/labels" --paginate 2>/dev/null \ - --jq '.[] | "- **\(.name)**: \(.description // "No description") (color: #\(.color))"' || echo "") + AVAILABLE_LABELS="" + if LABELS_RAW=$(gh api "repos/$REPO_FULL_NAME/labels" --paginate 2>/dev/null); then + AVAILABLE_LABELS=$(printf '%s' "$LABELS_RAW" | jq -sr 'add | .[] | "- **\(.name)**: \(.description // "No description") (color: #\(.color))"') + else + echo "⚠️ Label fetch failed; continuing without label context" >&2 + fi if [ "$DEBUG_MODE" = "true" ]; then if [ -n "$AVAILABLE_LABELS" ]; then @@ -137,32 +299,166 @@ if [ "$INCLUDE_LABELS" = "true" ] && [ -n "$PR_NUMBER" ] && [ -n "$REPO_FULL_NAM fi fi -# Fetch PR title and description +# Fetch PR title and description (from the shared PR object) PR_DESCRIPTION="" -if [ "$INCLUDE_PR_DESCRIPTION" = "true" ] && [ -n "$PR_NUMBER" ] && [ -n "$REPO_FULL_NAME" ] && [ -n "$GITHUB_TOKEN" ]; then +if [ "$INCLUDE_PR_DESCRIPTION" = "true" ] && [ -n "$PR_JSON" ]; then if [ "$DEBUG_MODE" = "true" ]; then - echo "🔍 Fetching PR title and description..." >&2 + echo "🔍 Extracting PR title and description..." >&2 + fi + PR_DESCRIPTION_FULL=$(echo "$PR_JSON" | jq -r \ + '"**PR Title**: " + .title + "\n\n**Description**:\n" + (.body // "No description provided")' 2>/dev/null || echo "") + PR_DESCRIPTION=$(printf '%s' "$PR_DESCRIPTION_FULL" | head -c 2000 | strip_partial_utf8) + # Same truncation contract as every other budget: detect from the source + # length and mark, so a cut-off description is never mistaken for the + # complete one. + if [ "$(printf '%s' "$PR_DESCRIPTION_FULL" | wc -c)" -gt 2000 ]; then + PR_DESCRIPTION="$PR_DESCRIPTION +[…truncated at 2000 bytes]" fi - PR_DESCRIPTION=$(gh api "repos/$REPO_FULL_NAME/pulls/$PR_NUMBER" \ - --jq '"**PR Title**: " + .title + "\n\n**Description**:\n" + (.body // "No description provided")' 2>/dev/null | head -c 2000 || echo "") if [ "$DEBUG_MODE" = "true" ] && [ -n "$PR_DESCRIPTION" ]; then echo "✅ Successfully fetched PR description" >&2 fi fi -# Fetch commit messages (limit to 15 most recent, exclude merges) -COMMIT_MESSAGES="" -if [ "$INCLUDE_COMMIT_MESSAGES" = "true" ] && [ -n "$PR_NUMBER" ] && [ -n "$REPO_FULL_NAME" ] && [ -n "$GITHUB_TOKEN" ]; then +# Fetch the PR's commit list once, shared by the commit-message list and the +# commit summary. --paginate emits one JSON array per page back to back, so +# slurp with jq -s and 'add' to merge the pages into a single array (this also +# makes the "most recent N" truncation global instead of per-page). +COMMITS_JSON="[]" +if { [ "$INCLUDE_COMMIT_MESSAGES" = "true" ] || [ "$INCLUDE_COMMIT_SUMMARY" = "true" ]; } && [ -n "$PR_NUMBER" ] && [ -n "$REPO_FULL_NAME" ] && [ -n "$GITHUB_TOKEN" ]; then if [ "$DEBUG_MODE" = "true" ]; then - echo "🔍 Fetching commit messages..." >&2 + echo "🔍 Fetching PR commits..." >&2 + fi + # Same fetch-failure discipline as the comment list: gh's exit is + # checked separately from the page merge, so a mid-pagination failure + # yields no data instead of a stale prefix. + if COMMITS_RAW=$(gh api "repos/$REPO_FULL_NAME/pulls/$PR_NUMBER/commits" --paginate 2>/dev/null); then + COMMITS_JSON=$(printf '%s' "$COMMITS_RAW" | jq -s 'add // []') + else + echo "⚠️ Commit list fetch failed; continuing without commit history" >&2 + COMMITS_JSON="[]" + fi + + if [ "$DEBUG_MODE" = "true" ]; then + echo "✅ Fetched $(echo "$COMMITS_JSON" | jq 'length') commit(s) from the PR" >&2 + fi +fi + +# Format the commit-message list from the cached commit JSON (limit to the +# MAX_COMMIT_MESSAGES most recent, exclude merges). Fully quoted messages are +# the token-expensive part of the history context, hence the separate, smaller +# cap compared to the overview statistic. +COMMIT_MESSAGES="" +if [ "$INCLUDE_COMMIT_MESSAGES" = "true" ] && [ "$COMMITS_JSON" != "[]" ] && [ "$COMMITS_JSON" != "" ]; then + COMMIT_MESSAGES_FULL=$(echo "$COMMITS_JSON" | jq -r --argjson n "$MAX_COMMIT_MESSAGES" \ + "$COMMIT_CLASSIFIERS"'[.[] | select(is_merge | not)] + | if $n > 0 then .[-$n:] else [] end + | .[] | (.commit.message // "") as $message + | ($message | split("\n")[0]) as $subject + | (if ($message | contains("\n")) + then ($message | sub("^[^\n]*\n+"; "") | split("\n") | map(" " + .) | join("\n")) + else "" end) as $body + | "- " + $subject + (if $body != "" then "\n" + $body else "" end)' 2>/dev/null || echo "") + COMMIT_MESSAGES=$(printf '%s' "$COMMIT_MESSAGES_FULL" | head -c 2500 | strip_partial_utf8) + # Same contract as the human-comments budget: detect clipping from the + # source length and mark it, so the model knows messages were cut. + if [ "$(printf '%s' "$COMMIT_MESSAGES_FULL" | wc -c)" -gt 2500 ]; then + COMMIT_MESSAGES="$COMMIT_MESSAGES +[…truncated at 2500 bytes]" fi - COMMIT_MESSAGES=$(gh api "repos/$REPO_FULL_NAME/pulls/$PR_NUMBER/commits" --paginate \ - --jq '[.[] | select(.commit.message | startswith("Merge") | not)] | .[-15:] | .[] | "- " + (.commit.message | split("\n")[0]) + (if (.commit.message | split("\n\n")[1]) then "\n " + (.commit.message | split("\n\n")[1]) else "" end)' 2>/dev/null | head -c 2500 || echo "") if [ "$DEBUG_MODE" = "true" ] && [ -n "$COMMIT_MESSAGES" ]; then COMMIT_COUNT=$(echo "$COMMIT_MESSAGES" | grep -c "^- " || echo "0") - echo "✅ Successfully fetched $COMMIT_COUNT commit messages" >&2 + echo "✅ Kept $COMMIT_COUNT commit message(s) (limit $MAX_COMMIT_MESSAGES)" >&2 + fi +fi + +# Build the commit overview: how many commits are already on the PR, who made +# them, and how many lines each author added/removed. The total comes from the +# cached list; per-commit line stats are NOT part of that list response, so +# each summarized commit costs one extra API call. MAX_SUMMARY_COMMITS bounds +# that cost (0 skips the per-commit calls entirely). The overview is cheap in +# tokens — a handful of numbers — so it may cover many more commits than the +# fully quoted message list (MAX_COMMIT_MESSAGES). +COMMIT_SUMMARY="" +if [ "$INCLUDE_COMMIT_SUMMARY" = "true" ] && [ -n "$COMMITS_JSON" ] && [ "$COMMITS_JSON" != "[]" ]; then + # Count merges directly with the shared classifier rather than deriving + # them by subtraction — self-consistent if the non-merge filter ever + # gains more exclusions — and guard both counts so a jq hiccup degrades + # to zero instead of propagating an empty string. + NONMERGE_COUNT=$(echo "$COMMITS_JSON" | jq "$COMMIT_CLASSIFIERS"'[.[] | select(is_merge | not)] | length' 2>/dev/null || echo 0) + MERGE_COUNT=$(echo "$COMMITS_JSON" | jq "$COMMIT_CLASSIFIERS"'[.[] | select(is_merge)] | length' 2>/dev/null || echo 0) + [[ "$NONMERGE_COUNT" =~ ^[0-9]+$ ]] || NONMERGE_COUNT=0 + [[ "$MERGE_COUNT" =~ ^[0-9]+$ ]] || MERGE_COUNT=0 + + AUTHOR_LINES="" + STATS_FAILURES=0 + if [ "$MAX_SUMMARY_COMMITS" -gt 0 ] && [ "$NONMERGE_COUNT" -gt 0 ]; then + # One "authoradditionsdeletions" row per listed commit, + # aggregated right after — no temp file to leak if a stats fetch + # goes wrong. A failed fetch counts as zero lines but is tracked + # separately, so "unknown" never masquerades as a verified +0/-0 + # (the summary header says how many commits lack line stats). + # The loop reads from process substitution, so it runs in the + # current shell and the counters below persist. + STATS_ROWS="" + while IFS=$'\t' read -r author sha; do + [ -n "$sha" ] || continue + if line_stats=$(gh api "repos/$REPO_FULL_NAME/commits/$sha" \ + --jq '"\(.stats.additions // 0)\t\(.stats.deletions // 0)"' 2>/dev/null); then + : + else + line_stats=$(printf '0\t0') + STATS_FAILURES=$((STATS_FAILURES + 1)) + fi + # Command substitution strips the trailing newline, so append it + # separately — without it the rows concatenate and awk mis-parses. + STATS_ROWS+=$(printf '%s\t%s' "$author" "$line_stats") + STATS_ROWS+=$'\n' + done < <(echo "$COMMITS_JSON" | jq -r --argjson n "$MAX_SUMMARY_COMMITS" \ + "$COMMIT_CLASSIFIERS"'[.[] | select(is_merge | not)] + | if $n > 0 then .[-$n:] else [] end + | .[] | [(.author.login // .commit.author.name), .sha] | @tsv') + # Aggregate per author; sort by added lines, then removed, then name. + AUTHOR_LINES=$(printf '%s' "$STATS_ROWS" | awk -F'\t' '{ count[$1]++; add[$1] += $2; del[$1] += $3 } + END { for (who in count) printf "%s\t%d\t%d\t%d\n", who, count[who], add[who], del[who] }' \ + | LC_ALL=C sort -t$'\t' -k3,3nr -k4,4nr -k1,1) + fi + + if [ "$NONMERGE_COUNT" -gt 0 ]; then + if [ "$NONMERGE_COUNT" -eq 1 ]; then + SUMMARY_HEADER="There is 1 commit already on this PR" + COMMIT_WORD="commit" + else + SUMMARY_HEADER="There are $NONMERGE_COUNT commits already on this PR" + COMMIT_WORD="commits" + fi + [ "$MERGE_COUNT" -gt 0 ] && SUMMARY_HEADER="$SUMMARY_HEADER (excluding $MERGE_COUNT merge commit(s))" + # Failed line-stat fetches must not read as verified zeros. + [ "$STATS_FAILURES" -gt 0 ] && SUMMARY_HEADER="$SUMMARY_HEADER (line stats unavailable for $STATS_FAILURES commit(s))" + if [ -n "$AUTHOR_LINES" ]; then + LISTED=$(( MAX_SUMMARY_COMMITS < NONMERGE_COUNT ? MAX_SUMMARY_COMMITS : NONMERGE_COUNT )) + # Operator-visible signal so "rate limited" is distinguishable + # from "one flaky fetch" when the header reports gaps. + [ "$STATS_FAILURES" -gt 0 ] && echo "⚠️ Line stats unavailable for $STATS_FAILURES of $LISTED listed commit(s)" >&2 + [ "$LISTED" -eq "$NONMERGE_COUNT" ] \ + && SCOPE="across all $NONMERGE_COUNT $COMMIT_WORD" \ + || SCOPE="across the $LISTED most recent of $NONMERGE_COUNT $COMMIT_WORD" + SUMMARY_BULLETS=$(printf '%s\n' "$AUTHOR_LINES" | awk -F'\t' \ + '{ word = ($2 == 1 ? "commit" : "commits") + print sprintf("- **%s**: %s %s, +%s/-%s lines", $1, $2, word, $3, $4) }') + COMMIT_SUMMARY="Commit Summary: +$SUMMARY_HEADER. Per-author commit counts and line totals $SCOPE: +$SUMMARY_BULLETS" + else + COMMIT_SUMMARY="Commit Summary: +$SUMMARY_HEADER." + fi + + if [ "$DEBUG_MODE" = "true" ]; then + echo "✅ Commit summary: $SUMMARY_HEADER" >&2 + fi fi fi @@ -180,7 +476,7 @@ PROMPT_PREFIX="Review this code diff thoroughly and report only actionable findi Focus on security, performance, code quality, and best practices. -Keep the review scannable and grouped by importance. Lead with critical issues if any exist. +Keep the review scannable and grouped by severity: must fix first, then should fix, then nits. " # Add GitHub Actions check status if available @@ -189,7 +485,7 @@ if [ -n "$CHECK_RUNS_STATUS" ]; then GitHub Actions Check Status: $CHECK_RUNS_STATUS -Please consider any failed or pending checks in your review. If tests are failing, investigate whether the code changes might be the cause. +Please consider any failed or pending checks in your review, and treat skipped and neutral runs as informational rather than failures. If tests are failing, investigate whether the code changes might be the cause. " fi @@ -197,7 +493,7 @@ fi if [ -n "$AVAILABLE_LABELS" ]; then PROMPT_PREFIX="${PROMPT_PREFIX} Available Repository Labels: -Please prefer using existing labels from this list over creating new ones: +Prefer existing labels from this list over creating new ones. Only apply labels that are genuinely useful for these changes — when unsure, add none rather than stretching a label to fit: $AVAILABLE_LABELS If none of these labels are appropriate for the changes, you may suggest new ones. @@ -213,6 +509,16 @@ $PR_DESCRIPTION " fi +# Add commit summary if available +if [ -n "$COMMIT_SUMMARY" ]; then + PROMPT_PREFIX="${PROMPT_PREFIX} +$COMMIT_SUMMARY + +Use the commit summary to gauge the PR's size and authorship; it records what already changed, not what should change. + +" +fi + # Add commit messages if available if [ -n "$COMMIT_MESSAGES" ]; then PROMPT_PREFIX="${PROMPT_PREFIX} @@ -227,7 +533,7 @@ fi # Add human comments context if available if [ -n "$HUMAN_COMMENTS" ]; then PROMPT_PREFIX="${PROMPT_PREFIX} -Human Comments on this PR: +Human Comments on this PR (newest first): $HUMAN_COMMENTS Please consider these human comments when reviewing the code. @@ -256,16 +562,14 @@ PROMPT="You are an expert code reviewer. Analyze this code diff thoroughly and r Focus on security, performance, code quality, and best practices. -Focus on high-value issues. Style suggestions are welcome if impactful, but not minor optimizations. Be concise: omit praise, change summaries, empty sections, and repeated conclusions. For each finding, include its file and line location, concrete failure scenario, impact, and suggested fix. Important: Focus on issues directly visible in the diff. If you cannot verify something from the diff alone (e.g., missing context, unclear defaults, code not shown): -- Default: Skip the issue to avoid spam -- Only ask for clarification if it's critical (security vulnerabilities, breaking bugs, data loss risks): \"Cannot verify [X] from diff - please confirm [specific question]\" -- If making an inference about non-critical issues, explicitly label it: \"Inference (not verified): [observation]\" +Focus on high-value issues. Style suggestions are welcome if impactful, but not minor optimizations. Be concise: omit praise, change summaries, empty sections, and repeated conclusions. For each finding, include its file and line location, concrete failure scenario, impact, and suggested fix. Tag every finding with exactly one severity — \"must fix\" (bugs, security issues, breaking changes that should block merge), \"should fix\" (real problems worth addressing but tolerable to defer), or \"nit\" (minor style or polish) — and order findings must fix first, then should fix, then nits. Never present an assumption as verified fact: label every inference explicitly as \"Inference (not verified): [observation]\" so it stands out from verified findings. If you cannot verify something from the diff alone (e.g., missing context, unclear defaults, code not shown), do not speculate and do not bury the question in a finding; add it to a final \"Should be checked\" section as \"Cannot verify [X] from diff - please confirm [specific question]\", limited to checks that genuinely matter (security vulnerabilities, breaking bugs, data loss risks). Review Structure: 1. Start with the \"## AI Code Review\" header -2. List actionable findings as bullet points, ordered by severity; preserve enough detail to understand and fix each issue -3. If there are no actionable findings, write only \"No actionable findings.\" before the verdict; do not add a summary or empty security section -4. End with one of these verdicts ONLY: +2. List actionable findings as bullet points tagged \"must fix\", \"should fix\", or \"nit\", in that order; preserve enough detail to understand and fix each issue, and highlight inferences with the explicit \"Inference (not verified):\" label +3. If specific things cannot be verified from the diff and are worth a human check, list them in a final \"Should be checked\" section before the verdict; omit the section entirely when there is nothing meaningful to check +4. If there are no actionable findings and nothing to check, write only \"No actionable findings.\" before the verdict; do not add a summary or empty security section +5. End with one of these verdicts ONLY: - \"✅ Approved\" (no issues found) - \"✅ Approved with recommendations\" (minor improvements suggested, but not blocking) - \"❌ Request changes\" (critical issues that must be fixed before merge) diff --git a/tests/test_reviewer_context.py b/tests/test_reviewer_context.py index 2b47e81..9cbfd18 100644 --- a/tests/test_reviewer_context.py +++ b/tests/test_reviewer_context.py @@ -7,6 +7,7 @@ import json import os from pathlib import Path +import re import subprocess import sys import tempfile @@ -35,13 +36,53 @@ def comment(body, login="reviewer[bot]", user_type="Bot"): } +def pull_commit(sha, message, login=None, name=None, merge=False): + parents = [{"sha": f"{sha}-parent-1"}, {"sha": f"{sha}-parent-2"}] if merge else [{"sha": f"{sha}-parent-1"}] + return { + "sha": sha, + "parents": parents, + "author": {"login": login} if login is not None else None, + "commit": {"author": {"name": name or login or "unknown"}, "message": message}, + } + + class ReviewerRequestTests(unittest.TestCase): def run_reviewer( - self, comments=(), *, previous=True, human=True, response=None, config=None + self, + comments=(), + *, + previous=True, + human=True, + response=None, + config=None, + pull_commits=None, + commit_stats=None, + check_runs=None, + labels=None, + pr=None, + fail_comments=False, + fail_commits=False, ): with tempfile.TemporaryDirectory() as directory: path = Path(directory) (path / "comments.json").write_text(json.dumps(comments)) + (path / "pull-commits.json").write_text( + json.dumps(pull_commits if pull_commits is not None else []) + ) + (path / "commit-stats.json").write_text(json.dumps(commit_stats or {})) + (path / "pr.json").write_text(json.dumps( + pr if pr is not None + else {"number": 123, "head": {"sha": "abc"}, + "title": "Example PR", "body": "Example body"} + )) + if fail_comments: + (path / "fail-comments").write_text("") + if fail_commits: + (path / "fail-commits").write_text("") + (path / "check-runs.json").write_text(json.dumps( + {"total_count": len(check_runs or []), "check_runs": check_runs or []} + )) + (path / "labels.json").write_text(json.dumps(labels or [])) expected = response if response is not None else CLEAN_REVIEW (path / "response.json").write_text(json.dumps({ "choices": [{ @@ -53,14 +94,54 @@ def run_reviewer( "gh": '''import json, os, subprocess, sys from pathlib import Path args = sys.argv[1:] -assert args[:2] == ["api", "repos/example/repo/issues/123/comments"], args -assert args[2] == "--jq" and len(args) == 4, args +assert args[0] == "api", args path = Path(os.environ["FIXTURE_DIR"]) with (path / "gh-calls.jsonl").open("a") as calls: calls.write(json.dumps(args) + "\\n") -result = subprocess.run(["jq", "-r", args[3]], - input=(path / "comments.json").read_text(), text=True) -sys.exit(result.returncode) +parts = args[1].split("?")[0].split("/")[3:] +if parts == ["issues", "123", "comments"]: + assert args[2:] == ["--paginate"], args + comments = json.loads((path / "comments.json").read_text()) + # Emulate gh api --paginate: one JSON array per page, GitHub's default + # page size of 30, oldest first. + for start in range(0, len(comments), 30): + sys.stdout.write(json.dumps(comments[start:start + 30])) + # A mid-stream failure flag: pages already written, then gh exits 1 — + # the partial-payload scenario the fetch discipline must reject. + if (path / "fail-comments").exists(): + sys.exit(1) + sys.exit(0) +if parts == ["pulls", "123", "commits"]: + assert args[2:] == ["--paginate"], args + if (path / "fail-commits").exists(): + sys.exit(1) + sys.stdout.write((path / "pull-commits.json").read_text()) + sys.exit(0) +if parts == ["pulls", "123"]: + assert len(args) == 2, args + sys.stdout.write((path / "pr.json").read_text()) + sys.exit(0) +if parts == ["commits", "abc", "check-runs"]: + assert args[2:] == ["--paginate"], args + document = json.loads((path / "check-runs.json").read_text()) + runs = document["check_runs"] + for start in range(0, len(runs), 30): + sys.stdout.write(json.dumps( + {"total_count": len(runs), "check_runs": runs[start:start + 30]} + )) + sys.exit(0) +if parts == ["labels"]: + assert args[2:] == ["--paginate"], args + sys.stdout.write((path / "labels.json").read_text()) + sys.exit(0) +if len(parts) == 2 and parts[0] == "commits": + assert args[2] == "--jq" and len(args) == 4, args + stats = json.loads((path / "commit-stats.json").read_text()) + assert parts[1] in stats, parts[1] + result = subprocess.run(["jq", "-r", args[3]], + input=json.dumps(stats[parts[1]]), text=True) + sys.exit(result.returncode) +sys.exit(f"unexpected gh api call: {args}") ''', "curl": '''import os, sys from pathlib import Path @@ -89,6 +170,7 @@ def run_reviewer( "INCLUDE_LABELS": "false", "INCLUDE_PR_DESCRIPTION": "false", "INCLUDE_COMMIT_MESSAGES": "false", + "INCLUDE_COMMIT_SUMMARY": "false", } environment.update(config or {}) result = subprocess.run( @@ -100,15 +182,56 @@ def run_reviewer( self.assertEqual(json.loads(result.stdout), expected) calls_file = path / "gh-calls.jsonl" calls = calls_file.read_text().splitlines() if calls_file.exists() else [] - self.assertEqual(len(calls), int(previous) + int(human)) + self.gh_calls = [json.loads(call) for call in calls] + # Every call must belong to a known context feature, and the + # comment list is fetched exactly once (shared by the previous- + # review and human-comment features) whenever either is enabled. + categorized = (self.comment_calls() + self.commit_calls() + + self.check_calls() + self.label_calls()) + self.assertEqual(len(self.gh_calls), len(categorized)) + self.assertEqual(len(self.comment_calls()), int(bool(previous or human))) return json.loads((path / "request.json").read_text()) + def comment_calls(self): + """GitHub API calls made for comment context.""" + return [ + call for call in getattr(self, "gh_calls", []) + if "/issues/123/comments" in call[1] + ] + + def commit_calls(self): + """GitHub API calls made for the commit history features.""" + return [ + call for call in getattr(self, "gh_calls", []) + if "pulls/123/commits" in call[1] + or ("/commits/" in call[1] and "check-runs" not in call[1]) + ] + + def check_calls(self): + """GitHub API calls made for the check-runs context.""" + return [ + call for call in getattr(self, "gh_calls", []) + if call[1].endswith("/pulls/123") or call[1].endswith("/check-runs") + ] + + def label_calls(self): + """GitHub API calls made for the label context.""" + return [ + call for call in getattr(self, "gh_calls", []) + if call[1].endswith("/labels") + ] + def test_concise_instructions_preserve_review_depth_and_protocol(self): request = self.run_reviewer(previous=False, human=False) prompt = request["messages"][0]["content"] self.assertIn("Analyze this code diff thoroughly", prompt) self.assertIn("omit praise, change summaries, empty sections", prompt) - self.assertIn("ordered by severity", prompt) + for tag in ("must fix", "should fix", "nit"): + self.assertIn(f'"{tag}"', prompt) + self.assertIn("order findings must fix first, then should fix, then nits", prompt) + self.assertIn('label every inference explicitly as "Inference (not verified):', prompt) + self.assertIn('add it to a final "Should be checked" section', prompt) + self.assertIn("omit the section entirely when there is nothing meaningful to check", prompt) self.assertIn("file and line location, concrete failure scenario, impact", prompt) self.assertIn('write only "No actionable findings." before the verdict', prompt) self.assertNotIn("Always include a", prompt) @@ -125,7 +248,7 @@ def test_concise_instructions_preserve_review_depth_and_protocol(self): ["pass", "fail", "uncertain"]) self.assertEqual(request["max_tokens"], 64000) self.assertEqual(request["temperature"], 0.1) - self.assertEqual(request["model"], "minimax/minimax-m2.5") + self.assertEqual(request["model"], "z-ai/glm-5.3") def test_sticky_review_is_only_previous_ai_context(self): body = f"{MARKER}\n## Review results\nSticky AI review content" @@ -154,7 +277,7 @@ def test_humans_quoting_review_identifiers_remain_human(self): for index, body in enumerate(bodies) ]) prompt = request["messages"][0]["content"] - self.assertIn("Human Comments on this PR:", prompt) + self.assertIn("Human Comments on this PR (newest first):", prompt) self.assertNotIn("Previous AI Review (for context", prompt) for body in bodies: self.assertEqual(prompt.count(body), 1) @@ -200,9 +323,12 @@ def test_disabling_human_context_keeps_previous_review(self): def test_actionable_output_and_custom_budget_are_preserved(self): findings = { "review": ( - f"{HEADER}\n\n- [High] file.py:12: Passing an empty list raises " - "IndexError, failing the request. Check the list before indexing." - f"\n\n❌ Request changes\n\n{FOOTER}" + f"{HEADER}\n\n- **must fix** — file.py:12: Passing an empty list " + "raises IndexError, failing the request. Check the list before " + "indexing.\n- **nit** — file.py:40: \"Inference (not verified): \" " + "the loop could early-exit.\n\nShould be checked:\n- Cannot verify " + "the migration is reversible from diff - please confirm a " + f"downgrade path exists.\n\n❌ Request changes\n\n{FOOTER}" ), "fail_pass_workflow": "fail", "labels_added": ["bug", "tests"], @@ -214,6 +340,473 @@ def test_actionable_output_and_custom_budget_are_preserved(self): self.assertEqual(request["max_tokens"], 12345) self.assertNotIn("response_format", request) + def commit_fixture(self): + commits = [ + # A subject starting with "Merge" on a single-parent commit: the + # exact parents-based predicate must count it. + pull_commit("a0", "MergeableHashMap: fix iteration", login="dana"), + pull_commit("a1", "feat: first", login="alice"), + pull_commit("a2", "feat: second", login="alice"), + pull_commit("a3", "fix: bob fix", name="Bob B"), + pull_commit("a4", "Merge branch 'x' into main", login="alice", merge=True), + pull_commit("a5", "feat: third", login="carol"), + ] + stats = { + "a0": {"stats": {"additions": 6, "deletions": 2}}, + "a1": {"stats": {"additions": 10, "deletions": 2}}, + "a2": {"stats": {"additions": 20, "deletions": 3}}, + "a3": {"stats": {"additions": 5, "deletions": 8}}, + "a5": {"stats": {"additions": 1, "deletions": 1}}, + } + return commits, stats + + def test_commit_summary_counts_authors_and_lines(self): + commits, stats = self.commit_fixture() + request = self.run_reviewer( + previous=False, human=False, + pull_commits=commits, commit_stats=stats, + config={"INCLUDE_COMMIT_SUMMARY": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn( + "There are 5 commits already on this PR (excluding 1 merge commit(s))", + prompt, + ) + self.assertIn("across all 5 commits", prompt) + self.assertIn("- **alice**: 2 commits, +30/-5 lines", prompt) + self.assertIn("- **Bob B**: 1 commit, +5/-8 lines", prompt) + self.assertIn("- **carol**: 1 commit, +1/-1 lines", prompt) + # The merge-sounding subject is counted, not skipped. + self.assertIn("- **dana**: 1 commit, +6/-2 lines", prompt) + # Bullet order follows added lines, so the biggest author leads. + self.assertLess( + prompt.index("- **alice**"), prompt.index("- **carol**"), + ) + # Line stats are fetched per listed non-merge commit only. + stats_urls = [call[1] for call in self.commit_calls() if "/pulls/" not in call[1]] + self.assertEqual(len(stats_urls), 5) + self.assertFalse(any("a4" in url for url in stats_urls), stats_urls) + # Only the summary block is present when messages are disabled. + self.assertNotIn("Commit History (showing development journey):", prompt) + + def test_commit_summary_limit_is_independent_of_message_limit(self): + commits, stats = self.commit_fixture() + request = self.run_reviewer( + previous=False, human=False, + pull_commits=commits, commit_stats=stats, + config={ + "INCLUDE_COMMIT_SUMMARY": "true", + "INCLUDE_COMMIT_MESSAGES": "true", + "MAX_SUMMARY_COMMITS": "1", + }, + ) + prompt = request["messages"][0]["content"] + self.assertIn("across the 1 most recent of 5 commits", prompt) + self.assertIn("- **carol**: 1 commit, +1/-1 lines", prompt) + self.assertNotIn("- **alice**:", prompt) + # The message list keeps its own default cap (3) and still shows + # recent history beyond the overview's single-commit scope. + self.assertIn("- feat: second", prompt) + self.assertNotIn("- feat: first", prompt) + stats_urls = [call[1] for call in self.commit_calls() if "/pulls/" not in call[1]] + self.assertEqual(len(stats_urls), 1) + + def test_commit_message_limit_is_configurable(self): + commits, stats = self.commit_fixture() + request = self.run_reviewer( + previous=False, human=False, + pull_commits=commits, commit_stats=stats, + config={"INCLUDE_COMMIT_MESSAGES": "true", "MAX_COMMIT_MESSAGES": "2"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("Commit History (showing development journey):", prompt) + self.assertIn("- feat: third", prompt) + self.assertIn("- fix: bob fix", prompt) + self.assertNotIn("- feat: first", prompt) + self.assertNotIn("- feat: second", prompt) + self.assertNotIn("Commit Summary:", prompt) + # No per-commit stats calls when the summary is disabled. + stats_urls = [call[1] for call in self.commit_calls() if "/pulls/" not in call[1]] + self.assertEqual(stats_urls, []) + + def test_commit_summary_zero_reads_no_individual_commits(self): + commits, stats = self.commit_fixture() + request = self.run_reviewer( + previous=False, human=False, + pull_commits=commits, commit_stats=stats, + config={"INCLUDE_COMMIT_SUMMARY": "true", "MAX_SUMMARY_COMMITS": "0"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("There are 5 commits already on this PR", prompt) + self.assertNotIn("Per-author", prompt) + # One list call only: zero individual commit reads. + urls = [call[1] for call in self.commit_calls()] + self.assertEqual(len(urls), 1) + self.assertIn("pulls/123/commits", urls[0]) + + def test_commit_summary_singular_count_and_fallback_author(self): + commits = [pull_commit("solo", "fix: solo commit", name="Dana D")] + request = self.run_reviewer( + previous=False, human=False, + pull_commits=commits, commit_stats={"solo": {"stats": {"additions": 4, "deletions": 1}}}, + config={"INCLUDE_COMMIT_SUMMARY": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("There is 1 commit already on this PR", prompt) + self.assertIn("- **Dana D**: 1 commit, +4/-1 lines", prompt) + + def test_check_runs_summarize_successes_and_list_non_passing(self): + request = self.run_reviewer( + previous=False, human=False, + check_runs=[ + {"name": "lint", "status": "completed", "conclusion": "success"}, + {"name": "unit-tests (3/8)", "status": "completed", "conclusion": "success"}, + {"name": "e2e-playwright", "status": "completed", "conclusion": "failure"}, + {"name": "ui-shard-2", "status": "completed", "conclusion": "skipped"}, + {"name": "docker-build", "status": "in_progress"}, + ], + config={"INCLUDE_CHECK_RUNS": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("2 of 5 checks passed. Non-passing checks:", prompt) + self.assertIn("- **e2e-playwright**: completed (failure)", prompt) + self.assertIn("- **ui-shard-2**: completed (skipped)", prompt) + self.assertIn("- **docker-build**: in_progress", prompt) + # Successful runs (including matrix shards) collapse into the count. + self.assertNotIn("- **lint**", prompt) + self.assertNotIn("- **unit-tests", prompt) + self.assertEqual(len(self.check_calls()), 2) + + def test_check_runs_all_passed_collapses_to_one_line(self): + request = self.run_reviewer( + previous=False, human=False, + check_runs=[ + {"name": "lint", "status": "completed", "conclusion": "success"}, + {"name": "unit-tests", "status": "completed", "conclusion": "success"}, + ], + config={"INCLUDE_CHECK_RUNS": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("All 2 checks passed.", prompt) + self.assertNotIn("Non-passing", prompt) + self.assertNotIn("- **lint**", prompt) + + def test_labels_instruction_and_list_stay_complete(self): + request = self.run_reviewer( + previous=False, human=False, + labels=[ + {"name": "bug", "description": "Something is broken", "color": "d73a4a"}, + {"name": "ui", "description": "Touches the web stack", "color": "0366d6"}, + ], + config={"INCLUDE_LABELS": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("Only apply labels that are genuinely useful", prompt) + self.assertIn("add none rather than stretching a label to fit", prompt) + # The label list itself stays complete — no trimming of data. + self.assertIn("- **bug**: Something is broken (color: #d73a4a)", prompt) + self.assertIn("- **ui**: Touches the web stack (color: #0366d6)", prompt) + + def test_human_comments_keep_newest_within_count_cap(self): + comments = [ + comment("older feedback that must drop", "alice", "User"), + comment("newest feedback that must survive", "bob", "User"), + ] + request = self.run_reviewer(comments, previous=False, + config={"MAX_HUMAN_COMMENTS": "1"}) + prompt = request["messages"][0]["content"] + self.assertIn("Human Comments on this PR (newest first):", prompt) + self.assertIn("newest feedback that must survive", prompt) + self.assertNotIn("older feedback that must drop", prompt) + + def test_human_comment_length_cap_marks_truncation(self): + body = "x" * 30 + request = self.run_reviewer( + [comment(body, "alice", "User")], previous=False, + config={"MAX_HUMAN_COMMENT_LENGTH": "10"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("x" * 10 + " […truncated]", prompt) + self.assertNotIn("x" * 11, prompt) + + def test_human_comments_total_budget_marks_truncation(self): + request = self.run_reviewer( + [comment("y" * 60, "alice", "User")], previous=False, + config={"MAX_HUMAN_COMMENTS_TOTAL": "50"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("[…truncated at 50 bytes]", prompt) + + def test_multibyte_comment_survives_a_byte_boundary_clip(self): + # The budget lands mid-emoji. Without stripping the partial UTF-8 + # sequence, jq 1.7 substitutes U+FFFD (verified: it exits 0 and + # silently corrupts the clipped text; older jq rejects outright) — + # the assertNotIn below is what pins the guard, since the request + # otherwise builds fine. The header is 34 bytes, so a 39-byte + # budget keeps one complete 4-byte emoji and cuts one byte into + # the next. + request = self.run_reviewer( + [comment("😀" * 20, "alice", "User")], previous=False, + config={"MAX_HUMAN_COMMENTS_TOTAL": "39"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("😀", prompt) + self.assertIn("[…truncated at 39 bytes]", prompt) + self.assertNotIn("\ufffd", prompt) + + def test_partial_comment_fetch_failure_drops_context(self): + # gh fails after emitting valid pages (mid-pagination rate limit): + # the emitted prefix must be discarded, not posing as the full list. + request = self.run_reviewer( + [comment("latest human feedback", "alice", "User")], + previous=False, fail_comments=True, + ) + prompt = request["messages"][0]["content"] + self.assertNotIn("Human Comments on this PR", prompt) + self.assertNotIn("latest human feedback", prompt) + + def test_commits_fetch_failure_skips_history_context(self): + commits, stats = self.commit_fixture() + request = self.run_reviewer( + previous=False, human=False, pull_commits=commits, commit_stats=stats, + fail_commits=True, + config={"INCLUDE_COMMIT_MESSAGES": "true", "INCLUDE_COMMIT_SUMMARY": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertNotIn("Commit Summary:", prompt) + self.assertNotIn("Commit History", prompt) + + def test_previous_review_multibyte_clip_is_clean(self): + # "### Previous AI Review (ts):\n" + the marker line + 9900 r's put + # the 10000-byte cut 21 bytes into the emoji run: five complete + # emoji survive, the sixth is cut mid-sequence and stripped. + sticky = comment(f"{MARKER}\n" + "r" * 9900 + "😀" * 10 + "\n\n✅ Approved") + request = self.run_reviewer([sticky], human=False) + prompt = request["messages"][0]["content"] + self.assertIn("Previous AI Review (for context", prompt) + self.assertIn("[…truncated at 10000 bytes]", prompt) + self.assertIn("😀", prompt) + self.assertNotIn("\ufffd", prompt) + + def test_per_comment_clip_slices_by_character_not_byte(self): + # jq slices by codepoints: a mixed multibyte body clips at a + # character boundary, never mid-sequence. + request = self.run_reviewer( + [comment("αβγ😀ϵζη", "alice", "User")], previous=False, + config={"MAX_HUMAN_COMMENT_LENGTH": "5"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("αβγ😀ϵ […truncated]", prompt) + self.assertNotIn("ζη", prompt) + self.assertNotIn("\ufffd", prompt) + + def test_no_pr_fetch_when_no_feature_needs_it(self): + self.run_reviewer(previous=False, human=False) + pulls_calls = [call for call in self.gh_calls if call[1].endswith("/pulls/123")] + self.assertEqual(pulls_calls, []) + + def test_commit_message_body_is_indented_under_subject(self): + commits = [pull_commit("c1", "subject line\n\nbody paragraph", login="alice")] + request = self.run_reviewer( + previous=False, human=False, pull_commits=commits, + config={"INCLUDE_COMMIT_MESSAGES": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("- subject line\n body paragraph", prompt) + + def test_commit_message_body_keeps_all_paragraphs(self): + commits = [pull_commit("c1", "subject\n\nfirst paragraph\n\nsecond paragraph", login="alice")] + request = self.run_reviewer( + previous=False, human=False, pull_commits=commits, + config={"INCLUDE_COMMIT_MESSAGES": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn(" first paragraph", prompt) + self.assertIn(" second paragraph", prompt) + + def test_commit_message_body_without_blank_line_is_kept(self): + # Non-conforming message (no blank line after the subject): the body + # must not be silently dropped. + commits = [pull_commit("c1", "fix: thing\nDetails line", login="alice")] + request = self.run_reviewer( + previous=False, human=False, pull_commits=commits, + config={"INCLUDE_COMMIT_MESSAGES": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("- fix: thing\n Details line", prompt) + + def test_check_status_prompt_marks_neutral_informational(self): + request = self.run_reviewer( + previous=False, human=False, + check_runs=[ + {"name": "advisory", "status": "completed", "conclusion": "neutral"}, + {"name": "lint", "status": "completed", "conclusion": "success"}, + ], + config={"INCLUDE_CHECK_RUNS": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("1 of 2 checks passed. Non-passing checks:", prompt) + self.assertIn("- **advisory**: completed (neutral)", prompt) + self.assertIn( + "treat skipped and neutral runs as informational rather than failures", + prompt, + ) + + def test_pr_description_clip_is_marked(self): + request = self.run_reviewer( + previous=False, human=False, + pr={"number": 123, "head": {"sha": "abc"}, + "title": "A change", "body": "d" * 3000}, + config={"INCLUDE_PR_DESCRIPTION": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("**PR Title**: A change", prompt) + self.assertIn("[…truncated at 2000 bytes]", prompt) + + def test_pr_object_fetched_once_for_description_and_check_runs(self): + request = self.run_reviewer( + previous=False, human=False, + check_runs=[{"name": "lint", "status": "completed", "conclusion": "success"}], + pr={"number": 123, "head": {"sha": "abc"}, "title": "T", "body": "B"}, + config={"INCLUDE_CHECK_RUNS": "true", "INCLUDE_PR_DESCRIPTION": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("**PR Title**: T", prompt) + self.assertIn("All 1 checks passed.", prompt) + pulls_calls = [call for call in self.gh_calls if call[1].endswith("/pulls/123")] + self.assertEqual(len(pulls_calls), 1, pulls_calls) + + def test_non_passing_check_list_is_capped(self): + request = self.run_reviewer( + previous=False, human=False, + check_runs=[ + {"name": f"fail-{index}", "status": "completed", "conclusion": "failure"} + for index in range(25) + ], + config={"INCLUDE_CHECK_RUNS": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("0 of 25 checks passed. Non-passing checks:", prompt) + self.assertIn("- **fail-19**:", prompt) + self.assertIn("+ 5 more non-passing run(s) not listed", prompt) + self.assertNotIn("- **fail-20**", prompt) + + def test_human_comments_exactly_filling_budget_are_not_marked(self): + # "**alice** (2026-09-12T10:00:00Z):\n" is 34 characters; a 16-char + # body makes the block exactly 50 — no clip, so no marker. + request = self.run_reviewer( + [comment("z" * 16, "alice", "User")], previous=False, + config={"MAX_HUMAN_COMMENTS_TOTAL": "50"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("z" * 16, prompt) + self.assertNotIn("[…truncated", prompt) + + def test_human_comments_clipped_after_trailing_newlines_still_marked(self): + # The comment body ends in blank lines: command substitution strips + # them from the clipped result, which must not hide the cut. + request = self.run_reviewer( + [comment("w" * 60 + "\n\n\n", "alice", "User")], previous=False, + config={"MAX_HUMAN_COMMENTS_TOTAL": "50"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("[…truncated at 50 bytes]", prompt) + + def test_human_comments_reach_beyond_the_first_page(self): + # GitHub's default page holds 30 comments, oldest first. The newest + # comments live on later pages; the selection must span all of them. + many = [comment(f"feedback number {index}", f"user-{index}", "User") + for index in range(1, 41)] + request = self.run_reviewer(many, previous=False, + config={"MAX_HUMAN_COMMENTS": "10"}) + prompt = request["messages"][0]["content"] + self.assertIn("feedback number 40", prompt) + self.assertIn("feedback number 31", prompt) + self.assertNotIn("feedback number 30", prompt) + self.assertNotIn("feedback number 1", prompt) + # One shared paginated fetch, not one call per page or per feature. + self.assertEqual(len(self.comment_calls()), 1) + self.assertEqual(self.comment_calls()[0][2:], ["--paginate"]) + + def test_previous_review_found_beyond_the_first_page(self): + many = [comment(f"noise {index}", f"user-{index}", "User") + for index in range(35)] + sticky = comment(f"{MARKER}\nLatest AI review beyond page one") + request = self.run_reviewer(many + [sticky], human=False) + prompt = request["messages"][0]["content"] + self.assertIn("Latest AI review beyond page one", prompt) + self.assertNotIn("noise 0", prompt) + + def test_commit_messages_clip_is_marked(self): + commits = [ + pull_commit(f"c{i}", f"feat: {i}\n\n" + "m" * 1100, login="alice") + for i in range(3) + ] + request = self.run_reviewer( + previous=False, human=False, pull_commits=commits, + config={"INCLUDE_COMMIT_MESSAGES": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("[…truncated at 2500 bytes]", prompt) + + def test_check_run_summary_spans_all_pages(self): + request = self.run_reviewer( + previous=False, human=False, + check_runs=[ + {"name": f"shard-{index}", "status": "completed", "conclusion": "success"} + for index in range(35) + ] + [{"name": "e2e", "status": "completed", "conclusion": "failure"}], + config={"INCLUDE_CHECK_RUNS": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn("35 of 36 checks passed. Non-passing checks:", prompt) + self.assertIn("- **e2e**: completed (failure)", prompt) + self.assertNotIn("- **shard-0**", prompt) + + def test_previous_review_clip_is_marked(self): + sticky = comment(f"{MARKER}\n" + "r" * 11000 + "\n\n✅ Approved") + request = self.run_reviewer([sticky], human=False) + prompt = request["messages"][0]["content"] + self.assertIn("Previous AI Review (for context", prompt) + self.assertIn("[…truncated at 10000 bytes]", prompt) + + def test_commit_summary_reports_unavailable_line_stats(self): + commits = [ + pull_commit("ok1", "feat: one", login="alice"), + pull_commit("bad1", "feat: two", login="alice"), + ] + # Only ok1 has stats; the fetch for bad1 fails (the stub rejects + # unknown shas), which must be reported instead of reading as +0/-0. + request = self.run_reviewer( + previous=False, human=False, + pull_commits=commits, + commit_stats={"ok1": {"stats": {"additions": 7, "deletions": 2}}}, + config={"INCLUDE_COMMIT_SUMMARY": "true"}, + ) + prompt = request["messages"][0]["content"] + self.assertIn( + "There are 2 commits already on this PR " + "(line stats unavailable for 1 commit(s))", + prompt, + ) + + def test_workflow_forwards_every_configurable_script_knob(self): + root = Path(__file__).resolve().parents[1] + script = (root / "ai-reviewer.sh").read_text() + workflow = (root / ".github" / "workflows" / "ai-code-reviewer.yml").read_text() + # Every ${VAR:-default} knob in the script must have a forwarding + # line in the workflow, derived from the script itself so a new knob + # fails this test until it is forwarded. Per-run values that come + # from the event (not repository variables) are excluded. + knobs = sorted(set(re.findall(r"\$\{([A-Z][A-Z_]+):-", script))) + self.assertIn("AI_MODEL", knobs) + self.assertIn("MAX_HUMAN_COMMENTS_TOTAL", knobs) + not_repository_variables = {"REPO_FULL_NAME"} + for name in knobs: + if name in not_repository_variables: + continue + self.assertIn(name + ": ${{ vars." + name, workflow, name) + if __name__ == "__main__": unittest.main()