|
| 1 | +--- |
| 2 | +name: pr-review |
| 3 | +description: Review Vortex pull requests for correctness, Rust soundness, performance, API compatibility, and test coverage. Use when reviewing PRs, reviewing code changes, or when the user mentions "/pr-review". |
| 4 | +--- |
| 5 | + |
| 6 | +# Vortex PR Review Skill |
| 7 | + |
| 8 | +Review Vortex changes for issues that CI may miss: semantic correctness, Rust soundness, |
| 9 | +zero-copy and alignment invariants, performance, API compatibility, and missing regression |
| 10 | +coverage. |
| 11 | + |
| 12 | +## Usage Modes |
| 13 | + |
| 14 | +### GitHub Actions Mode |
| 15 | + |
| 16 | +When invoked by a PR automation, the prompt may already include PR metadata, issue body, |
| 17 | +comments, and changed files. In this mode, use git commands for the diff and history: |
| 18 | + |
| 19 | +```bash |
| 20 | +git diff origin/<base-branch>...HEAD |
| 21 | +git diff --stat origin/<base-branch>...HEAD |
| 22 | +git log origin/<base-branch>..HEAD --oneline |
| 23 | +``` |
| 24 | + |
| 25 | +If the base ref is missing, fetch only that ref: |
| 26 | + |
| 27 | +```bash |
| 28 | +git fetch origin <base-branch> --depth=1 |
| 29 | +``` |
| 30 | + |
| 31 | +Do not refetch data already provided in the prompt. Use the supplied comments and metadata as |
| 32 | +review context. |
| 33 | + |
| 34 | +### Local CLI Mode |
| 35 | + |
| 36 | +When the user provides a PR number or URL, use `gh` to fetch the PR metadata, comments, and diff: |
| 37 | + |
| 38 | +```bash |
| 39 | +gh pr view <PR_NUMBER> --json title,body,author,baseRefName,headRefName,files,additions,deletions,commits |
| 40 | +gh pr view <PR_NUMBER> --json comments,reviews |
| 41 | +gh pr diff <PR_NUMBER> |
| 42 | +``` |
| 43 | + |
| 44 | +If `gh` cannot connect to `api.github.com` because of sandbox networking, rerun with escalated |
| 45 | +network permissions. |
| 46 | + |
| 47 | +## Review Workflow |
| 48 | + |
| 49 | +1. Read `AGENTS.md` and any nested `AGENTS.md` for project conventions. |
| 50 | +2. Identify the change intent from the PR title, body, commits, and tests. |
| 51 | +3. Group changed files by area: arrays, encodings, buffers, file/layout, integrations, bindings, |
| 52 | + docs, or CI. |
| 53 | +4. Trace changed behavior through callers, trait implementations, dtype/nullability handling, |
| 54 | + validity masks, and tests. |
| 55 | +5. Focus findings on actionable defects. Avoid commenting on formatting or issues already covered |
| 56 | + by clippy, rustfmt, or generated API checks. |
| 57 | +6. Scope verification to the change. Rust/API changes need Rust checks; docs-only, agent-only, |
| 58 | + symlink-only, and metadata-only changes should use targeted validation such as Markdown review, |
| 59 | + `ls`, `find`, `git status`, or relevant config linters. |
| 60 | + |
| 61 | +## Review Areas |
| 62 | + |
| 63 | +| Area | Focus | |
| 64 | +| --- | --- | |
| 65 | +| Correctness | Length and dtype invariants, nullability, validity masks, offset math, canonicalization, boundary conditions, empty arrays, scalar vs array behavior | |
| 66 | +| Rust soundness | `unsafe` blocks, aliasing, lifetimes, alignment, FFI boundaries, panic safety, ownership of buffers and arrays | |
| 67 | +| Compression and IO | Encoding metadata, statistics, layout evolution, file compatibility, scan projection/filter behavior, async IO edge cases | |
| 68 | +| Performance | Unnecessary copies, lost zero-copy behavior, avoidable allocations, poor cache locality, quadratic loops, excessive dynamic dispatch in hot paths | |
| 69 | +| Error handling | Correct `vortex_err!` and `vortex_bail!` usage, useful messages, no accidental panics on user data | |
| 70 | +| API compatibility | Public API docs, public-api lock updates, feature flags, crate boundaries, Python/Java binding impacts | |
| 71 | +| Tests | Regression coverage, edge cases, parameterized cases with `rstest`, use of `assert_arrays_eq!`, docs doctests when docs change | |
| 72 | +| Verification scope | Avoid requesting or running expensive workspace checks when the PR only changes docs, agent files, symlinks, or metadata | |
| 73 | + |
| 74 | +## Output |
| 75 | + |
| 76 | +Lead with findings, ordered by severity. For each finding include: |
| 77 | + |
| 78 | +- File and line reference. |
| 79 | +- Why the issue is a real bug or material risk. |
| 80 | +- A concrete fix or verification path when possible. |
| 81 | + |
| 82 | +Use inline review comments when the environment supports them and a precise changed line is the |
| 83 | +best place for the feedback. Keep broad design feedback in the summary. |
| 84 | + |
| 85 | +If no issues are found, say so explicitly and mention any residual risk or tests not run. |
| 86 | + |
| 87 | +## Principles |
| 88 | + |
| 89 | +- Review the code that changed, but inspect enough surrounding code to validate invariants. |
| 90 | +- Do not infer causation from commit messages alone. Verify with code, tests, or logs. |
| 91 | +- Do not ask for broad rewrites when a narrow fix would address the risk. |
| 92 | +- Do not downgrade a correctness or soundness issue to a nit because it is inconvenient. |
| 93 | +- Be specific and proportionate. |
0 commit comments