diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index 5907e13f3..7646f8eed 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -9,6 +9,15 @@ Review like a maintainer: direct, selective, and focused on issues that CI does not prove. Prefer a few high-confidence findings over exhaustive commentary. Zero findings is a valid result. +## Target-version evidence + +Resolve Python/dependency compatibility, pytest selectors and hardware needs +from the frozen head's metadata, requirements, CI, and fixtures. Run levels such +as `core_model` do not imply CPU-only execution. Read the target's accepted +module contracts; draft or deferred architecture is not a merge requirement. +Recheck only affected assumptions when reviewing another release. Historical +coverage does not establish compatibility with a new snapshot. + ## Quality contract Make every finding: @@ -103,6 +112,12 @@ If docs and live code disagree, verify the code/tests and report the drift. | [examples-policy.md](../precheck-pr/references/examples-policy.md) | The PR adds, copies, or renames Python under `examples/`; apply the canonical policy shared with `precheck-pr`. | | [find-simplifications](../find-simplifications/SKILL.md) | Every review; run a diff-scoped subtraction and simplification pass after correctness blockers. | +### Batch sessions and review helpers + +For a requested batch session, unanswered review replies, or inline-coordinate +validation, read [batch-review.md](references/process/batch-review.md). The +repository owns the helper scripts and their offline regression tests. + ### Delivery and reviewer coordination | Reference | Read when | @@ -110,6 +125,15 @@ If docs and live code disagree, verify the code/tests and report the drift. | [maintainer-style-study.md](references/delivery/maintainer-style-study.md) | Findings are ready for concise maintainer-style delivery. | | [review-requests.md](references/delivery/review-requests.md) | The user asks to identify, suggest, request, or ping code-owner reviewers. | +## Optional review workers + +For independent defect discovery on a large diff, or when the user selects +`auto`, `codex`, `claude`, or `none`, read +[reviewer-backends.md](references/process/reviewer-backends.md). Pass immutable +base/head SHAs and a bounded read-only scope. The parent owns domain routing, +verification, deduplication, severity, and any separately authorized posting. +Missing optional tooling falls back to direct review without blocking it. + ## Workflow ### 1. Freeze and report the snapshot diff --git a/.claude/skills/review-pr/references/checks/test-quality-evaluation.md b/.claude/skills/review-pr/references/checks/test-quality-evaluation.md index fb1427f01..20e6f1c99 100644 --- a/.claude/skills/review-pr/references/checks/test-quality-evaluation.md +++ b/.claude/skills/review-pr/references/checks/test-quality-evaluation.md @@ -20,11 +20,15 @@ were reverted or broken. Check: tolerances are deterministic or explicitly controlled; - normal, invalid, boundary, feature-off, failure/cancellation, and regression paths are covered where the diff changes them; -- run-level and domain markers place hardware/model tests in the intended CI - lane without silently skipping the contract. +- read the target's pytest configuration, collection hooks, hardware helpers, + and CI together: run-level/domain/backend/SKU/card-count selectors must place + tests in the intended lane. `core_model` alone is not CPU-only; use explicit + hardware selection such as `core_model and cpu` only when supported by the + target. Zero selected or skipped accelerator tests are not validation. -Map source symbols to tests with bounded `rg` searches; do not assume the test -tree mirrors production paths. +Map source symbols to tests with bounded `rg` searches and inspect the target's +test-writing guide. Components may mirror source while model E2E and feature +suites live elsewhere; imports, fixtures, and assertions establish coverage. ## Runtime check diff --git a/.claude/skills/review-pr/references/process/batch-review.md b/.claude/skills/review-pr/references/process/batch-review.md new file mode 100644 index 000000000..660b90312 --- /dev/null +++ b/.claude/skills/review-pr/references/process/batch-review.md @@ -0,0 +1,43 @@ +# Requested batch review and helper tools + +Use only for a requested batch session, review follow-up, or proposed inline +comments. Commands below run from this skill directory, require `gh`, `jq`, +Python, and Bash, and only read GitHub. Resolve the connected reviewer's login; +do not infer it from examples or repository ownership. + +1. Check outstanding replies first when the session includes follow-up: + `bash scripts/check_replies.sh --reviewer --days 14`. + Reopen each returned thread and inspect its current resolution before + deciding whether a response is needed. A reply can already be addressed by + code or another maintainer; do not send an automatic acknowledgement. +2. Select candidates with + `bash scripts/select_prs.sh --reviewer --days 7 --limit 5`. + This preserves the existing selector: open recent PRs, excluding own, + draft/WIP/"don't merge", and already-reviewed PRs, with low-review-count + candidates first. It does not filter ordinary docs/CI PRs or prove + mergeability, CI success, or complete search coverage. +3. Freeze and review each selected PR using the normal execution contract. + Keep CI conclusions separate from conflicts/mergeability. Match depth to + changed behavior, not candidate rank; no quota requires manufacturing + findings. Search/review APIs can be limited or fail; inspect raw status when + coverage matters rather than treating a zero count as a verified clean state. +4. Before separately authorized inline posting, validate the proposed payload: + `printf '%s' "$REVIEW_JSON" | bash scripts/verify_line_numbers.sh `. + Input is an object with a `comments` array containing `path`, `line`, + optional `side` (default `RIGHT`), and optional same-side `start_line`. + It checks actual diff hunks, including deletion/rename paths. It does not + verify the finding or submit it. Re-read base/head before and after its + live diff fetch; require equality with the reviewed snapshot and use that + head as `commit_id` for any authorized post. Otherwise refresh the review + and line mapping first. +5. Preserve a short session record of pinned heads, findings, validation gaps, + and already-posted comment links so a repeat pass reviews only the delta. + +Run helper regression tests without project dependencies or GitHub access: + +```bash +python tests/test_helpers.py +``` + +The helpers are bounded triage aids, not a scheduler. They do not authorize +follow-up comments, review events, code changes, or recurring monitoring. diff --git a/.claude/skills/review-pr/references/process/reviewer-backends.md b/.claude/skills/review-pr/references/process/reviewer-backends.md new file mode 100644 index 000000000..342d5fc50 --- /dev/null +++ b/.claude/skills/review-pr/references/process/reviewer-backends.md @@ -0,0 +1,80 @@ +# Reviewer Backends + +Use this reference when delegating defect discovery to Codex, Claude, or another reviewer. The parent `review-pr` workflow remains responsible for gates, domain routing, verification, prioritization, and any GitHub writes. + +## Backend Selection + +| Choice | Behavior | +|--------|----------| +| `auto` | Use one available reviewer backend; fall back to direct review | +| `codex` | Use an available Codex review agent or skill | +| `claude` | Use an available Claude review agent or CLI | +| `none` | Review directly without delegation | + +Honor an explicit user choice. Do not block when the requested backend is unavailable; report the fallback locally and continue directly unless the user required that backend specifically. + +Use one backend for ordinary code changes. Use at most two reviewers for large or high-risk changes, either with non-overlapping path scopes or as independent passes. Do not run multiple reviewers merely to vote on the same conclusion. + +## Common Read-Only Handoff + +Give every backend the same concrete target: + +```yaml +repository: /absolute/path/to/vllm-omni +base_ref: origin/main +base_sha: +head_sha: +merge_base_sha: +scope: complete-diff | path-partition +paths: [] +focus: [] +constraints: + read_only: true + no_github_writes: true + no_delegation: true +output: + finding: "[P0-P3] Imperative title — path:line" + exhaustive: true + include_assessment: true + include_test_gaps: true + include_residual_risks: true +``` + +Capture immutable SHAs before delegation. Ask the reviewer to: + +1. Read the applicable `AGENTS.md`. +2. Inspect the complete assigned diff and enough surrounding code to understand it. +3. Check relevant tests and call sites. +4. Report only concrete regressions introduced by the change. +5. Cite the smallest changed-line range that demonstrates each finding. +6. Continue through the full assigned scope after finding the first issue. + +The worker must not modify files, create commits, push branches, post comments, submit a review event, or delegate again. + +## Backend Adapters + +### Codex + +Use an available Codex review agent or review skill; do not assume that a named system skill is installed. Give it the common handoff plus selected vLLM-Omni focus areas. Use it for a complete change or bounded path partition, not for a one-function context lookup. + +### Claude + +Pass the same handoff and output format to the available Claude review surface. Do not assume a particular command, model, or installation path. Explicitly include the read-only and no-GitHub-write constraints in the prompt. + +### Direct fallback + +If no backend is available, the parent performs the same checks. Keep the same evidence threshold and comment budget. Do not mention missing tooling in the GitHub review unless the user asks. + +## Aggregate Results + +The parent reviewer must: + +1. Re-read the PR head SHA. If it changed, discard stale line mappings and rerun affected scopes. +2. Verify each finding against code, tests, and the captured diff. +3. Drop speculative, pre-existing, intentional, or style-only findings. +4. Deduplicate by root cause and affected call path rather than by line alone. +5. Classify `blocking` independently from `P0`-`P3`; priority alone does not determine the verdict. +6. Apply the vLLM-Omni comment budget after collecting the complete result. +7. Preserve material omitted findings, test gaps, and residual risks in the local report. + +Only the parent may turn selected findings into GitHub comments, and only with user authorization. diff --git a/.claude/skills/review-pr/scripts/check_replies.sh b/.claude/skills/review-pr/scripts/check_replies.sh new file mode 100644 index 000000000..5346cb920 --- /dev/null +++ b/.claude/skills/review-pr/scripts/check_replies.sh @@ -0,0 +1,87 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: Copyright contributors to the vLLM-Omni project +# check_replies.sh — Find unanswered contributor replies to your review comments. +# +# Usage: ./check_replies.sh --reviewer lishunyang12 [--days 14] +# +# Scans PRs you've reviewed for threads where a contributor replied +# but you haven't responded yet. Outputs actionable list. + +set -euo pipefail + +REPO="vllm-project/vllm-omni" +REVIEWER="" +DAYS=14 + +while [[ $# -gt 0 ]]; do + case $1 in + --reviewer) REVIEWER="$2"; shift 2 ;; + --days) DAYS="$2"; shift 2 ;; + *) echo "Unknown arg: $1" >&2; exit 1 ;; + esac +done + +if [ -z "$REVIEWER" ]; then + echo "Usage: $0 --reviewer " >&2 + exit 1 +fi + +SINCE=$(date -d "-${DAYS} days" +%Y-%m-%d 2>/dev/null || date -v-${DAYS}d +%Y-%m-%d) + +echo "Scanning for unanswered replies to ${REVIEWER} (last ${DAYS} days)..." >&2 + +# Find PRs the reviewer has commented on recently +REVIEWED_PRS=$(gh api "search/issues?q=repo:${REPO}+reviewed-by:${REVIEWER}+is:pr+is:open+updated:>=${SINCE}&per_page=50" \ + --jq '.items[].number' 2>/dev/null) + +UNANSWERED=0 + +for PR_NUM in $REVIEWED_PRS; do + # Get all review comments on this PR + COMMENTS=$(gh api "repos/${REPO}/pulls/${PR_NUM}/comments" \ + --jq '[.[] | {id, user: .user.login, body: .body[:100], in_reply_to_id, created_at, path, url: .html_url}]' 2>/dev/null) + + # Find threads: reviewer's comments that got a reply, but reviewer didn't reply back + # 1. Get reviewer's comment IDs + REVIEWER_IDS=$(echo "$COMMENTS" | jq -r --arg r "$REVIEWER" '[.[] | select(.user == $r) | .id]') + + # 2. Find replies to reviewer's comments (from others) + REPLIES=$(echo "$COMMENTS" | jq -c --arg r "$REVIEWER" --argjson rids "$REVIEWER_IDS" ' + [.[] | + select(.user != $r) | + select(.in_reply_to_id != null) | + select([.in_reply_to_id] | inside($rids)) + ] + ') + + REPLY_COUNT=$(echo "$REPLIES" | jq 'length') + if [ "$REPLY_COUNT" -eq 0 ]; then + continue + fi + + # 3. For each reply, check if reviewer replied after it + while IFS= read -r reply; do + THREAD_ID=$(echo "$reply" | jq -r '.in_reply_to_id') + REPLY_DATE=$(echo "$reply" | jq -r '.created_at') + REPLY_USER=$(echo "$reply" | jq -r '.user') + REPLY_URL=$(echo "$reply" | jq -r '.url') + + # Check if reviewer replied after this reply + FOLLOWUP=$(echo "$COMMENTS" | jq --arg r "$REVIEWER" --arg date "$REPLY_DATE" --argjson tid "$THREAD_ID" ' + [.[] | select(.user == $r) | select(.in_reply_to_id == $tid) | select(.created_at > $date)] | length + ') + + if [ "$FOLLOWUP" -eq 0 ]; then + REPLY_BODY=$(echo "$reply" | jq -r '.body') + echo "PR #${PR_NUM} | @${REPLY_USER} replied | ${REPLY_URL}" + echo " > ${REPLY_BODY}" + echo "" + UNANSWERED=$((UNANSWERED + 1)) + fi + done < <(echo "$REPLIES" | jq -c '.[]') +done + +if [ "$UNANSWERED" -eq 0 ]; then + echo "No unanswered replies found." >&2 +fi diff --git a/.claude/skills/review-pr/scripts/select_prs.sh b/.claude/skills/review-pr/scripts/select_prs.sh new file mode 100644 index 000000000..e315a5de7 --- /dev/null +++ b/.claude/skills/review-pr/scripts/select_prs.sh @@ -0,0 +1,73 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: Copyright contributors to the vLLM-Omni project +# select_prs.sh — Pick reviewable PRs from vllm-omni for a review session. +# +# Usage: ./select_prs.sh [--days 7] [--limit 5] [--reviewer lishunyang12] +# +# Filters: +# - Excludes your own PRs (--reviewer) +# - Excludes WIP / Draft / "Don't merge" PRs +# - Excludes pure docs/CI-only merged PRs +# - Excludes PRs you already reviewed +# - Prioritizes zero-review PRs +# +# Output: JSON array of {number, title, author, createdAt, reviewCount} + +set -euo pipefail + +REPO="vllm-project/vllm-omni" +DAYS=7 +LIMIT=5 +REVIEWER="" + +while [[ $# -gt 0 ]]; do + case $1 in + --days) DAYS="$2"; shift 2 ;; + --limit) LIMIT="$2"; shift 2 ;; + --reviewer) REVIEWER="$2"; shift 2 ;; + *) echo "Unknown arg: $1" >&2; exit 1 ;; + esac +done + +SINCE=$(date -d "-${DAYS} days" +%Y-%m-%d 2>/dev/null || date -v-${DAYS}d +%Y-%m-%d) + +echo "Fetching open PRs from last ${DAYS} days..." >&2 + +# Fetch all open PRs from the time range +ALL_PRS=$(gh pr list --repo "$REPO" --state open --limit 200 \ + --json number,title,author,createdAt,isDraft \ + --search "created:>=${SINCE}") + +# Filter in a single jq pass +FILTERED=$(echo "$ALL_PRS" | jq --arg reviewer "$REVIEWER" ' + [.[] | + # Skip own PRs + select(.author.login != $reviewer) | + # Skip drafts + select(.isDraft == false) | + # Skip WIP / Dont merge + select(.title | test("\\[WIP\\]|\\[Draft\\]|Don.t[ ]+merge|\\[Don.t\\]"; "i") | not) + ] +') + +# For each remaining PR, check review count and whether reviewer already reviewed +echo "Checking review status for $(echo "$FILTERED" | jq length) PRs..." >&2 + +echo "$FILTERED" | jq -c '.[]' | while read -r pr; do + NUM=$(echo "$pr" | jq -r '.number') + + # Get review count + REVIEW_COUNT=$(gh api "repos/${REPO}/pulls/${NUM}/reviews" --jq 'length' 2>/dev/null || echo "0") + + # Check if reviewer already reviewed + if [ -n "$REVIEWER" ]; then + ALREADY=$(gh api "repos/${REPO}/pulls/${NUM}/reviews" \ + --jq "[.[] | select(.user.login==\"${REVIEWER}\")] | length" 2>/dev/null || echo "0") + if [ "$ALREADY" -gt 0 ]; then + continue + fi + fi + + echo "$pr" | jq --argjson rc "$REVIEW_COUNT" '. + {reviewCount: $rc}' +done | jq -s "sort_by(.reviewCount) | .[:${LIMIT}]" diff --git a/.claude/skills/review-pr/scripts/verify_line_numbers.py b/.claude/skills/review-pr/scripts/verify_line_numbers.py new file mode 100755 index 000000000..86ddf8e57 --- /dev/null +++ b/.claude/skills/review-pr/scripts/verify_line_numbers.py @@ -0,0 +1,116 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: Copyright contributors to the vLLM-Omni project +"""Check review comment coordinates against the actual lines of a PR diff.""" + +import argparse +import ast +import json +import re +import subprocess +import sys + +HUNK = re.compile(r"^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@") + + +def diff_path(value): + """Decode a git path, including C-quoted UTF-8 bytes.""" + if value.startswith('"'): + value = ast.literal_eval(value).encode("latin1").decode("utf-8") + return None if value == "/dev/null" else value[2:] + + +def diff_lines(diff): + """Map each file/side to commentable lines, preserving rename/delete paths.""" + lines = {} + old_path = new_path = None + old_remaining = new_remaining = 0 + for text in diff.splitlines(): + if text.startswith("diff --git "): + old_path = new_path = None + old_remaining = new_remaining = 0 + elif match := HUNK.match(text): + old_line, old_count, new_line, new_count = match.groups() + old_line, new_line = int(old_line), int(new_line) + old_remaining = int(old_count) if old_count is not None else 1 + new_remaining = int(new_count) if new_count is not None else 1 + elif old_remaining or new_remaining: + # A hunk's content can itself start with --- or +++. + path = new_path or old_path + if text.startswith((" ", "-")): + lines.setdefault((path, "LEFT"), set()).add(old_line) + old_line += 1 + old_remaining -= 1 + if text.startswith((" ", "+")): + lines.setdefault((path, "RIGHT"), set()).add(new_line) + new_line += 1 + new_remaining -= 1 + elif text.startswith("--- "): + old_path = diff_path(text[4:]) + elif text.startswith("+++ "): + new_path = diff_path(text[4:]) + return lines + + +def validate_comments(comments, lines): + """Validate single-line and same-side multiline review comments.""" + valid = True + for comment in comments: + path = comment["path"] + side = comment.get("side", "RIGHT") + line = comment["line"] + start = comment.get("start_line", line) + start_side = comment.get("start_side", side) + available = lines.get((path, side), set()) + if ( + type(line) is not int + or type(start) is not int + or start < 1 + or line < start + or start_side != side + or line - start + 1 > len(available) + or not all(number in available for number in range(start, line + 1)) + ): + print( + f"ERROR: {path}:{start}-{line} ({side}) is outside diff lines", + file=sys.stderr, + ) + valid = False + else: + print(f"OK: {path}:{line} ({side})", file=sys.stderr) + return valid + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("pr_number", type=int) + args = parser.parse_args() + try: + review = json.load(sys.stdin) + comments = review["comments"] + if not isinstance(comments, list): + raise TypeError("comments must be an array") + result = subprocess.run( + [ + "gh", + "pr", + "diff", + str(args.pr_number), + "--repo", + "vllm-project/vllm-omni", + ], + check=True, + capture_output=True, + text=True, + ) + return 0 if validate_comments(comments, diff_lines(result.stdout)) else 1 + except subprocess.CalledProcessError as error: + print(error.stderr, file=sys.stderr, end="") + return 1 + except (OSError, ValueError, KeyError, TypeError, SyntaxError) as error: + print(f"ERROR: {error}", file=sys.stderr) + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.claude/skills/review-pr/scripts/verify_line_numbers.sh b/.claude/skills/review-pr/scripts/verify_line_numbers.sh new file mode 100644 index 000000000..2065abfb7 --- /dev/null +++ b/.claude/skills/review-pr/scripts/verify_line_numbers.sh @@ -0,0 +1,8 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: Copyright contributors to the vLLM-Omni project +# Usage: echo '' | ./verify_line_numbers.sh +# Requires Python 3 and gh. Exits nonzero for invalid comments or fetch failures. +set -euo pipefail + +exec python3 "$(dirname "${BASH_SOURCE[0]}")/verify_line_numbers.py" "$@" diff --git a/.claude/skills/review-pr/tests/test_helpers.py b/.claude/skills/review-pr/tests/test_helpers.py new file mode 100644 index 000000000..0c9db90ee --- /dev/null +++ b/.claude/skills/review-pr/tests/test_helpers.py @@ -0,0 +1,235 @@ +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: Copyright contributors to the vLLM-Omni project +"""Exercise repository review helpers without network access.""" + +import json +import os +import subprocess +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +REVIEW = ROOT / "scripts" + + +class HelperTests(unittest.TestCase): + def setUp(self): + self.temporary = tempfile.TemporaryDirectory() + self.addCleanup(self.temporary.cleanup) + self.root = Path(self.temporary.name) + self.bin = self.root / "bin" + self.bin.mkdir() + gh = self.bin / "gh" + gh.write_text("""#!/usr/bin/env python3 +import json, os, sys +from pathlib import Path +args = sys.argv[1:] +root = Path(os.environ['FIXTURES']) +if os.environ.get('FAIL_GH'): + print('GitHub unavailable', file=sys.stderr) + raise SystemExit(1) +if args[:2] == ['pr', 'list']: + print((root / 'prs.json').read_text()) +elif args[:2] == ['pr', 'diff']: + print((root / 'diff').read_text(), end='') +elif args[0] == 'api' and args[1].startswith('search/issues?'): + print('1') +elif args[0] == 'api' and args[1].endswith('/comments'): + print((root / 'comments.json').read_text()) +elif args[0] == 'api' and args[1].endswith('/reviews'): + print('0') +else: + raise SystemExit('Unexpected gh arguments: ' + repr(args)) +""") + gh.chmod(0o755) + self.env = dict( + os.environ, + PATH=f"{self.bin}{os.pathsep}{os.environ['PATH']}", + FIXTURES=str(self.root), + PYTHONDONTWRITEBYTECODE="1", + ) + + def run_script(self, script, *args, data=None): + return subprocess.run( + ["bash", str(script), *args], + input=data, + env=self.env, + text=True, + capture_output=True, + check=False, + ) + + + + + + + + + def test_select_filters_drafts_wip_and_own_prs(self): + prs = [ + {"number": i, "title": title, "author": {"login": author}, "isDraft": draft} + for i, (title, author, draft) in enumerate( + [ + ("Fix bug", "author", False), + ("[WIP] Feature", "author", False), + ("[Draft] Feature", "author", False), + ("Don't merge yet", "author", False), + ("Feature", "reviewer", False), + ("Feature", "author", True), + ], + 1, + ) + ] + (self.root / "prs.json").write_text(json.dumps(prs)) + result = self.run_script(REVIEW / "select_prs.sh", "--reviewer", "reviewer") + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual([pr["number"] for pr in json.loads(result.stdout)], [1]) + + def verify(self, comments, diff=None): + (self.root / "diff").write_text( + diff + or ( + "diff --git a/src/example.py b/src/example.py\n" + "--- a/src/example.py\n+++ b/src/example.py\n" + "@@ -10,2 +20,2 @@\n-old\n+new\n context\n" + ) + ) + return self.run_script( + REVIEW / "verify_line_numbers.sh", + "1", + data=json.dumps({"comments": comments}), + ) + + def test_verify_valid_nested_path_and_both_sides(self): + result = self.verify( + [ + {"path": "src/example.py", "line": line, "side": side, "body": "Check"} + for side, line in [("RIGHT", 20), ("RIGHT", 21), ("LEFT", 10)] + ] + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stderr.count("OK:"), 3) + + def test_verify_rejects_outside_hunk_or_missing_file(self): + for path, line in [("src/example.py", 999), ("missing.py", 20)]: + with self.subTest(path=path): + result = self.verify([{"path": path, "line": line, "body": "Check"}]) + self.assertNotEqual(result.returncode, 0) + + def test_verify_deleted_file_on_left(self): + diff = ( + "diff --git a/gone.py b/gone.py\n--- a/gone.py\n+++ /dev/null\n" + "@@ -1 +0,0 @@\n-old\n" + ) + result = self.verify( + [{"path": "gone.py", "line": 1, "side": "LEFT", "body": "Check"}], diff + ) + self.assertEqual(result.returncode, 0, result.stderr) + result = self.verify( + [{"path": "gone.py", "line": 1, "side": "RIGHT", "body": "Check"}], diff + ) + self.assertNotEqual(result.returncode, 0) + + def test_verify_checks_multiline_comment_start(self): + result = self.verify( + [ + { + "path": "src/example.py", + "start_line": 1, + "start_side": "RIGHT", + "line": 20, + "side": "RIGHT", + "body": "Check", + } + ] + ) + self.assertNotEqual(result.returncode, 0) + + def test_verify_multiple_files_hunks_and_renamed_path(self): + diff = ( + "diff --git a/old.py b/new.py\n--- a/old.py\n+++ b/new.py\n" + "@@ -1 +1 @@\n-old\n+new\n" + "@@ -10 +10 @@\n-old\n+new\n" + "diff --git a/second.py b/second.py\n--- a/second.py\n+++ b/second.py\n" + "@@ -1 +1 @@\n-old\n+new\n" + ) + result = self.verify( + [ + {"path": path, "line": line, "body": "Check"} + for path, line in [("new.py", 1), ("new.py", 10), ("second.py", 1)] + ], + diff, + ) + self.assertEqual(result.returncode, 0, result.stderr) + result = self.verify([{"path": "second.py", "line": 10, "body": "Check"}], diff) + self.assertNotEqual(result.returncode, 0) + + def test_verify_quoted_path_and_header_like_content(self): + diff = ( + 'diff --git "a/a\\tb.py" "b/a\\tb.py"\n' + '--- "a/a\\tb.py"\n+++ "b/a\\tb.py"\n' + "@@ -1 +1 @@\n--- content\n+++ content\n" + ) + result = self.verify([{"path": "a\tb.py", "line": 1, "body": "Check"}], diff) + self.assertEqual(result.returncode, 0, result.stderr) + + def test_verify_rejects_malformed_input_and_fetch_failure(self): + for data in ("not json", '{"comments": null}'): + result = self.run_script(REVIEW / "verify_line_numbers.sh", "1", data=data) + self.assertNotEqual(result.returncode, 0) + self.env["FAIL_GH"] = "1" + result = self.verify([{"path": "src/example.py", "line": 20, "body": "Check"}]) + self.assertNotEqual(result.returncode, 0) + self.assertIn("GitHub unavailable", result.stderr) + + def replies(self, answered): + comments = [ + { + "id": 100, + "user": "reviewer", + "body": "Please fix", + "in_reply_to_id": None, + "created_at": "2026-09-01T00:00:00Z", + "url": "root", + }, + { + "id": 101, + "user": "author", + "body": "Fixed", + "in_reply_to_id": 100, + "created_at": "2026-09-02T00:00:00Z", + "url": "reply", + }, + ] + if answered: + comments.append( + { + "id": 102, + "user": "reviewer", + "body": "Thanks", + "in_reply_to_id": 100, + "created_at": "2026-09-03T00:00:00Z", + "url": "answer", + } + ) + (self.root / "comments.json").write_text(json.dumps(comments)) + return self.run_script(REVIEW / "check_replies.sh", "--reviewer", "reviewer") + + def test_replies_ignores_answered_thread(self): + result = self.replies(answered=True) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout, "") + self.assertIn("No unanswered replies found", result.stderr) + + def test_replies_reports_unanswered_without_false_clean_summary(self): + result = self.replies(answered=False) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("@author replied", result.stdout) + self.assertNotIn("No unanswered replies found", result.stderr) + + + +if __name__ == "__main__": + unittest.main()