vllm-omni-review

name: vllm-omni-review description: Review PRs on vllm-project/vllm-omni by routing to the right domain skills, checking critical evidence, and focusing comments on blocking issues. Use when reviewing pull requests or local branches, triaging review depth, running detailed or default review, or checking tests, benchmarks, and breaking changes in vllm-omni.

vLLM-Omni PR Review

Overview

Review PRs like a real maintainer — direct, selective, and focused on high-signal issues. Prioritize 2-3 real problems per PR over exhaustive coverage. Most PRs should get 1-5 short comments; some just get an empty APPROVE.

Use this skill as a router for vllm-project/vllm-omni pull request reviews. Keep the default context small, load only the references that match the diff, and prioritize high-confidence findings over coverage theater.

Usage modes

Inspired by common PR-review skill patterns (e.g. explicit modes + tool choice); repo is always vllm-project/vllm-omni unless the user says otherwise.

Depth: Default = maintainer-style brevity (comment budget). If the user asks for detailed / in-depth / line-by-line, add a Specific comments list with path:line items; do not duplicate those points as long prose in the review body. Still respect the usual inline ceiling unless the user explicitly wants an audit-style pass.

What to prioritize (CI-complement)

• Investigate, don’t guess — If a checklist item might apply (e.g. connector lifecycle, diffusion cache), read surrounding code or use parallel subagents for independent areas. A wrong guess is worse than silence.
• Focus on what automation doesn’t prove — Design, API contracts, stage/connector invariants, test adequacy for omni paths, breaking behavior. Do not re-argue formatting or issues pre-commit / CI already failed (point at the gate instead).
• Structured notes — If you produce a multi-section writeup for the user, omit sections with no findings (no “No issues” padding).

Parallel investigation: Large diffs or multiple subsystems (e.g. entrypoints/ + engine/ + diffusion/) → split by directory or concern and investigate in parallel when subagents exist.

Subagent context compression: Never fetch a 500-line source file into your main context. Instead, delegate to a subagent with a specific question (e.g. "What does _resolve_ref_audio return?") and get back a compact summary. This is the single biggest token saver — a subagent burns its own context, you get back a paragraph.

When to use subagents vs direct fetch:

• Reading surrounding code to verify a pattern → subagent (returns summary)
• Reading a reference file you need for the blocker scan → direct (you need the full patterns)
• Fetching PR metadata / diff → direct (small, needed in main context)
• Verifying function signatures, return types, class hierarchies → subagent

Which reference to load (do not load everything)

Legacy paths (do not load — content merged): pitfalls.md → blocker-patterns.md Part 2; code-patterns.md → architecture.md Code patterns for review; python-style-guide.md → review-execution.md Python style (review flags); batch/CI triage → review-execution.md (Batch / CI sections).

Priority Hierarchy Under Context Pressure

If context is limited, prioritize: blocker scan → evidence → domain routing → verdict.

Always run the blocker scan. Under context pressure, do a shallow scan of the most critical categories (Correctness, Security) and flag that the scan was incomplete.

Core Workflow

Check whether this PR is still a draft or WIP in the PR title, if so, end the review process.

Token budget principle: Post inline comments as you find them. Use subagents for codebase investigation. Load references only after skimming the diff. If you're past ~60% context and haven't posted comments, wrap up and post what you have — partial review posted is better than a perfect review lost.

Step 0: Verify Review Gates First

If this is a ready-to-review PR, check the mergeability and required checks (DCO, pre-commit, mergeability). If failing, stop and ask the author to fix gates before proceeding.

For gate commands, review submission, and comment style, see references/review-execution.md.

Then continue with the workflow below.

Step 1: Fetch Diff First, Then Decide What to Load

Diff-first loading: Fetch the diff and PR metadata first. Skim the diff to understand what subsystems are touched, then load ONLY the references that match. Do not load references speculatively.

Fetch:

• PR metadata and changed files
• The diff
• Linked issues for [Bugfix] and [Feature] PRs only when conventions are unclear
• Related PRs only when conventions or prior decisions are unclear

Group changes by kind (runtime code, tests, docs, configs) to see where risk sits; then load references (Step 4) only for areas touched.

Do not fetch broad extra context unless the diff leaves real ambiguity.

Step 2: Blocker Scan (Required First)

