|
| 1 | +--- |
| 2 | +name: review-pr |
| 3 | +description: Review a pull request by URL for code format, types, semantic correctness, readability, redundant logic, bugs, and security concerns |
| 4 | +disable-model-invocation: true |
| 5 | +allowed-tools: Bash(gh pr *) Bash(gh api *) Bash(git diff *) Bash(git log *) Bash(git status) Bash(git fetch *) Bash(git checkout *) Bash(git rev-parse *) Bash(git merge-base *) Bash(make lint) Bash(make format) Bash(make types) Bash(make test) Read |
| 6 | +argument-hint: "<pr-url>" |
| 7 | +--- |
| 8 | + |
| 9 | +Review a pull request for code quality issues across six dimensions: format, types, semantic correctness, readability, redundant logic, and bugs/security. |
| 10 | + |
| 11 | +## Step 0: Fetch PR information |
| 12 | + |
| 13 | +`$ARGUMENTS` must be a GitHub PR URL (e.g. `https://github.com/owner/repo/pull/123`). |
| 14 | + |
| 15 | +If `$ARGUMENTS` is empty, ask the user for the PR URL. Do not proceed without it. |
| 16 | + |
| 17 | +1. Extract the PR number from the URL. |
| 18 | +2. Run `gh pr view <pr-url> --json number,title,baseRefName,headRefName,commits,files` to get PR metadata. |
| 19 | +3. Fetch the PR branch locally: |
| 20 | + - `git fetch origin pull/<number>/head:pr-<number>` to create a local ref for the PR. |
| 21 | + - Determine the merge base: `git merge-base origin/<baseRefName> pr-<number>` |
| 22 | +4. Use the merge base as the base ref for all diff commands. |
| 23 | + |
| 24 | +Tell the user which PR you're reviewing (number, title, head branch, base branch). |
| 25 | + |
| 26 | +## Step 1: Gather changes |
| 27 | + |
| 28 | +Run these commands to understand the full scope of changes: |
| 29 | + |
| 30 | +- `git log --oneline <merge-base>..pr-<number>` — list all commits in the PR. |
| 31 | +- `git diff --stat <merge-base>..pr-<number>` — file-level summary of what changed. |
| 32 | +- `git diff <merge-base>..pr-<number>` — full diff for detailed analysis. |
| 33 | + |
| 34 | +Read every changed file in full (from the PR branch) so you can evaluate changes in their surrounding context, not just the diff hunks in isolation. Use `git show pr-<number>:<file-path>` to read files from the PR branch without switching branches. |
| 35 | + |
| 36 | +## Step 2: Format check |
| 37 | + |
| 38 | +Run `make lint` and `make format` against the PR branch to check for formatting and linting violations. |
| 39 | + |
| 40 | +To do this without modifying the current working tree: |
| 41 | +- Check out the PR branch: `git checkout pr-<number>` |
| 42 | +- Run `make lint` and `make format`. |
| 43 | +- After the review is complete (Step 8), check out the original branch. |
| 44 | + |
| 45 | +Report results: |
| 46 | +- **Pass** — no issues found. |
| 47 | +- **Fail** — list every violation with file path, line number, and the rule ID. Group by file. |
| 48 | + |
| 49 | +Do NOT auto-fix. Only report. |
| 50 | + |
| 51 | +## Step 3: Type check |
| 52 | + |
| 53 | +Run `make types` to run pyright on the PR branch. |
| 54 | + |
| 55 | +Report results: |
| 56 | +- **Pass** — no type errors. |
| 57 | +- **Fail** — list every error with file path, line number, and the pyright message. Group by file. |
| 58 | + |
| 59 | +For the `mcp-app/` TypeScript code, if any `.ts` or `.tsx` files are in the diff, note that TypeScript type checking should be run separately inside `mcp-app/` if a tsconfig and type-check script exist there. |
| 60 | + |
| 61 | +## Step 4: Semantic review |
| 62 | + |
| 63 | +Analyze every changed file for semantic correctness. For each file, check: |
| 64 | + |
| 65 | +1. **Behavioral changes** — Does the change alter existing behavior? Is that intentional or an accidental regression? Look for: |
| 66 | + - Changed return values, error handling, or control flow. |
| 67 | + - Modified default values or function signatures. |
| 68 | + - Altered query/filter logic. |
| 69 | + |
| 70 | +2. **Edge cases** — Are there inputs or states the new code doesn't handle? |
| 71 | + - Empty/null/zero-length inputs. |
| 72 | + - Boundary values (off-by-one, max int, empty string vs None). |
| 73 | + - Concurrent or async race conditions. |
| 74 | + |
| 75 | +3. **API contract consistency** — Do changes to models, tool signatures, or config stay consistent across: |
| 76 | + - `models.py` ↔ `parsers.py` ↔ `formatters.py` |
| 77 | + - `commands.py` ↔ `tools/*.py` |
| 78 | + - `config.py` env var names ↔ documentation |
| 79 | + |
| 80 | +4. **Test coverage** — Are new code paths covered by tests? Flag any added logic that lacks a corresponding test case. |
| 81 | + |
| 82 | +Report each finding with: |
| 83 | +- File path and line number(s). |
| 84 | +- What the issue is. |
| 85 | +- Severity: **critical** (likely bug/regression), **warning** (potential issue), or **info** (suggestion). |
| 86 | + |
| 87 | +## Step 5: Readability and redundancy review |
| 88 | + |
| 89 | +Evaluate the changed code for maintainability: |
| 90 | + |
| 91 | +1. **Readability issues:** |
| 92 | + - Overly complex expressions that should be broken up. |
| 93 | + - Unclear variable or function names. |
| 94 | + - Deeply nested logic (3+ levels) that could be flattened with early returns or guard clauses. |
| 95 | + - Magic numbers or strings that should be named constants. |
| 96 | + - Functions doing too many things (violating single responsibility). |
| 97 | + |
| 98 | +2. **Redundant logic:** |
| 99 | + - Duplicate code across the diff or between the diff and existing code. |
| 100 | + - Conditions that are always true/false. |
| 101 | + - Unnecessary intermediate variables or re-assignments. |
| 102 | + - Dead code (unreachable branches, unused imports, unused variables). |
| 103 | + - Over-engineered abstractions for simple operations. |
| 104 | + |
| 105 | +Report each finding with file path, line number(s), what the issue is, and a concrete suggestion for improvement. |
| 106 | + |
| 107 | +## Step 6: Bug and security review |
| 108 | + |
| 109 | +Scan changes for bugs and security concerns, with special attention to this project's security model (read-only tools, SSH key auth, input validation): |
| 110 | + |
| 111 | +1. **Bugs:** |
| 112 | + - Incorrect exception handling (swallowing errors, catching too broadly). |
| 113 | + - Resource leaks (unclosed connections, file handles). |
| 114 | + - Async issues (missing `await`, unawaited coroutines). |
| 115 | + - Incorrect use of mutable default arguments. |
| 116 | + |
| 117 | +2. **Security (OWASP + project-specific):** |
| 118 | + - **Command injection** — Any user input reaching shell commands without validation. Check against the project's `commands.py` allowlist pattern. |
| 119 | + - **Path traversal** — File path parameters that aren't validated against allowlists (see `ALLOWED_LOG_PATHS`, `MAX_FILE_READ_BYTES`). |
| 120 | + - **Information disclosure** — Sensitive data in logs, error messages, or tool outputs. Check that `audit.py` redaction covers new fields. |
| 121 | + - **SSH security** — Any weakening of host key verification, key-based auth, or connection pooling. |
| 122 | + - **Read-only violation** — Any tool missing `readOnlyHint=True` or performing writes without going through the gatekeeper. |
| 123 | + - **Input validation** — Missing or insufficient validation on tool parameters (see `utils/validation.py`). |
| 124 | + - **Dependency risks** — New dependencies that are unmaintained, have known CVEs, or pull in excessive transitive deps. |
| 125 | + |
| 126 | +Rate each finding: |
| 127 | +- **critical** — Exploitable vulnerability or guaranteed bug. Must fix before merge. |
| 128 | +- **warning** — Potential issue depending on context. Should fix or explicitly justify. |
| 129 | +- **info** — Defense-in-depth suggestion or minor hardening opportunity. |
| 130 | + |
| 131 | +## Step 7: Run tests |
| 132 | + |
| 133 | +Run `make test` on the PR branch to verify all tests pass. |
| 134 | + |
| 135 | +Report results: |
| 136 | +- **Pass** — all tests green, with coverage summary if printed. |
| 137 | +- **Fail** — list failing tests with their error messages. Note whether failures are related to the PR changes or pre-existing. |
| 138 | + |
| 139 | +## Step 8: Restore original branch and present summary |
| 140 | + |
| 141 | +Check out the original branch that was active before the review began. |
| 142 | + |
| 143 | +Present the full review in this format: |
| 144 | + |
| 145 | +``` |
| 146 | +## PR Review: #<number> — <title> |
| 147 | +**Branch:** <head> → <base> |
| 148 | +
|
| 149 | +### Format & Lint |
| 150 | +<results from Step 2> |
| 151 | +
|
| 152 | +### Type Check |
| 153 | +<results from Step 3> |
| 154 | +
|
| 155 | +### Tests |
| 156 | +<results from Step 7> |
| 157 | +
|
| 158 | +### Semantic Issues |
| 159 | +<findings from Step 4, ordered by severity> |
| 160 | +
|
| 161 | +### Readability & Redundancy |
| 162 | +<findings from Step 5> |
| 163 | +
|
| 164 | +### Bugs & Security |
| 165 | +<findings from Step 6, ordered by severity> |
| 166 | +
|
| 167 | +### Verdict |
| 168 | +
|
| 169 | +**<APPROVE | REQUEST CHANGES | NEEDS DISCUSSION>** |
| 170 | +
|
| 171 | +<1-3 sentence summary of the overall state of the PR. Call out the most important finding if any.> |
| 172 | +``` |
| 173 | + |
| 174 | +Verdict criteria: |
| 175 | +- **APPROVE** — No critical or warning findings. All checks pass. |
| 176 | +- **REQUEST CHANGES** — Any critical finding, or 3+ warnings. |
| 177 | +- **NEEDS DISCUSSION** — Warnings that involve design decisions or tradeoffs the author should weigh in on. |
| 178 | + |
| 179 | +After presenting the review, ask the user if they'd like to: |
| 180 | +1. Fix the reported issues (offer to help with specific fixes). |
| 181 | +2. Re-run the review after making changes. |
| 182 | +3. Discuss any specific finding in more detail. |
0 commit comments