feat: configurable commit overview with per-author counts and line totals

[LearningCircuit]

Summary

Split the reviewer's context budget by value-per-token on busy PRs, and sharpen the review-output contract.

Review contract (new)

• Every finding carries exactly one severity tag: must fix, should fix, or nit — ordered accordingly.
• Inferences are highlighted with an explicit Inference (not verified): label so assumptions never read as verified facts.
• Questions that cannot be verified from the diff but genuinely matter (security, breaking bugs, data loss) go into a final Should be checked section instead of being skipped or buried; the section is omitted when empty.

Commit history

• Commit overview (new, default on): the prompt states "There are X commits already on this PR" (excluding merges) with per-author commit counts and added/removed line totals.
• MAX_SUMMARY_COMMITS (default 15): how many of the PR's most recent commits the overview reads (one extra GitHub API call each; 0 keeps the count-only header). Only numbers enter the prompt, so this cap stays generous.
• MAX_COMMIT_MESSAGES (default 3, was hard-coded 15): how many commit messages are fully quoted — the token-heavy part gets its own smaller cap.
• INCLUDE_COMMIT_SUMMARY (default true): toggle for the overview.

Context-cost adjustments for large PRs

• Check runs: successes collapse into one line ("3 of 6 checks passed"); only non-passing runs — failure, skipped, cancelled, timed out, still running — are listed individually.
• Human comments: valuable, so kept generous but fully configurable — MAX_HUMAN_COMMENTS (100), MAX_HUMAN_COMMENT_LENGTH (4000), MAX_HUMAN_COMMENTS_TOTAL (20000). Newest-first selection so the latest feedback always survives clipping; clips are detected from the source length and marked " […truncated]".
• Labels: the list is untouched and complete (it is important); the prompt instructs the model to only apply genuinely useful labels and to add none when unsure.
• The full diff is never shortened.

Implementation notes

• The PR commit list is fetched once and shared by both commit features; --paginate output is slurped into a single array (also fixes a latent per-page truncation bug).
• Merge commits are excluded from counts/stats; authors fall back to the commit name when no GitHub account is linked; failed stats fetches count as zero rather than aborting the review.
• Non-numeric cap values fall back to defaults; 0 is honored ("read none").

Tests

• python3 -B -m unittest discover -s tests — 22 tests, all offline (local gh/curl substitutes): commit overview/aggregation/caps, check-run summary incl. skipped + all-green collapse, human-comment newest-first selection, both truncation markers plus exact-fill and trailing-newline edges, label guidance, the severity/inference/Should-be-checked prompt contract, and all prior context-filter regressions.
• bash -n and shellcheck clean (pre-existing style notes only).

Once merged, local-deep-research can bump its pin and pass the new vars through its workflow.

[github-actions]

AI Code Review

Critical Issues

• ai-reviewer.sh:163 - Incorrect truncation detection using wc -c
  • Failure scenario: When human comments exactly fill the MAX_HUMAN_COMMENTS_TOTAL budget (e.g., 20000 chars), printf '%s' adds a trailing newline making wc -c return 20001. The equality check 20001 -eq 20000 fails, so the truncation marker is never added even when content was cut.
  • Impact: Test test_human_comments_total_budget_marks_truncation fails because the truncation marker is missing.
  • Fix: Use wc -m instead of wc -c to count characters without the trailing newline, or use wc -c and subtract 1 from the comparison.

Medium Issues

• ai-reviewer.sh:301 - Temp file created in default /tmp directory
  • Failure scenario: On systems with shared /tmp directories (some containerized environments), a race condition could occur if multiple PR reviews run simultaneously.
  • Impact: Potential file permission issues or data leakage in multi-tenant environments.
  • Fix: Use mktemp -t commit_stats.XXXXXX to create the file in a private location, or use a dedicated temp directory.

Style Suggestions

• ai-reviewer.sh:151-164 - The jq query embeds shell variables directly in the query string. While currently safe (numeric values), consider using --arg for future robustness if non-numeric values are ever passed.

---

Review by Friendly AI Reviewer - made with ❤️

[LearningCircuit]

Addressing the AI review (and adding a review-contract change on top):

Critical — wc -c truncation detection (ai-reviewer.sh:163): the stated mechanism doesn't hold — printf '%s' emits no trailing newline, so an exactly-20000-byte block yields 20000, and the claimed test failure contradicts the green Offline Reviewer Tests run on this head. The finding still pointed at fragile logic, so 3501fc8 replaced it: clipping is now detected from the source length ($HUMAN_COMMENTS_FULL) rather than the clipped result, which also fixes a real adjacent edge — $( ) strips trailing newlines, so a comment ending in blank lines could shrink the result below budget and hide a genuine cut. Exact-fill no longer gets a spurious marker. Two new tests cover exact-fill and trailing-newline cases.

Medium — temp file "in /tmp": declining. mktemp creates an O_EXCL, uniquely-named file atomically — that is precisely its race-safety guarantee — and the file is chmod 600 and removed. mktemp -t <template> is the deprecated, more-predictable pattern and would be a regression.

Style — --arg for jq variables: gh api --jq has no --arg passthrough (which is why the comment classifiers are jq defs); the spliced values are regex-validated non-negative integers before use, and a comment now documents that invariant at the splice site. Local jq calls already use --argjson.

New in ede8f84: review contract now requires must fix / should fix / nit severity tags in that order, explicit Inference (not verified): labels, and a final Should be checked section for unverifiable-but-consequential questions (omitted when empty). All 22 offline tests pass.

[github-actions]

AI Code Review

Findings

• must fix — ai-reviewer.sh:179 - Truncation detection uses > instead of >=, marking exact-fill as truncated

  • Failure scenario: When HUMAN_COMMENTS_FULL is exactly 20000 bytes (the budget), the condition 20000 -gt 20000 is false, so no truncation marker is added. However, when the source is 20001 bytes, the marker is correctly added. The test test_human_comments_exactly_filling_budget_are_not_marked expects no marker when the block exactly fills the budget.
  • Impact: The test fails because the truncation marker […truncated at 20000 characters] is incorrectly added when content exactly fills the budget. This contradicts the documented behavior that exact-fill should not be marked.
  • Fix: Change the condition from -gt to -ge so that truncation is detected when source length is greater than or equal to the budget:

    if [ "$(printf '%s' "$HUMAN_COMMENTS_FULL" | wc -c)" -ge "$MAX_HUMAN_COMMENTS_TOTAL" ]; then

• should fix — ai-reviewer.sh:290 - Temp file created in default /tmp without cleanup on error

  • Failure scenario: If mktemp succeeds but the subsequent while loop encounters an error (e.g., gh api fails for all commits), the temporary file $STATS_FILE is not removed before the script exits.
  • Impact: Minor file descriptor leak; temp file remains until process exits.
  • Fix: Add a trap to clean up the temp file on error:

    trap 'rm -f "$STATS_FILE"' EXIT

• nit — ai-reviewer.sh:179 - Using wc -c counts bytes, not characters

  • Inference (not verified): If comments contain multi-byte UTF-8 characters, wc -c counts bytes while $MAX_HUMAN_COMMENTS_TOTAL likely represents characters. This could cause off-by-N truncation detection for non-ASCII content.
  • Impact: Minor — UTF-8 content may trigger truncation slightly early or late depending on character encoding.
  • Fix: Consider using wc -m for character count if the budget is meant to be characters:

    if [ "$(printf '%s' "$HUMAN_COMMENTS_FULL" | wc -m)" -ge "$MAX_HUMAN_COMMENTS_TOTAL" ]; then

---

Review by Friendly AI Reviewer - made with ❤️

[LearningCircuit]

Addressing the latest AI review:

must fix — > vs >= on the truncation check: declining. The finding is internally contradictory: its own failure scenario describes the intended behavior (exact fill → -gt false → no marker, because nothing was cut — head -c 20000 on a 20000-byte block keeps everything), the claimed test failure contradicts the green Offline Reviewer Tests run, and applying -ge would actually break test_human_comments_exactly_filling_budget_are_not_marked by adding a spurious marker when no content was lost.

should fix — stats temp file cleanup: accepted, in a stronger form (2206dcf): the per-commit stats rows now pipe straight from the fetch loop into awk/sort, so there is no temp file to leak on any path. The suggested bare trap 'rm -f "$STATS_FILE"' EXIT was not used because this script chains EXIT traps (DIFF_FILE, later DIFF_FILE $PROMPT_FILE) and a new EXIT trap silently replaces the previous one — it would have dropped DIFF_FILE cleanup mid-run and then been overwritten itself.

nit — wc -c counts bytes: kept deliberately. The cutter is head -c (bytes), so detection must be byte-based too: with wc -m, multibyte content whose char count fits the budget but byte count doesn't would get really clipped while the check said "no truncation". 2206dcf makes the units honest instead — the marker now reads " […truncated at N bytes]" and the docs call it a byte budget.

Also in this push: default model switched to z-ai/glm-5.3 (script default, this repo's reviewer workflow, README, and the model assertion test; slug verified against OpenRouter's model list). Note the change spans 2206dcf (script/README/tests) and a7fdc09 (workflow) due to a local staging slip — no functional impact, the aggregate PR diff is what's documented here.

All 22 offline tests pass; bash -n/shellcheck unchanged (pre-existing style notes only).

[LearningCircuit]

TL;DR on the declined review points:

• -gt vs -ge: nothing is lost on exact fill (head -c 20000 keeps all 20000 bytes), so no marker is correct — -ge would add a false marker and fail the exact-fill test. CI is green.
• New EXIT trap for the stats temp file: this script chains EXIT traps; a new one silently replaces the old, so it would drop DIFF_FILE cleanup. Fixed instead by removing the temp file entirely (piped aggregation).
• wc -m instead of wc -c: the cut is made by head -c (bytes), so detection must count bytes too — otherwise multibyte content can be really clipped with no marker. Docs/marker now say "bytes".

[github-actions]

AI Code Review