Execute this scan before any other review activity. For each category, explicitly mark PASS or list blocking issues found.

BLOCKER scan:
| Category            | Result                                  |
|---------------------|-----------------------------------------|
| Correctness         | PASS / ISSUES: (list)                   |
| Reliability/Safety  | PASS / ISSUES: (list)                   |
| Breaking Changes    | PASS / (check PR description first)     |
| Test Coverage       | PASS / (check PR desc for evidence) / needs tests |
| Documentation       | PASS / ISSUES: (list)                   |
| Security            | PASS / ISSUES: (list)                   |

Blocker categories:

Evidence standard: Code inspection suffices for code-level blockers. For test coverage, require CI logs or PR description evidence.

Confidence threshold: Flag obvious cases only. For suspicious but uncertain cases, add a non-blocking comment.

Special cases:

For detailed anti-patterns with code examples, see references/blocker-patterns.md.

If blockers found: Track issues internally (category + file + line). Do not paste structured BLOCKING ISSUES: templates into the review body.

If no blockers: List non-blocking suggestions and proceed to Step 3.

Step 3: Run Verification (Blocking)

When hardware access (SSH/server/GPU) is available, verify the PR works — not just that it looks correct. Bugs found during verification are blocking. See references/verification.md for detailed procedures.

1. Checkout — gh pr checkout <n> on a server with the appropriate GPU/model
2. Unit tests — run affected tests at minimum (e.g. pytest -m "core_model" for the changed area)
3. E2E smoke — for bench/tool/metric PRs, start a server and run a quick smoke test (10 prompts, low concurrency) to verify new outputs appear and the tool doesn't crash
4. Compare claims — check PR claims (metrics appear, values sensible, no crashes) against actual output
5. Report — post findings as a PR comment; bugs found are blocking

If no hardware access, skip to Step 4.

Step 4: Route to the Right Skill

Use the title prefix and changed directories to decide whether a domain skill is required. Doc-only, config-only, and test-only PRs usually skip domain skills unless the diff crosses into model or API areas.

Full prefix table, multi-skill combos, hardware detection, and delegation triggers: references/review-routing.md.

Step 5: Load Only the Relevant Review Reference

Use only the files in this table. Older docs may mention references/pitfalls.md or references/code-patterns.md; those files were removed — use Part 2 of blocker patterns and Code patterns for review in architecture instead.

Pick the narrowest references that match the diff; avoid loading every row by default.

Step 6: Ask for Concrete Validation Evidence

When tests or benchmarks are missing and PR description evidence is insufficient, ask for specific evidence:

For [Feature] PRs affecting performance or [Performance] PRs, use the checklist in references/tests-docs-checklist.md section 5.

Be explicit in review comments. Treat "manual verification only" as insufficient unless automation is genuinely impossible.

Step 7: Verify Perf/Accuracy Claims — A/B Required (Blocking)

When to activate: [Performance] / [Perf] prefix, or quantitative perf/accuracy claims in PR body, or Step 5 flagged missing benchmarks.

Core rule: Every perf PR must provide A/B results for BOTH perf AND accuracy. Speedup with silent quality degradation is a blocker.

Load references/perf-verification.md for full workflow, regression rules, git worktree setup, and evidence templates.

Quick check (reviewer asks contributor):

1. Perf A/B — Latency, throughput, VRAM: baseline (main) vs PR, same HW/inputs. Mean ± stddev over ≥3 runs, warmup excluded.
2. Accuracy A/B — Quantitative metric or side-by-side samples. Same seeds/inputs, baseline vs PR.
3. Environment — HW, software versions, model, config, exact commands.

Regression rules: Accuracy degradation = blocking. VRAM > 5% regression = blocking. Latency > 10% regression = must explain.

Independent verification (when hardware available): git worktrees (main vs PR), fresh venvs, A/B run with 20-min timeout per branch. Deliver Claimed vs Measured report locally; ask before posting to PR.

Step 8: Evaluate Test Quality (Blocking)

When to activate: PR adds or modifies test files, or PR touches core code (engine/, stages/, connectors/) without adding tests, or PR is test-only.

Load references/test-quality-evaluation.md.

Workflow:

1. Static analysis (always runs) — check assertion quality, anti-patterns, marker compliance, edge case coverage
2. Detect hardware — same detection as Step 6 (cross-referenced from perf-verification.md)
3. Find affected tests — map changed source files to test files via grep (not path convention)
4. Filter by hardware — skip tests requiring unavailable resources
5. Run tests — pytest with --run-level core_model by default; use advanced_model only if hardware is sufficient
6. Categorize failures — test bug / code bug / infrastructure / flaky
7. Assess quality — score assertion quality, edge case coverage, marker compliance, anti-patterns (A-D grades for internal analysis)

Graceful degradation:

Delivery: Local assessment first, ask user before posting. Convert worst 1-2 findings to inline comments (counts against comment budget). If D-grade dimension or code bug found, escalate to REQUEST_CHANGES via Step 9.

Step 9: Incremental Posting + Final Verdict

Post inline comments directly to GitHub as you find them. Do not accumulate comments for a batch post at the end. Each gh api call posts one or more comments immediately. If context runs out mid-review, the comments already posted are safe on GitHub.

Posting strategy:

• After completing the blocker scan, post any blocking-issue comments immediately
• As domain review surfaces issues, post each comment right away
• Minor style nits can be batched (up to 3) in a single review call if they're on the same file
• If you find yourself past ~60% context, stop investigating and post whatever you have

Final Verdict

Do not submit a review event (APPROVE / COMMENT / REQUEST_CHANGES) — leave the verdict decision to the user.

Summarize locally:

• What was validated
• What still lacks evidence
• Recommended verdict (local presentation only)

Recommended verdict mapping:

• APPROVE — no blockers; body optional (empty is fine).
• COMMENT — suggestions only; body optional (~50% should be empty).
• REQUEST_CHANGES — genuine blocking issues only (crashes, data loss, security, policy gates).

For tone and inline style, see references/review-execution.md. For maintainer phrasing samples, see references/maintainer-style-study.md.

Review Heuristics

Trust PR description and CI evidence before demanding new tests. Prefer regression tests for [Bugfix], contract tests for API changes, and scan engine → connectors → stages → entrypoints before peripheral files. Skip style nits unless they mask correctness.

When to Fetch More Context

Fetch more context when:

• The diff snippet hides lifecycle or cleanup behavior
• A config key or API field is introduced without nearby validation
• A benchmark claim references unseen measurement code
• The PR appears to rely on prior design discussion

Keep additional fetches narrow and tied to a specific uncertainty.

Diffusion Model PR Review

When: Title prefix [Model], [New Model], [Image], [ImageGen], [Video], [VideoGen], or [Diffusion], or a new diffusion model path under vllm_omni/diffusion/.

Load references/diffusion-checklist.md instead of expanding the full workflow here: Dimensions 1–4 (including optional canonical PR body template + measurement guidance for Dimension 1), gh/grep detection commands, Quick Red Flags, combined-feature rules.

Gate order before approve: PR body evidence (script, samples, latency, VRAM) → offline and online paths → at least one acceleration and one memory optimization (new models) → docs tables + usage examples → required e2e online test → offline test if no e2e → combined test when 2+ accel/memory features.

Use Quick Red Flags actions for comment severity; write human-shaped GitHub comments, not giant pasted templates.

Batch Review Session

For daily sessions (reply-first, PR selection, pacing, re-review): references/review-execution.md — sections Batch and daily review sessions and related scripts.

References

All paths are under skills/vllm-omni-review/references/. There is no pitfalls.md, code-patterns.md, or python-style-guide.md — see the legacy map in Which reference to load above.

• Review execution — Gates, gh fetch/submit, comment budget, tone, Python style (review flags) (imports, naming, common flags — use to avoid nit wars), batch session, CI log triage
• Verification — Hardware verification: checkout, unit tests, E2E smoke, claim comparison, reporting
• Review routing — Prefix → domain skill, hardware, multi-skill
• Blocker patterns — Part 1: merge-blocking patterns; Part 2: MRO/mixins, connectors, async/sync, diffusion latents, API validation, tensor parallel, mocks (former pitfalls content)
• Architecture — Layers and critical paths; end section Code patterns for review = async, distributed, KV cache, validation, connectors, errors, logging (former code-patterns content)
• Diffusion checklist — Diffusion PR dimensions, PR body template, Quick Red Flags
• Tests & docs checklist — High-risk coverage matrix and docs sync
• Maintainer style study — Example maintainer phrasing