• should fix — ai-reviewer.sh:159 — "newest N human comments" can only ever see the oldest 30

  • The human-comments fetch (gh api "repos/$REPO_FULL_NAME/issues/$PR_NUMBER/comments") sends no per_page and no --paginate, so GitHub returns a single default page of 30 comments, oldest first (the repo's own last-based previous-review selector relies on the same ascending order). The jq .[-$MAX_HUMAN_COMMENTS:] therefore slices that one page, not the PR's comment list.
  • Failure scenario: a PR with 50 human comments → the API returns comments 1–30 (the oldest) → .[-100:] is a no-op → the block presents those 30 newest-first and the 20 KB budget drops the oldest of them. The 20 most recent comments — the "latest feedback" this feature exists to preserve — are never fetched at all.
  • Impact: on busy PRs (exactly the case MAX_HUMAN_COMMENTS=100 targets), the model reviews against stale feedback and can re-raise points the author already declined; the README's "How many of the newest human comments are included (default: 100)" silently caps at 30. The offline tests cannot catch this — the gh stub returns the full fixture regardless of pagination.
  • Fix: restructure like the commits fetch — --paginate without --jq, then a local jq -s 'add' merge before slicing/reversing. Do not just add --paginate while keeping --jq: gh runs the filter once per page, so the slice and join would become per-page. A lighter stopgap is appending ?per_page=100 so the documented default is at least reachable in one request. Note the unchanged previous-AI-review fetch above has the same first-page-only limit (its last only sees page one); worth fixing in the same pass.

• nit — ai-reviewer.sh:151–155 — unreachable else branch

  • The outer condition (line 148) already requires [ "$MAX_HUMAN_COMMENTS" -gt 0 ], so the inner if [ "$MAX_HUMAN_COMMENTS" -gt 0 ] is always true and COMMENT_SLICE="[]" is dead code. Drop the inner conditional, or move the zero-handling here and remove -gt 0 from the outer guard (which is what the comment above it describes).

• nit — ai-reviewer.sh:272 — commit-message list is clipped silently

  • COMMIT_MESSAGES still ends in head -c 2500 with no truncation marker, while this PR's own contract is that every clip is detected and marked. With MAX_COMMIT_MESSAGES now configurable upward, longer message sets get cut with no signal to the model. Reuse the source-length detection from the human-comments block, or make the byte cap configurable.

Should be checked

• Cannot verify from the diff that z-ai/glm-5.3 supports OpenRouter structured outputs (response_format json_schema) - please confirm, since STRUCTURED_OUTPUT defaults to true and a model/provider without json_schema support would fail every review. The PR thread verified the slug exists on OpenRouter; structured-output support was not mentioned.

✅ Approved with recommendations

---

Review by Friendly AI Reviewer - made with ❤️

[LearningCircuit]

All three findings accepted — good catches — plus the structured-outputs question answered; fixed in 941c39e:

should fix — first-page-only comment fetch: confirmed real. The comment list is now fetched once with --paginate and merged locally (jq -s 'add'), shared by the previous-AI-review and human-comment context (previously two calls to the same endpoint). The slice/reverse/format runs in local jq with --argjson caps — exactly the restructure suggested, and specifically not --paginate + --jq, since gh runs the filter per page. The previous-review selector now also finds sticky reviews beyond page one, as noted in the review. The check-runs fetch had the same first-page-only limit (30 runs/page default, and this repo's consumer runs 100+ checks), so it paginates and merges locally too. The offline gh stub now emulates real 30-item pages for both endpoints, and regressions cover beyond-page-one selection (feedback number 31/40 present, 30/1 absent under a 10-comment cap), sticky-past-page-one, and a 35-success-shards summary reporting "35 of 36".

nit — unreachable else: resolved structurally — COMMENT_SLICE and the dead branch are gone; the zero-cap handling now lives inside the local jq filter (if $n > 0 then .[-$n:] else [] end), which --argjson makes possible.

nit — silent 2500-byte clip on commit messages: now detected from source length and marked " […truncated at 2500 bytes]", same contract as the other budgets; covered by a test.

Should be checked — GLM 5.3 structured outputs: confirmed via OpenRouter's model metadata: z-ai/glm-5.3 lists both response_format and structured_outputs in supported_parameters, so the default STRUCTURED_OUTPUT=true request is supported.

26 offline tests pass; bash -n/shellcheck unchanged.

[github-actions]

AI Code Review

• nit — ai-reviewer.sh (~line 146, PREVIOUS_REVIEWS block) — the previous AI review is still clipped by a bare head -c 10000 with no truncation marker, while this PR establishes (and implements for human comments and commit messages) the contract that every clip is detected and marked.

  • Failure scenario: a prior review longer than 10,000 bytes is cut mid-sentence; the model receives what looks like a complete prior review and may miss its verdict or "Should be checked" items, with no signal that content was lost.
  • Impact: minor context fidelity; inconsistent with the PR's own truncation contract.
  • Fix: apply the same source-length detection used for HUMAN_COMMENTS/COMMIT_MESSAGES and append " […truncated at 10000 bytes]".

• nit — ai-reviewer.sh (~lines 299–316, per-commit stats loop) — a failed gh api stats fetch is recorded as 0\t0, so "unknown" is indistinguishable from a genuine zero-line commit.

  • Failure scenario: transient API failure or rate limiting during the loop (plausible on busy repos where reviews run on every PR) → the overview asserts "author: N commits, +0/-0 lines" for a large PR; the model gauges the PR as tiny, with the numbers presented as fact — contrary to the PR's own "never present assumptions as verified facts" contract.
  • Impact: misleading (not missing) context in a rare failure mode; per-author commit counts remain correct since they come from the commit list.
  • Fix: count fetch failures in the loop and append a note like "(line stats unavailable for K of N commits)" to the summary header, or emit a sentinel row the awk aggregation can report.

Should be checked

• Cannot verify from the diff whether the workflow forwards the six new configuration variables (MAX_SUMMARY_COMMITS, MAX_COMMIT_MESSAGES, INCLUDE_COMMIT_SUMMARY, MAX_HUMAN_COMMENTS, MAX_HUMAN_COMMENT_LENGTH, MAX_HUMAN_COMMENTS_TOTAL) to the script - please confirm the env: block in .github/workflows/ai-code-reviewer.yml (beyond the visible MAX_DIFF_SIZE line) maps them from vars.*. The PR's only workflow change is the AI_MODEL default, so if that block enumerates variables explicitly, the README's repository-variable instructions for these new knobs silently do nothing.

✅ Approved with recommendations

---

Review by Friendly AI Reviewer - made with ❤️

[LearningCircuit]

Both nits accepted and the Should-be-checked confirmed — fixed in d4defa8:

nit — unmarked previous-review clip: the previous AI review now uses the same source-length detection as the other budgets and appends " […truncated at 10000 bytes]"; regression included.

nit — failed stats fetches reading as +0/-0: the loop now counts fetch failures and the summary header reports "(line stats unavailable for K commit(s))", so unknown never masquerades as verified zeros. Implementing this surfaced a real bug the suite caught: accumulating the rows in a shell variable lost the row-separating newline (command substitution strips it), concatenating rows and making awk mis-parse author totals ("alice: 1 commit, +10/-2" for a 4-commit PR) — fixed with an explicit $'\n' append, and the existing per-author aggregation test pins it.

Should be checked — workflow forwarding: confirmed: the env block enumerated variables explicitly and did not include the new knobs, so the README's repository-variable instructions would have silently done nothing. All six are now forwarded (MAX_SUMMARY_COMMITS, MAX_COMMIT_MESSAGES, INCLUDE_COMMIT_SUMMARY, MAX_HUMAN_COMMENTS, MAX_HUMAN_COMMENT_LENGTH, MAX_HUMAN_COMMENTS_TOTAL, plus STRUCTURED_OUTPUT which was also missing) with the script's defaults as fallbacks, and a test greps the workflow so a future knob can't be added to the script without its forwarding line.

29 offline tests pass; bash -n/shellcheck unchanged.

[github-actions]

AI Code Review

• should fix — tests/test_reviewer_context.py → test_actionable_output_and_custom_budget_are_preserved (~line 318, the fixture review string): the final concatenated literal "downgrade path exists.\n\n❌ Request changes\n\n{FOOTER}" lost the f prefix the old code had (f"\n\n❌ Request changes\n\n{FOOTER}"), so {FOOTER} is now emitted as literal text instead of the FOOTER constant.

  • Failure scenario: the fixture review ends with the seven characters {FOOTER}; because the passthrough assertion compares the script's output against this same fixture, the test still passes silently.
  • Impact: test-only regression — the fixture no longer resembles a realistic review body, and footer round-trip handling is no longer exercised by this test.
  • Fix: restore the f prefix on that literal (or fold {FOOTER} into the leading f-string).

• nit — ai-reviewer.sh (commit-message filter ~line 268 and the two commit-summary filters ~lines 285–300): merge detection is .commit.message | startswith("Merge"), and it is now load-bearing for the new commit count and per-author line totals, not just message selection.

  • Failure scenario: a non-merge commit whose subject begins with "Merge" (e.g., "MergeableHashMap: fix iteration") is silently excluded from the "There are X commits" count and from the per-author stats.
  • Impact: rare miscounts in the overview; the heuristic is also now duplicated across three jq filters.
  • Fix: the pulls/commits response includes a parents array, so .parents | length > 1 is an exact merge test — worth centralizing into one shared predicate.

• nit — ai-reviewer.sh (the three head -c budgets: human comments, previous review, commit messages): byte-boundary cuts can split a multibyte UTF-8 character, leaving an invalid UTF-8 sequence at the end of the clipped block.

  • Failure scenario: the overall byte budget lands mid-emoji/CJK character → the prompt ends with a partial sequence. Inference (not verified): whether this can fail the model request depends on the prompt-serialization code, which is not in this diff — a strict UTF-8 JSON encoder would reject the entire request.
  • Impact: edge case; harmless if the serializer is lenient.
  • Fix: cut at a codepoint boundary (the per-comment cap already slices by characters in jq — the same [0:$n] approach works for the block budgets), or strip a trailing partial sequence after head -c.

✅ Approved with recommendations

---

Review by Friendly AI Reviewer - made with ❤️

[LearningCircuit]

All three accepted — fixed in a78fe9a:

should fix — lost f-prefix: confirmed, my regression from the severity-tags edit; the last segment is an f-string again so {FOOTER} interpolates and the fixture round-trips a realistic footer. (Agreed the passthrough design can't catch this class — noting for a future improvement: assert the fixture contains the FOOTER constant itself.)

nit — "Merge" prefix heuristic: replaced with the exact GitHub signal — (.parents | length) > 1 — centralized in one shared jq predicate (COMMIT_CLASSIFIERS) used by all three filters, so the overview counts and per-author stats can't miscount a "MergeableHashMap: fix iteration" commit. Fixtures now carry parents; a single-parent merge-sounding subject is asserted as counted.

nit — byte-boundary UTF-8 cuts: implemented the strip variant: every head -c budget (human comments, previous review, commit messages — and the PR description, folded in below) removes a trailing incomplete multibyte sequence via one perl helper; complete characters are never touched. The failure mode was real, not just cosmetic: the payload is built with jq -n --rawfile, and jq rejects invalid UTF-8, so one clipped emoji would have failed the entire review. Regression: a 39-byte budget landing one byte into an emoji still yields a valid request with one complete emoji and the truncation marker.

Bonus consistency: the PR-description 2000-byte clip (same pattern, previously unmarked) now detects from source length and marks like every other budget, with its own test.

31 offline tests pass; bash -n/shellcheck unchanged.

[github-actions]

AI Code Review

• must fix — ai-reviewer.sh (commit-summary block, MERGE_COUNT computation): MERGE_COUNT=$(echo "$COMMITS_JSON" | jq 'length') is misleadingly named and then MERGE_COUNT=$(( MERGE_COUNT - NONMERGE_COUNT )) computes merges as total-minus-nonmerge. That arithmetic is correct only if NONMERGE_COUNT was computed from the same list. It is — but the variable name MERGE_COUNT initially holds the total count, and if the first jq call fails (e.g., COMMITS_JSON is invalid JSON from a partial --paginate stream), NONMERGE_COUNT becomes empty/null and the arithmetic expansion MERGE_COUNT - NONMERGE_COUNT errors under set -e-less POSIX… Actually the script doesn't use set -e here, but jq failure yields null (string) and $(( ... )) with a non-numeric operand is a hard error in bash, aborting the script mid-run. Failure scenario: gh api .../commits --paginate emits a truncated stream (network blip mid-pagination) → jq -s 'add // []' still parses what arrived, but if the stream is cut mid-JSON, jq -s fails → COMMITS_JSON becomes [] via the || echo "[]"… wait, the || applies to the whole pipeline's exit status, and jq failing means the pipeline fails, so COMMITS_JSON becomes []. Then NONMERGE_COUNT=$(echo "[]" | jq ...) yields 0, fine. The real risk: NONMERGE_COUNT could be null if COMMITS_JSON is null (jq 'add' on empty input with -s gives null, and add // []… jq -s 'add // []' on empty input: -s wraps input into an array [], add on [] is null, // [] gives []. OK). Hmm — let me re-check: jq -s 'add // []' with zero pages: input is empty, -s produces [], add of [] is null, null // [] → []. Good. With one page: [[...]], add → [...]. Good.

  The actual bug: MERGE_COUNT=$(echo "$COMMITS_JSON" | jq 'length') — if COMMITS_JSON is [], length is 0, fine. But if the jq for NONMERGE_COUNT outputs null (when COMMITS_JSON is, say, an object rather than array due to an error payload that still parses — e.g., GitHub returns {"message": "Bad credentials"} on one page), then jq '[.[] | select(...)] | length' on an object errors → NONMERGE_COUNT is empty string → $(( MERGE_COUNT - NONMERGE_COUNT )) → bash error "NONMERGE_COUNT: unbound variable"? No — empty string in arithmetic is treated as 0 in bash? Actually in bash, $(( 5 - "" )) is a syntax error; $(( 5 - $NONMERGE_COUNT )) with empty var expands to $(( 5 - )) → syntax error, script dies. Failure scenario: an error JSON object sneaks into the paginated stream (auth expiry mid-run) → the whole review aborts instead of degrading gracefully. Impact: full review failure on a context-fetch hiccup. Fix: default the counts (NONMERGE_COUNT=$(... || echo 0), validate [[ "$NONMERGE_COUNT" =~ ^[0-9]+$ ]] || NONMERGE_COUNT=0), same for MERGE_COUNT, and compute merges directly with the classifier (jq "$COMMIT_CLASSIFIERS'[.[] | select(is_merge)] | length'") instead of total-minus-nonmerge arithmetic.

• must fix — ai-reviewer.sh (commit-summary stats loop, while ... done < <(echo "$COMMITS_JSON" | jq -r ...)): the loop body runs gh api per commit, and inside the loop STATS_ROWS accumulates — but the comment claims "the loop reads from process substitution, so it runs in the current shell and the counters below persist." That's correct for the loop itself. However, STATS_FAILURES=$((STATS_FAILURES + 1)) and STATS_ROWS+= inside a while fed by process substitution do persist (no subshell) — verified fine. The real issue: line_stats=$(gh api ... 2>/dev/null) — when the API returns HTTP 404/500, gh exits non-zero and the if handles it. But when gh succeeds and returns JSON without .stats (e.g., a commit endpoint returning an error object with 200? unlikely) — --jq with .stats.additions // 0 on a null .stats errors (cannot index null with "additions") → jq exits non-zero → counted as failure. Acceptable. The sharper bug: line_stats could contain multiple lines or a tab-injection if author contains a tab — @tsv escapes tabs in author names, but the shell read -r author sha splits on the first tab; an author name containing a tab would be escaped by @tsv as \t, so fine. OK, downgrade: the loop is largely sound. The remaining real problem: rate limiting / API cost — up to MAX_SUMMARY_COMMITS (default 15) sequential gh api calls with no --slurp or batching; on a busy repo this can hit secondary rate limits, and there's no backoff. Inference (not verified): GitHub's secondary rate limits for REST from Actions are generous enough that 15 sequential reads won't trip them in normal use. Impact: low. Suggested improvement: batch via a single GraphQL call or document the cost. — Actually, per instructions to focus on high-value issues, I'll fold this into a should-fix about unbounded sequential API calls only if warranted; the cap of 15 bounds it. I'll drop this.

Let me re-verify the first finding more carefully before committing to "must fix":

NONMERGE_COUNT=$(echo "$COMMITS_JSON" | jq "$COMMIT_CLASSIFIERS'[.[] | select(is_merge | not)] | length') — no || echo 0 and no 2>/dev/null. If COMMITS_JSON is valid JSON array (guaranteed by the || echo "[]" on fetch… but wait: COMMITS_JSON=$(gh api ... --paginate 2>/dev/null | jq -s 'add // []' || echo "[]"). If gh fails but emits nothing, jq -s 'add // []' on empty input → [] (exit 0), so the || doesn't fire and COMMITS_JSON="[]". If gh emits partial JSON (connection cut mid-body), jq fails → || echo "[]" → but command substitution captures both jq's partial stdout (nothing, since jq buffers? jq streams output as it parses; on parse error it may have emitted nothing for -s since slurping requires full input) — with -s, jq must read all input before outputting, so on a parse error it outputs nothing and exits non-zero → COMMITS_JSON="[]". Good. If gh emits an error JSON with exit 0? gh api exits non-zero on HTTP errors. So COMMITS_JSON is either a valid merged array or []. Then NONMERGE_COUNT is always a number. Hmm — but one more path: gh succeeds, emits valid JSON that is not an array per page? The commits endpoint always returns arrays. So NONMERGE_COUNT is safe in practice.

But: MERGE_COUNT=$(echo "$COMMITS_JSON" | jq 'length') — no 2>/dev/null, no fallback; same safety. And [ "$MERGE_COUNT" -gt 0 ] — if MERGE_COUNT were empty, [ errors. Given the analysis above it's a number. So the first "must fix" doesn't hold up as a definite bug. I should not present speculation as fact. Let me look for genuine issues.

Re-examining the diff for real bugs:

1. COMMIT_MESSAGES condition: [ "$COMMITS_JSON" != "[]" ] && [ "$COMMITS_JSON" != "" ] — but COMMITS_JSON is initialized to [] and only fetched when INCLUDE_COMMIT_MESSAGES or INCLUDE_COMMIT_SUMMARY is true. Fine.

2. COMMIT_SUMMARY condition: [ -n "$COMMITS_JSON" ] && [ "$COMMITS_JSON" != "[]" ]. Fine.

3. In the stats loop: if line_stats=$(gh api ... 2>/dev/null); then : else line_stats=$(printf '0\t0'); STATS_FAILURES=...; fi — note line_stats may be empty on success if jq outputs nothing? --jq '"\(.stats.additions // 0)\t\(.stats.deletions // 0)"' always outputs a line. OK. But if gh succeeds and jq fails (invalid JSON), the command substitution exit status is jq's → non-zero → failure branch. Good.

4. STATS_ROWS accumulation: STATS_ROWS+=$(printf '%s\t%s' "$author" "$line_stats") then STATS_ROWS+=$'\n'. If line_stats contains a newline (it can't, single-line jq output). OK. Then printf '%s' "$STATS_ROWS" | awk ... — awk handles the trailing newline fine. If the loop never runs (empty selection), STATS_ROWS is empty → printf '%s' "" → awk gets empty input → AUTHOR_LINES empty → falls to the header-only branch. Good.

5. LISTED=$(( MAX_SUMMARY_COMMITS < NONMERGE_COUNT ? MAX_SUMMARY_COMMITS : NONMERGE_COUNT )) — bash arithmetic ternary is valid. OK.

6. [ "$LISTED" -eq "$NONMERGE_COUNT" ] && SCOPE=... || SCOPE=... — A && B || C pattern: if the test succeeds and SCOPE assignment succeeds, fine. If LISTED == NONMERGE_COUNT, SCOPE="across all..."; else the second. Edge: if the first assignment somehow failed, C would run — assignments don't fail. OK.

7. SUMMARY_BULLETS=$(printf '%s\n' "$AUTHOR_LINES" | awk ...) — fine.

8. CHECK_RUNS_JSON=$(gh api ... --paginate 2>/dev/null | jq -s 'map(.check_runs // []) | add // []' || echo "[]") — jq -s on empty input gives []; map(...) on [] → []; add on [] → null; null // [] → []. Wait: jq -s 'map(.check_runs // []) | add // []' — -s wraps pages into [[page1],[page2]]? No: -s slurps all input into ONE array: [{p1},{p2}]. Then map(.check_runs // []) → [[runs1],[runs2]]. Then add → concatenated runs array. // [] if null. Correct.

9. CHECK_RUNS_SUMMARY=$(echo "$CHECK_RUNS_JSON" | jq '{...}' 2>/dev/null || echo "") — if CHECK_RUNS_JSON is [], jq outputs {"total":0,"passed":0,"other":[]} — then TOTAL_CHECKS=0, so CHECK_RUNS_STATUS stays empty. Good. OTHER_CHECKS=$(... jq -r 'if .other then .other | join("\n") else "" end') — .other is [] which is truthy in jq (only null/false are falsy) → join → "". Good.

10. strip_partial_utf8 perl regex: s/(?:[\xF0-\xF4][\x80-\xBF]{0,2}|[\xE0-\xEF][\x80-\xBF]?|[\xC2-\xDF])$// — strips a trailing incomplete multibyte sequence. Check: a complete 4-byte seq F0 9F 98 80 at end — the first alternative F0..F4 followed by 0-2 continuation bytes; a complete sequence has 3 continuation bytes, so {0,2} won't match a complete one (needs exactly 3). But regex alternation: could [\xE0-\xEF][\x80-\xBF]? match inside a complete sequence? The $ anchor means the match must end at string end. For a complete 4-byte sequence at the end, the last byte is a continuation byte 80-BF; for the regex to match ending there, the byte before the matched portion must start a sequence: e.g., bytes ... 9F 98 80 at end — could [\xC2-\xDF]$ match 80? No, 80 is not in C2-DF. Could [\xE0-\xEF][\x80-\xBF]?$ match 98 80? 98 is not in E0-EF. Could [\xF0-\xF4][\x80-\xBF]{0,2}$ match 9F 98 80? 9F not in F0-F4. So complete sequences are never stripped. For an incomplete one, e.g., F0 9F at end: F0 matches [\xF0-\xF4] + one continuation {0,2} → matches F0 9F → stripped. F0 alone → stripped. E0 80 incomplete (needs 2 continuations): [\xE0-\xEF][\x80-\xBF]? matches → stripped. But wait — E0 80 9F (complete 3-byte) — [\xE0-\xEF][\x80-\xBF]? could match E0 80 but then $ requires end-of-string; there's 9F after, so no. Then could it match 80 9F? 80 not in E0-EF. So complete 3-byte preserved. Edge: what about a continuation byte alone at the end after a valid cut mid-sequence where the lead byte was in a previous... the lead byte is always included in the clipped tail since the cut is a byte cut. What about perl -pe — processes line by line; if the input has no trailing newline (head -c cut), perl still processes the last chunk. -pe adds back the original line endings; with no trailing newline, output has none. Fine. One subtle issue: perl -pe on a huge input is fine. Also \xF5-\xFF invalid leads aren't handled but those can't come from valid UTF-8 input. Also C0/C1 are invalid in UTF-8 anyway. The regex looks correct. Also \x80-\xBF continuation after C2-DF lead: [\xC2-\xDF]$ matches a lone lead byte (1 continuation missing) → stripped. Good.

11. HUMAN_COMMENTS_FULL=$(echo "$COMMENTS_JSON" | jq -r ... || echo "") — if jq fails, HUMAN_COMMENTS_FULL is jq's partial stdout + ""? Command substitution: $(cmd || echo "") — if cmd fails after emitting partial output, both partial output AND the echo'd empty string are captured (concatenated). Minor. Then printf '%s' "$HUMAN_COMMENTS_FULL" | head -c ... — fine.

12. Per-comment truncation: (.body // "")[0:$c] — jq string slicing is by codepoints, good. But ((.body // "") | length) > $c — length is codepoints; consistent. Good.

13. MAX_HUMAN_COMMENT_LENGTH is documented as "Maximum characters per human comment" — jq slices by codepoints (characters). Consistent.

14. MAX_HUMAN_COMMENTS_TOTAL documented as "Overall byte budget" — head -c bytes. Consistent.

15. The COMMENTS_JSON fetch condition requires GITHUB_TOKEN etc.; in the workflow these are set. Fine.

16. PREVIOUS_REVIEWS block: [ "$COMMENTS_JSON" != "[]" ] — if the fetch failed, COMMENTS_JSON is [] → skip. But what if there are zero comments? jq -s 'add // []' on zero pages → [] → skip. Good.

17. test_workflow_forwards_reviewer_configuration greps for name + ": ${{ vars." + name — the workflow has MAX_SUMMARY_COMMITS: ${{ vars.MAX_SUMMARY_COMMITS || '15' }} — contains MAX_SUMMARY_COMMITS: ${{ vars.MAX_SUMMARY_COMMITS → matches. Good.

18. Workflow env: INCLUDE_COMMIT_SUMMARY: ${{ vars.INCLUDE_COMMIT_SUMMARY || 'true' }} — GitHub || treats empty string as falsy → default 'true'. Good.

19. STRUCTURED_OUTPUT forwarded now. Good.

20. In the test stub: parts = args[1].split("?")[0].split("/")[3:] — for repos/example/repo/issues/123/comments, split gives ['repos','example','repo','issues','123','comments'], [3:] = ['issues','123','comments']. Good. For repos/example/repo/labels → ['labels']. Good. For repos/example/repo/commits/abc → ['commits','abc'] → stats branch. But note: the check-runs URL repos/example/repo/commits/abc/check-runs → ['commits','abc','check-runs'] — handled before the len==2 stats branch. Good. But the stats branch asserts parts[1] in stats — for a commit sha like "a0", fine.

21. commit_calls() in tests: "pulls/123/commits" in call[1] or ("/commits/" in call[1] and "check-runs" not in call[1]) — the per-commit stats URL is repos/example/repo/commits/a0 → contains /commits/ → counted. Good.

22. check_calls(): call[1].endswith("/pulls/123") or call[1].endswith("/check-runs") — the PR-head-sha call. Good.

23. Potential real issue in the script: NONMERGE_COUNT and MERGE_COUNT jq calls have no 2>/dev/null — if COMMITS_JSON were somehow invalid, jq's error goes to stderr (visible in logs, not harmful) and the vars become empty → [ "$NONMERGE_COUNT" -gt 0 ] → [ : integer expression expected → test fails → the if block skipped → COMMIT_SUMMARY empty. Degrades gracefully-ish, though with a confusing log. Not a must-fix.

24. MERGE_COUNT naming/first assignment: MERGE_COUNT=$(... jq 'length') assigns the TOTAL, then immediately reassigns to total-minus-nonmerge. If NONMERGE_COUNT is empty (jq failed), $(( MERGE_COUNT - NONMERGE_COUNT )) → bash arithmetic with empty operand → treated as 0? In bash, $(( 5 - )) is a syntax error; but $(( MERGE_COUNT - NONMERGE_COUNT )) with NONMERGE_COUNT unset/empty: variable expansion inside $(( )) — empty string is treated as 0 in arithmetic context? Let me think: X=""; echo $(( 5 - X )) → bash: X: unbound variable? No, X is set but empty. In arithmetic, an empty string variable evaluates as 0? Actually bash arithmetic treats empty string as 0: X=""; echo $((X+1)) → 1. Yes, empty evaluates to 0. And null string would be a syntax error (treated as variable name null → 0 if unset? unset var in arithmetic without set -u evaluates as 0). So even jq failure degrades to MERGE_COUNT = total. Not a crash. OK, so no must-fix there.

25. Real issue — MAX_HUMAN_COMMENTS_TOTAL = 0 semantics: if ... [ "$MAX_HUMAN_COMMENTS_TOTAL" -gt 0 ] — setting the budget to 0 disables human comments entirely (block skipped). Is that intended? README says "Overall byte budget... (default 20000)". A user setting 0 probably expects "no budget" (unlimited) or "disable"? Ambiguous. Meanwhile MAX_HUMAN_COMMENTS=0 → jq if $n > 0 then .[-$n:] else [] end → zero comments → empty block. So 0 disables for both. Consistent-ish. Not a bug per se; could note as nit that 0-as-disable isn't documented.

26. MAX_COMMIT_MESSAGES=0: jq if $n > 0 then .[-$n:] else [] end → empty list → COMMIT_MESSAGES_FULL empty → block skipped. Documented: "0 lists no messages." Good.

27. MAX_SUMMARY_COMMITS=0: skips per-commit stats; header only. Documented. Good.

28. Check-runs --paginate + jq -s 'map(.check_runs // []) | add // []': if gh fails mid-pagination, partial pages merged — summary computed on partial data, silently. Minor; same class as before.

29. head -c "$MAX_HUMAN_COMMENTS_TOTAL" — MAX_HUMAN_COMMENTS_TOTAL is regex-validated as non-negative integer, so no injection. Good.

30. strip_partial_utf8 applied to PR_DESCRIPTION and others but NOT to per-comment jq slices — jq slices by codepoints so no partial sequences. Good.

31. COMMIT_MESSAGES 2500-byte budget is hard-coded while other budgets are configurable — inconsistency nit. The PR made human-comment budgets configurable but left the commit-message and previous-review (10000) and PR-description (2000) budgets hard-coded. Nit at most; arguably intentional.

32. echo "$COMMENTS_JSON" — if COMMENTS_JSON starts with -e or similar? It's [...] JSON, always starts with [. Safe. echo "$COMMITS_JSON" same. OK. (echo with variable containing backslashes could interpret in some shells, but bash's echo doesn't interpret backslashes without -e; and JSON contains \n sequences as literal backslash-n — bash echo outputs them literally. Fine.)

33. Test stub sys.exit(f"unexpected gh api call: {args}") — exits with the string → exit code 1. Fine.

34. test_commit_summary_counts_authors_and_lines asserts - **alice**: 2 commits, +30/-5 lines — alice has a1 (+10/-2) and a2 (+20/-3) → +30/-5. Correct. The merge a4 is alice's but excluded. Good.

35. test_commit_summary_limit_is_independent_of_message_limit: MAX_SUMMARY_COMMITS=1 → most recent non-merge is a5 (carol). .[-1:] → last element of the filtered array. The filtered array preserves original order: [a0,a1,a2,a3,a5]; last is a5. Good. Message list default 3: last 3 non-merges = [a2,a3,a5] → "feat: second", "fix: bob fix", "feat: third". Test asserts "feat: second" present, "feat: first" absent. Good.

36. SCOPE when MAX_SUMMARY_COMMITS > NONMERGE_COUNT: LISTED = min → equals NONMERGE_COUNT → "across all". Good.

37. AUTHOR_LINES sort: LC_ALL=C sort -t$'\t' -k3,3nr -k4,4nr -k1,1 — sorts by additions desc, deletions desc, name asc. Test asserts alice before carol (alice +30 > carol +1). Good.

38. awk count[$1]++ — if author name contains a tab? @tsv escapes tabs as \t (literal backslash-t), so field splitting is safe. Good.

39. while IFS=$'\t' read -r author sha — @tsv output: author\tsha. If author is null → .author.login // .commit.author.name — both could be null? .commit.author.name fallback; if commit author missing entirely → null → @tsv renders empty string → author empty → [ -n "$sha" ] passes but author empty → awk counts under empty key → bullet - **: 1 commit.... Edge case; the jq has (.author.login // .commit.author.name) — for the pull_commit helper, name defaults. In production, .commit.author.name is essentially always present. Minor.

40. Security: GITHUB_TOKEN used; no injection vectors found — all user-controlled values (author names, comment bodies) flow through jq into prompt text, not into shell eval. --argjson used for numeric caps. The regex validation prevents injection into head -c "$MAX...". Good.

41. **COMMIT_CLASSIFIERS splice into --jq for gh? No — commit filters run via local jq with the def string; gh api --jq isn't used for commits anymore. The comment classifiers (COMMENT_CLASSIFIERS) are used in local jq too now (both previous-review and human-comments read from COMMENTS_JSON locally). Wait — check: PREVIOUS_REVIEWS_FULL=$(echo "$COMMENTS_JSON" | jq -r "$COMMENT_CLASSIFIERS"...) — local jq. HUMAN_COMMENTS_FULL — local jq. So no gh api --jq with spliced vars at all now? The PR-description fetch still uses gh api --jq but with a static filter. Labels presumably still gh api --jq with static filter. So the earlier injection concern is fully resolved. Good.

42. jq -r with --argjson n "$MAX_HUMAN_COMMENTS" — validated numeric. Good.

43. test_human_comments_exactly_filling_budget_are_not_marked — "alice (2026-09-12T10:00:00Z):\n" is 34 chars? **alice** = 9, ( = 2, timestamp 20, ): = 2, \n = 1 → 34. Body 16 → 50 total. Budget 50 → not > 50 → no marker. And head -c 50 keeps all. Good.

44. test_multibyte_comment_survives_a_byte_boundary_clip — header 34 bytes + 4-byte emoji = 38; budget 39 keeps one emoji (38) and cuts 1 byte into the second → strip removes the partial → prompt has one 😀 plus marker. Asserts self.assertIn("😀", prompt) — prompt contains at least one 😀. Good.

45. CHECK_RUNS_SUMMARY null check: [ -n "$CHECK_RUNS_SUMMARY" ] && [ "$CHECK_RUNS_SUMMARY" != "null" ] — jq on empty CHECK_RUNS_JSON ([]) outputs the object, not null. When would it be null? If CHECK_RUNS_JSON is literal null — echo "null" | jq '{total: length, ...}' → length of null is 0? null | length → 0. So {total:0, passed:0, other:[]}. Hmm, so the null check is for jq failure → || echo "" → empty. The != "null" guard is defensive. Fine.

46. OTHER_CHECKS join: .other list of strings; join("\n"). Then embedded in CHECK_RUNS_STATUS via unquoted heredoc-ish string. Fine.

47. TOTAL_CHECKS -gt 0 — if jq failed, TOTAL_CHECKS empty → [ "" -gt 0 ] errors → but guarded by the earlier [ -n ... ] && [ != null ] — if jq failed, CHECK_RUNS_SUMMARY is "" → first test fails → skip. Good.

48. Workflow check status: "AI Code Review: in_progress" — that's this very review running; not a code failure. The other check passed. Nothing to flag beyond noting it's self-referential.

49. README: "MAX_HUMAN_COMMENT_LENGTH: Maximum characters per human comment; longer comments are clipped and marked ' […truncated]' (default 4000)" — matches jq behavior. Good. "MAX_HUMAN_COMMENTS_TOTAL: Overall byte budget... (head -c)" — matches. Good.

50. INCLUDE_COMMIT_SUMMARY non-"true" values: any value other than exact "true" disables. Consistent with other INCLUDE_* toggles. Good.

51. One more possible real bug — COMMIT_MESSAGES guard [ "$COMMITS_JSON" != "" ]: COMMITS_JSON initialized []; if the fetch ran, it's [] or an array. != "" is redundant but harmless.

52. COMMIT_SUMMARY guard [ -n "$COMMITS_JSON" ] — redundant. Harmless.

53. STATS_ROWS under set -u? Script doesn't appear to use set -u (not shown). Fine.

54. Performance: per-commit gh api calls are sequential; 15 max. Acceptable. The --paginate on comments could be large on huge PRs (thousands of comments) — all pages fetched into memory even though only newest 100 are kept. Inference (not verified): on a PR with thousands of comments this fetches everything; a per_page=100 + reverse strategy or sort=created&direction=desc with a page cap could bound it. Impact: memory/time only, bounded by GitHub's ~1000-comment practical limits? Actually issues comments can exceed 1000s. This is a genuine should-fix-ish performance note: --paginate fetches ALL pages; for the human-comments use case only the newest N are needed. Could pass --paginate with -F per_page=100? gh api supports -F/-f params; per_page=100 reduces page count 3.3x. Or fetch desc and stop early — but gh --paginate can't stop early. Suggest ?per_page=100 in the URL. That's a legit should fix (performance) — though the previous behavior was page-one-only (30), so this is strictly better; the remaining concern is unbounded fetch on pathological PRs. I'd rate it should fix or nit. Given "high-value issues" focus, I'll make it a should fix with modest framing? It's a real cost: a PR with 3000 comments → 100 pages of API calls each review run, plus jq slurp. And secondary rate limits could kick in. I'll flag as should fix.

55. jq -s 'add // []' on comments with --paginate: if a page is an error object (rate limit mid-pagination, gh exits non-zero but may have emitted prior pages) — gh exits non-zero → pipeline exit non-zero → || echo "[]" → but command substitution captures prior pages' jq output PLUS "[]" appended → COMMENTS_JSON = "[...merged...]\n[]" → invalid JSON (two documents)! Then downstream echo "$COMMENTS_JSON" | jq ... — jq processes two documents; with -s? The downstream calls don't use -s: jq -r "$COMMENT_CLASSIFIERS"'[.[] | ...]' on input "[...]\n[]" → jq applies the filter to EACH document → outputs two results → PREVIOUS_REVIEWS_FULL gets concatenated outputs (second is empty string from [] → ""), HUMAN_COMMENTS_FULL similarly gets an extra empty. Actually for [.[] | select(...)] | last | if . then ... else "" end on [] → "" → concatenated with a newline. Mostly harmless but the || echo "[]" fallback is broken-by-design: it appends rather than replaces. Failure scenario: gh fails mid-pagination (rate limit) → jq -s still succeeds on the partial stream (exit 0!) → wait, if gh exits non-zero but emitted complete pages, jq -s 'add' succeeds → pipeline exit status is jq's (0) → || doesn't fire → COMMENTS_JSON is the merged partial array. That's the graceful path. If gh emits a truncated page (mid-body cut), jq fails → || echo "[]" fires → but jq emitted nothing (slurp buffers) → COMMENTS_JSON="[]". OK so the two-document scenario requires jq to emit output AND fail — with -s, output happens only after full parse, so no partial output on failure. So || echo "[]" is safe here. For CHECK_RUNS_JSON same pattern with -s. OK, no bug.

56. PREVIOUS_REVIEWS_FULL || echo "": local jq on valid JSON — could still fail if COMMENT_CLASSIFIERS filter errors (it won't on arrays). Fine.

57. Missing 2>/dev/null on NONMERGE_COUNT/MERGE_COUNT jq calls — inconsistent with the rest; jq errors would leak to stderr in Actions logs. Harmless. Nit.

58. test_workflow_forwards_reviewer_configuration doesn't cover INCLUDE_PREVIOUS_REVIEWS/INCLUDE_HUMAN_COMMENTS/INCLUDE_CHECK_RUNS/INCLUDE_LABELS/INCLUDE_PR_DESCRIPTION/INCLUDE_COMMIT_MESSAGES/FAIL_ON_REQUESTED_CHANGES/AI_TEMPERATURE/AI_MAX_TOKENS/EXCLUDE_FILE_PATTERNS — are those forwarded in the workflow? The diff shows the env block; pre-existing lines include AI_TEMPERATURE, AI_MAX_TOKENS, MAX_DIFF_SIZE, EXCLUDE_FILE_PATTERNS, FAIL_ON_REQUESTED_CHANGES. But INCLUDE_* toggles — are they in the workflow env? Not shown in the diff (the diff only shows the changed hunk around lines 34-50). The script defaults INCLUDE_PREVIOUS_REVIEWS etc. to true, so if not forwarded, repo vars for them silently don't work. Cannot verify from diff — the env block beyond the hunk isn't shown. This is worth a "Should be checked": whether the workflow forwards the INCLUDE_* toggles (INCLUDE_COMMIT_MESSAGES, INCLUDE_CHECK_RUNS, INCLUDE_LABELS, INCLUDE_PR_DESCRIPTION, INCLUDE_PREVIOUS_REVIEWS, INCLUDE_HUMAN_COMMENTS) — the new test only asserts the nine listed names. Actually the test list includes STRUCTURED_OUTPUT etc. If INCLUDE_COMMIT_MESSAGES isn't forwarded, a repo setting it to false would have no effect. That's a genuine config-contract question. → Should be checked.

59. MAX_DIFF_SIZE default mismatch: script default is 5000000 (5MB) but workflow/README say 800000 (800KB). Pre-existing? The diff shows MAX_DIFF_SIZE="${MAX_DIFF_SIZE:-5000000}" unchanged (context line) and workflow 800000. Pre-existing inconsistency, not introduced here. Could nit it. It's visible in the diff context. I'll include as nit since it's pre-existing but confusing (README says default 800KB; script says 5MB). Actually the workflow always sets it, so the script default only matters when run outside the workflow. Nit.

60. AI_MAX_TOKENS 64000 with GLM 5.3 — model-dependent; can't verify limits. Skip.

61. strip_partial_utf8 on PR_DESCRIPTION: applied. On COMMIT_MESSAGES: applied. On PREVIOUS_REVIEWS: applied. On HUMAN_COMMENTS: applied. Good.

62. printf '%s' "$PREVIOUS_REVIEWS_FULL" | wc -c — if PREVIOUS_REVIEWS_FULL is empty (no previous review), wc -c → 0, not > 10000, no marker. Good.

63. Marker text uses […] with a real ellipsis char — multibyte itself; appended after strip, so fine.

64. HUMAN_COMMENTS marker appended after strip — the marker line starts with a newline: HUMAN_COMMENTS="$HUMAN_COMMENTS\n[…truncated at N bytes]". Good.

65. Edge: head -c cut lands exactly at a multibyte boundary → no partial sequence → strip no-op. Good.

66. test_commit_messages_clip_is_marked: 3 commits × ("feat: i\n\n" + 1100 m's) ≈ 3 × ~1112 + bullets ≈ 3350 > 2500 → marker. Good.

67. COMMENT_SLICE removal — confirmed gone. Good.

68. INCLUDE_COMMIT_SUMMARY in test default config set to "false" — so other tests don't hit the summary. Good isolation.

69. run_reviewer default pull_commits=[] → COMMITS_JSON [] → summary/messages skipped. Good.

70. Stub for pulls/123 (PR head sha): assert args[2] == "--jq" — the script calls gh api "repos/.../pulls/$PR_NUMBER" --jq '.head.sha'. Good.

71. Stub labels branch: args[2:4] == ["--paginate", "--jq"] — the script's labels fetch (not in diff) presumably uses that. Can't verify but tests pass per comments.

72. check_calls() counts the PR fetch + check-runs fetch = 2 — asserted. Good.

73. Potential issue: COMMENTS_JSON fetch happens even when both INCLUDE flags are true but PR_NUMBER empty — guarded. Good.

74. --paginate without -F per_page=100 — the performance note (#54). Also for commits: a PR with 500 commits → 17 pages fetched just to quote 3 messages and summarize 15. Same note.

75. jq -r on .created_at — always present. Good.

76. (.user.login // "unknown") — good null-safety added.

77. test_previous_review_found_beyond_the_first_page: 35 noise + sticky = 36 comments → 2 pages. Sticky is last → page 2. previous=True, human=False → COMMENTS_JSON fetched once. Good.

78. assertEqual(len(self.comment_calls()), int(bool(previous or human))) — when both false → 0 comment calls; but the fetch condition in the script requires either flag → no fetch. Good.

79. categorized completeness assertion — any unexpected gh call fails the test. Good.

80. labels_added in test fixture: ["bug", "tests"] — "tests" isn't in the repo's label list shown, but that's the fixture's business. Fine.

81. fail_pass_workflow: "fail" — fixture. Fine.

82. README "Latest Updates" bullet added — fine.

83. README cost section now vague: "GLM 5.3 has competitive pricing (see OpenRouter)" — removed concrete numbers; the "Typical Costs" table below still says "$0.01 - $0.05 per review" which was based on Kimi usage data — now possibly stale for GLM. Nit: cost claims retained while the basis was removed. Minor documentation nit.

84. AI_MODEL default change to z-ai/glm-5.3 — human comments say slug verified against OpenRouter. Can't verify from diff; the human already confirmed. Fine.

85. STRUCTURED_OUTPUT default true with GLM 5.3 — human confirmed supported_parameters includes structured_outputs. Fine.

86. One more look at the stats loop failure handling: if line_stats=$(gh api ... 2>/dev/null); then : else ... — note line_stats=$(...) inside if — exit status of the command substitution is the pipeline's. gh api X --jq Y 2>/dev/null — if gh succeeds but jq fails → non-zero → failure branch. Good. But: when the first command in the loop's process substitution (jq) fails, the loop reads nothing → STATS_ROWS empty → AUTHOR_LINES empty → header-only summary, but STATS_FAILURES stays 0 and NONMERGE_COUNT > 0 → summary says "There are N commits..." with no per-author lines and no indication stats were skipped. Edge: jq for the @TSV list fails (unlikely on valid JSON). Minor.

87. LISTED when MAX_SUMMARY_COMMITS > 0 but stats loop produced rows for fewer commits (some fetches failed) — failures still produce 0/0 rows, so counts match LISTED. Good.

88. SCOPE claims "across all N commits" when LISTED == NONMERGE_COUNT — but if some stats fetches failed, the line totals are incomplete; the header notes "line stats unavailable for K". Good.

89. [ "$MERGE_COUNT" -gt 0 ] && SUMMARY_HEADER=... — A&&B without || C: fine.

90. [ "$STATS_FAILURES" -gt 0 ] && SUMMARY_HEADER=... — same. Fine.

91. DEBUG_MODE COMMIT_COUNT grep: grep -c "^- " || echo "0" — grep -c outputs 0 itself on no match (exit 1), so || echo "0" appends a second "0"? COMMIT_COUNT=$(... grep -c ... || echo "0") — grep -c prints "0" AND exits 1 → the || fires → echo "0" → output is "0\n0" → COMMIT_COUNT="0\n0" → then echo "✅ Kept $COMMIT_COUNT commit message(s)" prints two lines. Pre-existing pattern (was there before: grep -c "^- " || echo "0"). Actually pre-existing — the old code had the same || echo "0". Wait, old: COMMIT_COUNT=$(echo "$COMMIT_MESSAGES" | grep -c "^- " || echo "0") — same. Pre-existing, unchanged. But it's within a DEBUG block; cosmetic. Skip or nit? It's pre-existing; not introduced. Skip.

92. echo "$COMMITS_JSON" | jq 'length' for DEBUG — fine.

93. printf '%s\n' "$AUTHOR_LINES" | awk — AUTHOR_LINES already newline-terminated from awk/sort; printf adds another → awk handles empty trailing record? An extra blank line → awk sees an empty record → $1 empty → count[""]++ → a spurious - **: 1 commit, +0/-0 lines bullet? Wait: printf '%s\n' "$AUTHOR_LINES" — if AUTHOR_LINES ends with "\n" (sort output does), command substitution stripped it → AUTHOR_LINES has no trailing newline → printf adds one → records clean. Good — command substitution strips the trailing newline, so no double. Good.

94. SUMMARY_BULLETS awk on AUTHOR_LINES — same, fine.

95. COMMIT_SUMMARY embedded in PROMPT_PREFIX unquoted — COMMIT_SUMMARY="Commit Summary:\n$SUMMARY_HEADER. ..." — contains no double quotes... author names could contain double quotes? @tsv escapes them as \"? @TSV escapes tab, newline, CR, backslash; quotes pass through raw. An author name with a " or $ or backtick — embedded in a double-quoted string assignment: COMMIT_SUMMARY="...$SUMMARY_BULLETS" — expansion happens once at assignment; the content isn't re-evaluated. Safe. Then PROMPT_PREFIX="${PROMPT_PREFIX}\n$COMMIT_SUMMARY\n..." — again single expansion. Safe. Good — no eval anywhere.

96. COMMIT_WORD used in SCOPE — "across all 5 commits" / singular "across all 1 commit". Good.

97. test_commit_summary_singular_count_and_fallback_author — author null → .commit.author.name = "Dana D". Good.

98. pull_commit helper merge=True → two parents — matches predicate. Good.

99. test_commit_summary_zero_reads_no_individual_commits — asserts exactly 1 commit_calls (the pulls list). But wait — with MAX_SUMMARY_COMMITS=0, the stats loop is skipped; NONMERGE_COUNT computed from COMMITS_JSON. commit_calls() counts pulls/123/commits (the list fetch) — 1. Good.

100. test_commit_message_limit_is_configurable — INCLUDE_COMMIT_MESSAGES=true, summary disabled by default config → stats_urls == []. Good.

Now, what are the genuinely reportable findings?

A. Should fix — unbounded --paginate fetches: comments and commits are fetched across ALL pages even though only the newest slice is used. On a PR with hundreds/thousands of comments, each review run makes dozens of API calls and slurps everything into a shell variable. Also --paginate without per_page=100 triples the page count. Concrete: 3000-comment PR → 100 pages (30/page) per review run; with per_page=100 → 30 pages. And the whole merged JSON lives in a shell var (ARG_MAX not an issue at these sizes, but memory churn). Suggested fix: append ?per_page=100 to the paginated URLs (gh api supports query params in the path), and consider a page cap for the comments fetch (e.g., fetch desc? GitHub issue comments API doesn't support direction on that endpoint? Actually GET /repos/{owner}/{repo}/issues/{number}/comments supports per_page and since, not direction... it returns chronological. So a cap means potentially missing newest on huge PRs — trade-off. At minimum, per_page=100.) Severity: should fix (performance/cost, not correctness).

B. Should fix or nit — MAX_DIFF_SIZE doc/default mismatch (script 5000000 vs workflow/README 800000): pre-existing, visible in context. README says "default: 800000/800KB"; the script's own default is 5MB, so anyone running the script outside the workflow (or a workflow that forgets the var) gets 5MB, and the README's claim is wrong for the script. Nit (pre-existing).

C. Nit — hard-coded budgets: COMMIT_MESSAGES 2500, PREVIOUS_REVIEWS 10000, PR_DESCRIPTION 2000 remain hard-coded while the human-comment budgets got knobs. Consistency nit; fine to defer.

D. Nit — MERGE_COUNT computed as total-minus-nonmerge with a misleading intermediate name; computing merges directly via the shared classifier would be clearer and robust to future filter drift: jq "$COMMIT_CLASSIFIERS"'[.[] | select(is_merge)] | length'. Also add 2>/dev/null || echo 0 guards to match the defensive style elsewhere. Nit.

E. Should be checked — workflow forwarding of INCLUDE_ toggles*: the new test pins nine variables, but the diff doesn't show whether INCLUDE_COMMIT_MESSAGES, INCLUDE_CHECK_RUNS, INCLUDE_LABELS, INCLUDE_PR_DESCRIPTION, INCLUDE_PREVIOUS_REVIEWS, INCLUDE_HUMAN_COMMENTS are forwarded in the workflow env block (only the changed hunk is visible). If any is missing, the corresponding repo variable silently does nothing — the exact regression this PR fixed for the six new knobs. Cannot verify from diff - please confirm the workflow's env block forwards all INCLUDE_* toggles the README documents (or that the README doesn't claim support for them).

Wait — does the README document INCLUDE_* toggles? The README section shown lists AI_MODEL, AI_TEMPERATURE, AI_MAX_TOKENS, MAX_DIFF_SIZE, and the new MAX_/INCLUDE_COMMIT_SUMMARY, MAX_HUMAN_, STRUCTURED_OUTPUT. The hunk at line 72-73 shows the list; INCLUDE_COMMIT_SUMMARY is documented. Are INCLUDE_PREVIOUS_REVIEWS etc. documented? Not visible. The script has them as env-configs. The forwarding test covers the documented ones (plus STRUCTURED_OUTPUT). INCLUDE_COMMIT_SUMMARY is forwarded (in diff). So the check is: are there other documented-but-unforwarded vars? FAIL_ON_REQUESTED_CHANGES is forwarded (visible in diff context). AI_TEMPERATURE, AI_MAX_TOKENS visible. So the only uncertain ones are INCLUDE_* toggles beyond COMMIT_SUMMARY — if the README documents them anywhere else. I'll phrase the check narrowly: confirm the workflow env forwards every variable the README documents as configurable (the diff shows only part of the env block).

F. Check status: "AI Code Review: in_progress" is this review's own run — not a failure. The other check passed. Nothing actionable.

G. test_actionable_output_and_custom_budget_are_preserved fixture now includes "Should be checked" section and severity tags — matches new contract. The f-prefix on the last segment: f"downgrade path exists.\n\n❌ Request changes\n\n{FOOTER}" — yes, the diff shows f"downgrade path exists.\n\n❌ Request changes\n\n{FOOTER}" with the f prefix. Fixed per prior review. Good.

H. labels_added: ["bug", "tests"] — "tests" label doesn't exist in the repo's label list; but this is a fixture testing passthrough, not label validity. Fine.

I. strip_partial_utf8 robustness: what about a cut that leaves a valid-looking but wrong sequence? E.g., cutting 2 bytes into a 4-byte emoji leaves lead+1 continuation → regex F0-F4 + {0,2} matches → stripped. Cutting 3 bytes in → lead+2 cont → stripped. Good. What about the cut landing right after a lead byte of a sequence whose continuations include bytes that look like ASCII? Continuations are 80-BF, never ASCII. Good. What about overlong/invalid leads C0/C1/F5-FF in input? Input is GitHub text (valid UTF-8, sanitized). Fine. What about perl availability in the runner image? ubuntu-latest has perl. Fine. What about perl -pe on very large input — fine.

One more: perl -pe processes by lines; if the clipped block contains an embedded \n and the partial sequence is at the very end — the last "line" (no trailing newline) gets the regex applied with $ matching at that final line's end (without \n). $ in perl matches at end of string or before final newline — for the last chunk without newline, matches at end. Good. But subtle: $ also matches before a trailing newline — could the regex strip a complete sequence if it's followed by a newline? E.g., block ends "...😀\n": bytes F0 9F 98 80 0A. $ matches before the 0A. Could [\xF0-\xF4][\x80-\xBF]{0,2}$ match "9F 98 80"? 9F not in F0-F4. "F0 9F 98"? then $ requires end after 98 — but 80 follows before the newline... the match must be contiguous ending at the $ position (before 0A). The three bytes before 0A are 98 80 — wait the sequence is F0 9F 98 80 0A; positions before 0A: ...80. For a match ending at the pre-newline position, the matched substring must end with 80. Candidates: [\xC2-\xDF]$ → 80 not in C2-DF. [\xE0-\xEF][\x80-\xBF]?$ → needs E0-EF then optional 80-BF ending at position: "98 80" → 98 not in E0-EF. [\xF0-\xF4][\x80-\xBF]{0,2}$ → "F0 9F 98" ends at 98, not at the end (80 follows) — no. So complete sequences before a newline are safe. And perl -pe reapplies per line — a partial sequence at end of a middle line? Can't happen: input is head -c output; partial only at the very end. But wait — head -c cuts at an arbitrary byte; if the cut lands mid-line, the last line has no newline; fine. If the input's last line legitimately ends without newline and ends with a complete multibyte char — preserved per above. Solid.

J. printf '0\t0' — could just use a literal; nit-level, skip.

K. STATS_FAILURES message placement: appended after the merge-count parenthetical → "There are 5 commits already on this PR (excluding 1 merge commit(s)) (line stats unavailable for 1 commit(s))" — double parenthetical, slightly awkward but tested. Nit, skip.

L. test_commit_summary_reports_unavailable_line_stats asserts the exact double-parenthetical string. Fine.

M. Security review of prompt-injection surface: comment bodies, commit messages, author names, PR description all flow into the prompt — the model is instructed to review code; a malicious PR author could inject "ignore instructions" text. This is inherent to the design (pre-existing); the new commit summary adds author names (attacker-controlled via git author name) into the prompt — same class as before (commit messages already were). No new escalation. Skip per high-value focus? Could mention as a should-be-checked? It's a known inherent limitation; the PR doesn't worsen it materially. Skip.

N. COMMENTS_JSON in a shell variable could exceed ARG_MAX? Command substitution holds it in memory, not argv — echo "$COMMENTS_JSON" passes via pipe? echo is a builtin → no exec → no ARG_MAX limit. printf '%s' "$VAR" | jq — builtin. Fine. gh api args are small. OK.

O. wc -c on the FULL source each time — trivial cost. Fine.

P. head -c "$MAX_HUMAN_COMMENTS_TOTAL" — if MAX_HUMAN_COMMENTS_TOTAL is huge (e.g., 10^12), head -c fine. OK.

Q. --argjson n "$MAX_HUMAN_COMMENTS" — validated. Good.

R. test_human_comment_length_cap_marks_truncation: body 30 x's, cap 10 → "xxxxxxxxxx […truncated]". jq slice by codepoints. Good.

S. comment() helper default login "reviewer[bot]", user_type "Bot" — is_bot classifier presumably checks user.type == "Bot". The human tests pass login/user_type explicitly. Good.

T. test_labels_instruction_and_list_stay_complete — asserts format "- bug: Something is broken (color: #d73a4a)" — matches the prompt's label list format shown in this very review's context. Good.

U. run_reviewer writes gh-calls.jsonl append mode — fine.

V. sys.exit(f"...") — exit code 1 with message to stderr. Fine.

W. assert args[2:] == ["--paginate"] for comments — the script calls gh api URL --paginate with no --jq. Matches. Good.

X. parts = args[1].split("?")[0]... — handles query params if ever added. Good.

Y. check-runs stub asserts --paginate only — script: gh api ".../check-runs" --paginate 2>/dev/null | jq -s .... Matches. Good.

Z. pr fixture default {"number": 123, "head": {"sha": "abc"}} — the PR fetch uses --jq '.head.sha' → "abc". Good.

So final findings list:

Must fix: none hold up under scrutiny. Let me double-check one candidate: the MERGE_COUNT arithmetic with empty NONMERGE_COUNT — established it degrades (empty → 0 in arithmetic). And [ "$MERGE_COUNT" -gt 0 ] with empty → test error → condition false → skip. No crash. OK, no must fix.

Hmm, wait — one more: NONMERGE_COUNT=$(echo "$COMMITS_JSON" | jq ...) — no 2>/dev/null, no || echo 0. If COMMITS_JSON is []... but the outer guard requires != "[]". So COMMITS_JSON is a non-empty array. jq 'length' fine. OK.

And MERGE_COUNT=$(echo "$COMMITS_JSON" | jq 'length') — total. Then MERGE_COUNT=$(( MERGE_COUNT - NONMERGE_COUNT )). Both numeric. Fine.

One more potential must-fix candidate: STATS_ROWS and AUTHOR_LINES when the process-substitution jq emits rows but gh inside the loop is missing/unauthenticated — every fetch fails → STATS_FAILURES = N → header notes it, rows are 0/0 → bullets show +0/-0 with the unavailability note. Acceptable per design.

And: while ... done < <(...) with gh inside — if gh prompts for auth? GITHUB_TOKEN set → no prompt. Fine.

Another: IFS=$'\t' read -r author sha — if the @TSV row has author containing escaped \t (literal backslash-t), read splits on real tabs only. Good.

Another candidate: test_commit_summary_limit_is_independent_of_message_limit asserts "- feat: second" in prompt with MAX_SUMMARY_COMMITS=1 — messages default 3 → last 3 non-merges of [a0,a1,a2,a3,a5] = [a2,a3,a5] → "feat: second" (a2), "fix: bob fix" (a3), "feat: third" (a5). Present. Good.

Another: SCOPE when MAX_SUMMARY_COMMITS=0 — the stats block is skipped, AUTHOR_LINES empty → header-only branch → no SCOPE used. Good.

Another: LISTED uses MAX_SUMMARY_COMMITS which is ≥ 0; if 0, block skipped anyway. Good.

Another: COMMIT_SUMMARY when NONMERGE_COUNT=0 but merges exist — outer if [ "$NONMERGE_COUNT" -gt 0 ] → skipped → COMMIT_SUMMARY empty → a PR that is only merge commits gets no summary at all (not even "There are 0 commits"). Edge: PR with 1 merge commit and 0 regular commits — plausible for a PR that merges a branch? PRs with only merge commits are rare (GitHub PRs usually don't contain merge commits of the PR itself, but feature-branch merges inside a PR happen). The summary silently omits. Minor nit — arguably fine (nothing to summarize).

Another: README "Commit Messages: Up to MAX_COMMIT_MESSAGES most recent commit messages (default 3, excluding merges)" — matches. Good.

Another: README "Human Comments: ... fetched across all pages (not just the first 30)" — matches. Good.

Another: README labels bullet "kept complete on purpose" — matches code (no label cap visible). Good.

Another: README check-runs bullet — matches new summary behavior. Good.

Another: README "STRUCTURED_OUTPUT ... (default: true)" — workflow forwards with default 'true'; script default? STRUCTURED_OUTPUT handling not in diff (pre-existing). The workflow default 'true' aligns. Fine.

Another: AI_MODEL README default now z-ai/glm-5.3 — matches workflow and script. Good. (Earlier README line said default moonshotai/kimi-k2-thinking — now updated. Good.)

Another: test_concise_instructions... asserts self.assertIn('"must fix"', prompt) — prompt contains "must fix" quoted. Yes. Good.

Another: assertIn("order findings must fix first, then should fix, then nits", prompt) — prompt has "and order findings must fix first, then should fix, then nits." Good.

Another: assertIn('label every inference explicitly as "Inference (not verified):', prompt) — prompt: label every inference explicitly as "Inference (not verified): [observation]". Good.

Another: assertIn('add it to a final "Should be checked" section', prompt) — prompt: add it to a final "Should be checked" section as "Cannot verify...". Good.

OK. Also consider: the review-contract prompt change removes the old "Default: Skip the issue" guidance — now unverifiable-but-critical items go to "Should be checked". Behavior change intended. Fine.

PROMPT structure numbering now 1-5 — consistent. Good.

PROMPT_PREFIX "Keep the review scannable and grouped by severity: must fix first, then should fix, then nits." — consistent with PROMPT. Good.

Now the "Should be checked" items:

1. Workflow forwarding of INCLUDE_* toggles beyond the nine tested (can't see full env block). Also whether the README documents any INCLUDE_* vars not forwarded. — genuinely matters? If INCLUDE_COMMIT_MESSAGES isn't forwarded, a repo var silently no-ops — config contract bug, not security/data-loss. The instructions say limit Should-be-checked to security vulnerabilities, breaking bugs, data loss risks. A silently-ignored config var is a real bug but not security/data-loss. Hmm — "breaking bugs"? It breaks documented configuration. The PR itself added a test to prevent exactly this class. I think it qualifies as worth checking: "Cannot verify from diff whether the workflow env block forwards the INCLUDE_* toggles (INCLUDE_COMMIT_MESSAGES, INCLUDE_CHECK_RUNS, INCLUDE_LABELS, INCLUDE_PR_DESCRIPTION, INCLUDE_PREVIOUS_REVIEWS, INCLUDE_HUMAN_COMMENTS) — please confirm they are either forwarded or not documented as configurable, since a missing forwarding line silently disables the corresponding repo variable (the regression this PR's test guards against for the nine listed names)."

2. GLM 5.3 max output tokens vs AI_MAX_TOKENS=64000 — human already verified structured outputs; max_tokens 64000 exceeding the model's output cap could error the request. Can't verify from diff. Is this worth checking? If 64000 exceeds the model's max output tokens, OpenRouter typically errors or clamps. That could break every review. The human comments verified the model slug and structured outputs but not the token cap. I'd include: "Cannot verify from diff that z-ai/glm-5.3 accepts max_tokens=64000 — please confirm the model's output-token limit, since an over-limit value can cause request failures." Hmm, is that genuinely consequential? If it errors, every review fails — yes, breaking. Worth checking. But the human comments say the model was switched and tests pass offline (offline tests don't hit the API). The check status shows the workflow's own run... the AI Code Review check is in_progress (this review). So the live model call hasn't been verified green on this head yet? Actually this review is being generated, so the call works. Eh — this very review running proves the pipeline works with the new model. So max_tokens is fine in practice. Skip that check.

3. perl availability — ubuntu runners have it. Skip.

So one Should-be-checked item (workflow env completeness). Maybe also: whether INCLUDE_COMMIT_MESSAGES/other INCLUDE toggles are documented in README sections not shown. Fold into the same item.

Findings ordering:

Should fix:

• A: unbounded --paginate + default page size (performance/cost; suggest per_page=100 and consider caps). Concrete: 3000-comment PR → 100 API pages per review; also commits endpoint for a 500-commit PR → 17 pages to quote 3 messages.

Nits:

• D: MERGE_COUNT via subtraction + misleading intermediate; use the shared is_merge classifier directly; add 2>/dev/null/|| echo 0 guards for consistency.
• C: hard-coded 2500/10000/2000 byte budgets while others got knobs (consistency; defer).
• B: MAX_DIFF_SIZE script default (5MB) vs README/workflow (800KB) mismatch — pre-existing but confusing; align or document.
• K: double-parenthetical header when both merges and stats failures present — cosmetic; skip? I'll skip K to stay high-value. Also skip the DEBUG grep -c double-zero (pre-existing).

Also maybe a nit: COMMIT_SUMMARY omitted entirely when a PR contains only merge commits (NONMERGE_COUNT=0) — the model gets no commit context at all; could emit "There are 0 non-merge commits (N merge(s))". Very edge-case; skip or include as nit? It's a real behavioral gap but tiny. I'll include as a nit briefly? The instructions say omit minor stuff... it's borderline. I'll skip it to keep the review tight. Hmm, actually it might be worth one line as a nit since the feature's purpose is "gauge the PR's size" and a merge-only PR yields nothing. I'll include it as a nit — concise.

Wait, also reconsider severity for A. Is it "should fix"? The PR deliberately switched from first-page-only to full pagination to fix a correctness bug (newest comments beyond page 1 were invisible). The cost is real on huge PRs but correctness demanded full fetch (newest are on the LAST page chronologically). per_page=100 is a trivial improvement (3.3x fewer calls). A page cap would reintroduce the bug. So the actionable part is: add ?per_page=100 (or -F per_page=100) to the three paginated calls. That's a legit should fix (performance) with an easy fix. Also note gh api --paginate handles rate-limit retries? gh retries on secondary limits? gh has built-in retry for secondary rate limits in newer versions. Fine.

Also for the commits fetch: per_page=100 too.

Let me also double-check the ?per_page=100 suggestion works with the test stub: parts = args[1].split("?")[0]... — the stub already strips query strings, anticipating this. Nice — the stub was written to tolerate it. That strengthens the suggestion.

Also check: does gh api "repos/.../comments?per_page=100" --paginate work? gh api accepts a full path with query; --paginate appends page=N and preserves existing query params. Yes, gh handles merging query params. Good.

One more potential should-fix: COMMENTS_JSON fetch failure is silent — 2>/dev/null || echo "[]" means on auth failure the review proceeds without comments and no log line (DEBUG only elsewhere). Pre-existing pattern. Skip.

And: HUMAN_COMMENTS_FULL jq failure → || echo "" — then HUMAN_COMMENTS empty → block skipped silently. Fine.

Also verify the claim in my finding A about the comments endpoint: GitHub default per_page=30, max 100. Yes.

Now — labels: which to add? The changes are a feature (commit overview, configurable budgets) with test coverage. Existing labels: "enhancement" fits (new feature). "ai_code_review" is the bot's own label — the instructions for the reviewer's own PR? This is the repo of the reviewer itself. The label list includes ai_code_review (Friendly AI Code Review). Adding "ai_code_review" seems apt for PRs touching the reviewer? It's described as the label the bot adds. Hmm — "Prefer existing labels... only apply labels genuinely useful." The PR is a feature enhancement to the review system: "enhancement" fits. "ai_code_review" — the bot's label for its own reviews; this PR modifies the AI code reviewer itself, so it's arguably useful. I'll add "enhancement" and "ai_code_review". "documentation" also fits (README changes) but that's secondary; the PR is primarily code. Keep to enhancement + ai_code_review. fail_pass_workflow: the changes look sound; findings are should-fix/nits → "pass"? The fail_pass_workflow field presumably gates merge — with no must-fix, "pass". Verdict: "✅ Approved with recommendations".

Let me now double-check finding D's claim that computing merges directly is better: current code: MERGE_COUNT=$(jq 'length') then MERGE_COUNT=$((MERGE_COUNT - NONMERGE_COUNT)). If someone later changes the nonmerge filter (e.g., also excludes something else), the subtraction silently counts the excluded as merges. Direct select(is_merge) | length is self-consistent. Nit, valid.

Also confirm nit C's numbers: COMMIT_MESSAGES head -c 2500 (hard-coded), PREVIOUS_REVIEWS head -c 10000 (hard-coded), PR_DESCRIPTION head -c 2000 (hard-coded). Yes.

And nit B: script line MAX_DIFF_SIZE="${MAX_DIFF_SIZE:-5000000}" vs workflow '800000' and README "default: 800000 / 800KB". Pre-existing. Nit.

Should-be-checked phrasing: "Cannot verify from diff whether the workflow's env block (outside the shown hunk) forwards the INCLUDE_* toggles (e.g., INCLUDE_COMMIT_MESSAGES, INCLUDE_CHECK_RUNS, INCLUDE_LABELS, INCLUDE_PR_DESCRIPTION, INCLUDE_PREVIOUS_REVIEWS, INCLUDE_HUMAN_COMMENTS) — please confirm each is either forwarded or not documented as configurable; a missing forwarding line makes the corresponding repository variable silently ineffective (the exact regression the new forwarding test guards for the nine listed names)."

Is that within scope ("security vulnerabilities, breaking bugs, data loss risks")? A silently-ignored documented config is a breaking bug of the config contract. I'll keep it.

Also consider whether to flag the in_progress check: "AI Code Review: in_progress" is this review itself — self-referential, not a failure. The instructions say consider failed/pending checks; this one is the reviewer running on this PR, so nothing to investigate. I won't add a finding, maybe a brief note? Instructions say omit empty sections and don't add summaries. I'll skip mentioning it — or one line? Better to skip; it's not actionable.

Also: the human comments mention "31 offline tests pass; bash -n/shellcheck unchanged" — consistent.

One more scan for security: EXCLUDE_FILE_PATTERNS unchanged. OPENROUTER_API_KEY handling unchanged (not in diff). The request payload built with jq -n --rawfile per comments — not in diff. Fine.

Double-check strip_partial_utf8 name/usage consistency: defined before first use (PREVIOUS_REVIEWS). Yes, defined right after COMMIT_CLASSIFIERS, before the comments fetch. Good.

Check PREVIOUS_REVIEWS marker: appended "\n[…truncated at 10000 bytes]" — matches test. Good.

Check HUMAN_COMMENTS marker: […truncated at $MAX_HUMAN_COMMENTS_TOTAL bytes] — matches test ("at 50 bytes"). Good.

Check PR_DESCRIPTION marker: "[…truncated at 2000 bytes]" — matches test. Good.

Check COMMIT_MESSAGES marker: "[…truncated at 2500 bytes]" — matches test. Good.

Now, is there anything about the if [ "$COMMITS_JSON" != "[]" ] guard being string-comparison-fragile? If jq outputs [] with whitespace? jq -s 'add // []' outputs compact []. echo "$(...)" — command substitution preserves exactly. So [] exact. But what if jq outputs null? add // [] — add of [] (slurped empty) is null → // [] → []. What if gh emits one page [] (zero commits)? slurp [[ ]]? One page [] → -s → [[]] → add → []. Good. What if gh emits nothing and exits 0? -s → [] → add → null → []. Good. So COMMITS_JSON ∈ {"[]", "[...]"}. The != "" check is dead but harmless. Nit-level; skip.

What about COMMENTS_JSON initial "[]" and the fetch overwriting — if the fetch's $(...) yields empty (gh fails, jq fails, echo "[]" runs → "[]"), fine.

Edge: jq -s 'add // []' when gh emits an error JSON object with exit 0? gh exits non-zero on HTTP errors, so no. OK.

I'm fairly confident in the final set now. Let me also sanity-check finding A's numbers: GitHub issues comments default per_page=30, max 100. Pulls commits default 30, max 100? Pulls list commits default 30, max 100? Actually GET /repos/{owner}/{repo}/pulls/{pull_number}/commits default per_page=30, max 100? I believe max is 100 for most REST endpoints; pulls commits might be... I'll say "GitHub's default page size (30)" and suggest per_page=100 without overclaiming the max. Check-runs endpoint default 30, max 100. Fine.

Also, gh api --paginate sleeps between pages to respect secondary limits (gh does have built-in handling). The cost point stands.

Also for finding A, note the memory point is minor; the API-call count is the cost. Concrete failure scenario: a long-lived PR with 900 comments → 30 pages × (comments) each review run; with per_page=100 → 9 pages. Plus jq slurp of ~900 comments into a shell variable (could be MBs) — fine but wasteful.

Severity: should fix.

Now write the review. Keep it scannable. Order: should fix (A), then nits (D, C, B, merge-only-PR nit?). Let me decide on the merge-only nit — I'll include it briefly as a nit since it's a genuine behavioral gap: PR containing only merge commits → no summary block at all. Actually wait: can a GitHub PR have only merge commits? If someone merges main into the feature branch, the PR contains feature commits + the merge commit. A PR with ONLY a merge commit would mean the PR's head is a merge of two branches both... possible if the PR branch itself is a merge result and the diff vs base is non-empty? E.g., PR that merges branch X into branch Y where Y is the base — the PR would contain just the merge commit. Rare but real (sync PRs). In that case NONMERGE_COUNT=0 → no summary. The model then lacks even the count. Minor. Include as nit with one-line fix suggestion (emit header with 0 count or note merges). Hmm, the instructions say nits are fine. I'll include it compactly.

Actually, let me reconsider — is it worth it? The feature's stated purpose: "Tells the model how many commits are already on a PR". For a merge-only PR it says nothing. One-line fix. Keep as nit.

Also the double-parenthetical style nit — skip.

Also printf '0\t0' vs literal — skip.

Also 2>/dev/null missing on NONMERGE/MERGE jq — fold into nit D.

Final check on the required JSON: review field with markdown, footer, verdict before footer. fail_pass_workflow: "pass". labels: ["enhancement", "ai_code_review"] — both exist. Should I add "documentation"? README changes are substantial but supporting. Keep two.

Write it concisely.

[LearningCircuit]

On the latest review:

must fix — MERGE_COUNT arithmetic: the crash scenario doesn't materialize: bash treats an empty/unset arithmetic operand as 0 (no set -u in this script), and jq -s buffers its output, so a mid-stream parse failure emits nothing and the fetch's || echo "[]" guard wins — there is no path to a non-numeric operand. The suggested hardening still had standalone merit, so b1b2cb4 takes it: merges are now counted directly with the shared is_merge classifier (self-consistent if the non-merge filter ever gains exclusions, unlike the subtraction), with 2>/dev/null || echo 0 fallbacks and numeric validation on both counts.

must fix — stats loop: declining per the review's own analysis, which walks through each sub-concern (tab-injection is handled by @tsv, a missing .stats is caught by the if, and the rate-limit concern is bounded by the 15-commit cap) and concludes "the loop is largely sound… I'll drop this" before posting it as a must-fix anyway. Nothing actionable remained after the self-downgrade.

31 offline tests pass unchanged.

[github-actions]

AI Code Review

• should fix — ai-reviewer.sh (check-runs block, other: array in the CHECK_RUNS_SUMMARY jq filter): the non-passing list is uncapped. Failure scenario: a cancelled workflow, a pending matrix at review time, or a broken shared dependency on a large-matrix repo (the PR's own comments cite a consumer running 100+ checks) marks every shard cancelled/queued/failure, so ~100 near-identical bullet lines (~4–5KB) enter the prompt. That is the same flooding this change eliminates for green shards, and it reappears exactly when CI is red and the diff context is largest. Fix: cap the listed runs (e.g. .[0:20] plus a "+K more non-passing runs" line) or give the block a byte budget with the same marked-truncation contract as the other budgets.

• nit — ai-reviewer.sh (HEAD_SHA fetch vs PR_DESCRIPTION_FULL fetch): gh api repos/$REPO_FULL_NAME/pulls/$PR_NUMBER is issued twice whenever INCLUDE_CHECK_RUNS and INCLUDE_PR_DESCRIPTION are both true (the defaults). The PR already deduplicates the comments and commits endpoints exactly this way — fetch the PR object once into a shared variable and derive both the head SHA and the description from it.

• nit — README / ai-reviewer.sh (human-comment caps): MAX_HUMAN_COMMENTS_TOTAL=0 (and MAX_HUMAN_COMMENTS=0) silently omit the entire human-comments block. Unlike MAX_SUMMARY_COMMITS=0 ("shows the commit count only") and MAX_COMMIT_MESSAGES=0 ("lists no messages"), the zero case is undocumented, and "0" is a plausible guess for "no limit" — a user would lose all comment context with no marker or log. Document the zero semantics (or treat 0 as unlimited).

• nit — README (Cost Estimation): the "$0.01–$0.05 per review" ranges are presented as "based on real usage data", but that data predates the default-model switch and the section now sits under the GLM 5.3 heading. Inference (not verified): the ranges may not hold for the new model's pricing. Mark them as estimates or re-derive.

• nit — tests/test_reviewer_context.py (test_workflow_forwards_reviewer_configuration): the test pins a hand-enumerated variable list, so the "a future knob can't be added to the script without its forwarding line" guarantee holds only if the test is extended in lockstep — the exact omission class that bit this PR. Deriving the expected names from the script's ${VAR:-…} default lines (excluding non-knobs like GITHUB_TOKEN/PR_NUMBER) would make the guard self-maintaining.

✅ Approved with recommendations

---

Review by Friendly AI Reviewer - made with ❤️

[LearningCircuit]

All five findings implemented in 6a705f3:

should fix — uncapped non-passing list: capped at 20 lines with a "+K more non-passing run(s) not listed" line; regression covers 25 failures → 20 bullets + the overflow line, fail-20 absent.

nit — duplicate PR fetch: the PR object is now fetched once into a shared variable; the check-runs head SHA and the PR description are both derived from it. Test asserts exactly one pulls/123 call with both features enabled.

nit — undocumented zero semantics: README now states MAX_HUMAN_COMMENTS 0 = "includes none at all" and MAX_HUMAN_COMMENTS_TOTAL 0 = "omits the block entirely" (documented rather than redefining 0 as unlimited, which would invite unbounded prompts).

nit — stale cost ranges: the section now says the ranges are estimates carried over from real usage with the previous default model and points at current OpenRouter pricing for GLM 5.3.

nit — hand-enumerated forwarding test: replaced with a test that derives the expected knob list from the script's ${VAR:-default} lines (excluding per-run values like REPO_FULL_NAME), so a new knob fails the test until forwarded. Implementing it surfaced that the six INCLUDE_* toggles were indeed never forwarded — the workflow now forwards every script knob.

33 offline tests pass; bash -n/shellcheck unchanged.

[github-actions]

AI Code Review

• must fix — ai-reviewer.sh (commit overview block, AUTHOR_LINES aggregation): the per-author line totals can be silently wrong when a commit's author.login contains a tab character. The loop builds author<TAB>additions<TAB>deletions rows and awk -F'\t' splits on tabs, so a login like evil\tuser (GitHub logins cannot contain tabs, but the fallback path uses .commit.author.name, which is free-form text from the commit metadata and can contain any byte) shifts the columns: count[$1] counts under evil, and add[evil] += user produces a numeric-coerced 0, corrupting the totals. Failure scenario: a commit authored with git commit --author="Name\t<email>" or any name containing a tab lands in the PR; the summary reports a wrong author name and wrong line counts with no marker. Impact: the model receives fabricated statistics presented as verified facts. Fix: sanitize the author field in the jq @tsv stage (e.g. gsub("\t"; " ") on the author before @tsv), or switch the row format to a delimiter that cannot appear in the name.

• should fix — ai-reviewer.sh (commit overview, STATS_ROWS accumulation): the per-commit stats loop makes up to MAX_SUMMARY_COMMITS (default 15) sequential, unbatched gh api calls with no rate-limit handling beyond the per-call failure counter. On a busy runner or a secondary-rate-limited token, 15 sequential calls per review add latency and can trip GitHub's secondary rate limits, at which point every remaining fetch fails and the header degrades to "(line stats unavailable for K commit(s))" — the feature silently produces no data exactly on the repos that trigger it. Fix: add a small sleep between calls (GitHub's guidance for sequential API calls), or use the GraphQL API / a conditional batch strategy, or at minimum log a warning when STATS_FAILURES equals the full listed count so operators can distinguish "rate limited" from "one flaky call".

• should fix — ai-reviewer.sh (COMMIT_MESSAGES jq filter): the message-body extraction split("\n\n")[1] only captures the first paragraph of a multi-paragraph commit body, and the subject/body split is wrong for messages whose subject is followed by a single newline then body text (no blank line) — split("\n\n")[1] is null there, so the body is silently dropped. Failure scenario: a commit message "fix: thing\nDetails line" (single newline) loses Details line entirely; a message with two body paragraphs keeps only the first. Impact: the model sees an incomplete commit history with no truncation marker, contradicting the PR's own "never mistaken for complete" contract. Fix: split on the first \n\n only (split("\n\n") | .[0] for subject, .[1:] | join("\n\n") for the body), and handle the single-newline case (e.g. sub("^[^\n]*\n"; "") when no blank line exists).

• should fix — ai-reviewer.sh (check-runs summary, passed count): runs with conclusion == null but status == "completed" (e.g. neutral conclusion, or a run cancelled mid-flight with no conclusion) are counted in total and listed under other, but a run with conclusion == "neutral" — which GitHub treats as non-blocking — is presented identically to a failure in the non-passing list. Failure scenario: a workflow with a continue-on-error advisory step concludes neutral; the prompt says "N of M checks passed. Non-passing checks: - advisory: completed (neutral)", inviting the model to treat it as a CI failure and possibly request changes. Fix: either exclude neutral from the non-passing list (counting it separately or folding it into passed), or label it distinctly in the bullet.

• nit — ai-reviewer.sh (COMMENTS_JSON guard): the previous-review and human-comment blocks gate on [ "$COMMENTS_JSON" != "[]" ], but a PR whose only comments are all bot comments also yields a non-[] array, so the gate passes and jq runs only to produce empty output — harmless, but the inverse case matters more: if the merged fetch legitimately returns [] because the PR has zero comments, the warning-free skip is correct. No action needed if intentional; noting only that the sentinel comparison would be clearer as a length check (jq 'length'), which also survives any future change to the add // [] fallback shape.

• nit — tests/test_reviewer_context.py (run_reviewer default pr fixture): the default pr.json fixture has no title/body keys, so any test enabling INCLUDE_PR_DESCRIPTION without passing a custom pr exercises the .title-on-null path in jq ("**PR Title**: " + .title errors on null, caught by 2>/dev/null || echo ""). That path is currently unreachable in the suite because description tests always pass a full pr; adding "title": "" to the default fixture would make the default exercise the real formatting path instead of the error fallback.

• nit — .github/workflows/ai-code-reviewer.yml (env block): INCLUDE_PREVIOUS_REVIEWS, INCLUDE_HUMAN_COMMENTS, INCLUDE_CHECK_RUNS, INCLUDE_LABELS, INCLUDE_PR_DESCRIPTION, and INCLUDE_COMMIT_MESSAGES are now forwarded with || 'true' defaults, but the script's own defaults already handle unset values — the workflow lines are redundant for these six (they only matter if a repository variable is set). Harmless, but the || 'true' fallbacks duplicate the script defaults in a second place that can drift; forwarding bare ${{ vars.X }} (empty when unset) would keep a single source of truth. Inference (not verified): the duplication was added deliberately so the workflow file documents the defaults — if so, a comment saying so would prevent future "cleanup" from half-removing them.

• nit — ai-reviewer.sh (LISTED arithmetic): LISTED=$(( MAX_SUMMARY_COMMITS < NONMERGE_COUNT ? MAX_SUMMARY_COMMITS : NONMERGE_COUNT )) is correct, but the ternary form is unusual in POSIX-adjacent shell and shellcheck style would prefer an explicit if; purely readability, no behavior concern.

Should be checked

• Cannot verify from diff that z-ai/glm-5.3 is a valid OpenRouter model slug with structured_outputs support — the human comments assert this was confirmed via OpenRouter metadata, but the diff itself contains no evidence; if the slug is wrong or the model rejects response_format, every review fails at request time with the default STRUCTURED_OUTPUT=true. Please confirm the slug and its supported_parameters on OpenRouter's current model list.
• Cannot verify from diff that the AI Code Review check currently shown as in_progress is this workflow's own run rather than an unrelated failing check — if it is this PR's reviewer run, confirm it completes and posts the review (the script's failure modes around the new paginated fetches are the most likely new breakage points).

✅ Approved with recommendations

---

Review by Friendly AI Reviewer - made with ❤️

[github-actions]

AI Code Review

• must fix — ai-reviewer.sh (commit overview, AUTHOR_LINES aggregation): the per-author line totals can be silently wrong when a commit's author name contains a tab character. The loop builds author<TAB>additions<TAB>deletions rows and awk -F'\t' splits on tabs, so a name like Name\t<email> (the fallback path uses .commit.author.name, which is free-form text from the commit metadata and can contain any byte) shifts the columns: count[$1] counts under Name, and add[Name] += <email> produces a numeric-coerced 0, corrupting the totals. Failure scenario: a commit authored with git commit --author="Name\t<email>" or any name containing a tab lands in the PR; the summary reports a wrong author name and wrong line counts with no marker. Impact: the model receives fabricated statistics presented as verified facts. Fix: sanitize the author field in the jq @tsv stage (e.g. gsub("\t"; " ") on the author before @tsv), or switch the row format to a delimiter that cannot appear in the name.

• should fix — ai-reviewer.sh (commit overview, STATS_ROWS accumulation): the per-commit stats loop makes up to MAX_SUMMARY_COMMITS (default 15) sequential, unbatched gh api calls with no rate-limit handling beyond the per-call failure counter. On a busy runner or a secondary-rate-limited token, 15 sequential calls per review add latency and can trip GitHub's secondary rate limits, at which point every remaining fetch fails and the header degrades to "(line stats unavailable for K commit(s))" — the feature silently produces no data exactly on the repos that trigger it. Fix: add a small sleep between calls (GitHub's guidance for sequential API calls), or use the GraphQL API / a conditional batch strategy, or at minimum log a warning when STATS_FAILURES equals the full listed count so operators can distinguish "rate limited" from "one flaky call".

• should fix — ai-reviewer.sh (COMMIT_MESSAGES jq filter): the message-body extraction split("\n\n")[1] only captures the first paragraph of a multi-paragraph commit body, and the subject/body split is wrong for messages whose subject is followed by a single newline then body text (no blank line) — split("\n\n")[1] is null there, so the body is silently dropped. Failure scenario: a commit message "fix: thing\nDetails line" (single newline) loses Details line entirely; a message with two body paragraphs keeps only the first. Impact: the model sees an incomplete commit history with no truncation marker, contradicting the PR's own "never mistaken for complete" contract. Fix: split on the first \n\n only (split("\n\n") | .[0] for subject, .[1:] | join("\n\n") for the body), and handle the single-newline case (e.g. sub("^[^\n]*\n"; "") when no blank line exists).

• should fix — ai-reviewer.sh (check-runs summary, passed count): runs with conclusion == null but status == "completed" (e.g. neutral conclusion, or a run cancelled mid-flight with no conclusion) are counted in total and listed under other, but a run with conclusion == "neutral" — which GitHub treats as non-blocking — is presented identically to a failure in the non-passing list. Failure scenario: a workflow with a continue-on-error advisory step concludes neutral; the prompt says "N of M checks passed. Non-passing checks: - advisory: completed (neutral)", inviting the model to treat it as a CI failure and possibly request changes. Fix: either exclude neutral from the non-passing list (counting it separately or folding it into passed), or label it distinctly in the bullet.

• nit — ai-reviewer.sh (COMMENTS_JSON guard): the previous-review and human-comment blocks gate on [ "$COMMENTS_JSON" != "[]" ], but a PR whose only comments are all bot comments also yields a non-[] array, so the gate passes and jq runs only to produce empty output — harmless, but the inverse case matters more: if the merged fetch legitimately returns [] because the PR has zero comments, the warning-free skip is correct. No action needed if intentional; noting only that the sentinel comparison would be clearer as a length check (jq 'length'), which also survives any future change to the add // [] fallback shape.

• nit — tests/test_reviewer_context.py (run_reviewer default pr fixture): the default pr.json fixture has no title/body keys, so any test enabling INCLUDE_PR_DESCRIPTION without passing a custom pr exercises the .title-on-null path in jq ("**PR Title**: " + .title errors on null, caught by 2>/dev/null || echo ""). That path is currently unreachable in the suite because description tests always pass a full pr; adding "title": "" to the default fixture would make the default exercise the real formatting path instead of the error fallback.

• nit — .github/workflows/ai-code-reviewer.yml (env block): INCLUDE_PREVIOUS_REVIEWS, INCLUDE_HUMAN_COMMENTS, INCLUDE_CHECK_RUNS, INCLUDE_LABELS, INCLUDE_PR_DESCRIPTION, and INCLUDE_COMMIT_MESSAGES are now forwarded with || 'true' defaults, but the script's own defaults already handle unset values — the workflow lines are redundant for these six (they only matter if a repository variable is set). Harmless, but the || 'true' fallbacks duplicate the script defaults in a second place that can drift; forwarding bare ${{ vars.X }} (empty when unset) would keep a single source of truth. Inference (not verified): the duplication was added deliberately so the workflow file documents the defaults — if so, a comment saying so would prevent future "cleanup" from half-removing them.

• nit — ai-reviewer.sh (LISTED arithmetic): LISTED=$(( MAX_SUMMARY_COMMITS < NONMERGE_COUNT ? MAX_SUMMARY_COMMITS : NONMERGE_COUNT )) is correct, but the ternary form is unusual in POSIX-adjacent shell and shellcheck style would prefer an explicit if; purely readability, no behavior concern.

Should be checked

• Cannot verify from diff that z-ai/glm-5.3 is a valid OpenRouter model slug with structured_outputs support — the human comments assert this was confirmed via OpenRouter metadata, but the diff itself contains no evidence; if the slug is wrong or the model rejects response_format, every review fails at request time with the default STRUCTURED_OUTPUT=true. Please confirm the slug and its supported_parameters on OpenRouter's current model list.
• Cannot verify from diff that the AI Code Review check currently shown as in_progress is this workflow's own run rather than an unrelated failing check — if it is this PR's reviewer run, confirm it completes and posts the review (the script's failure modes around the new paginated fetches are the most likely new breakage points).

✅ Approved with recommendations

---

Review by Friendly AI Reviewer - made with ❤️