devx/.devin/skills/pr-review/SKILL.md

pr-review

Deep, critical PR review with auto-fix. This skill guides the agent through a thorough review of a pull request, posting inline comments for each issue found, auto-fixing them, resolving the discussion threads, and marking the PR as ready-to-merge when no blocking issues remain.

When to Invoke

Invoke this skill when asked to review a PR, or when a PR is open and needs review before merge. Do NOT invoke automatically on every PR — this is an on-demand deep review, not a CI gate.

Prerequisites

• The PR must be open in a Gitea repo
• The agent needs Gitea MCP access (gitea server)
• The agent needs git push access to the PR's head branch
• The PR should have passed CI (validate job) before deep review

Review Categories

Review every PR against these 8 categories. For each issue found, post an inline comment on the specific line, then auto-fix it.

1. Functional Correctness

• Does the code actually do what the spec/PR title claims?
• Are edge cases handled? (empty input, null, boundary values, concurrent access)
• Are error paths tested? Not just happy path.
• Does the code handle all return values? (ignored errors, unchecked None)
• Are there off-by-one errors, wrong comparisons, inverted conditions?
• Do loops terminate correctly? (no infinite loops, correct break/continue)
• Are regex patterns correct? (anchored, escaped, non-greedy where needed)
• Are API responses validated before use? (status codes, response shape)

2. Completeness

• Are all requirements from the spec implemented? (check each REQ-ID)
• Are all acceptance criteria in the spec checked off?
• Are tests written for all new code paths?
• Are error messages user-facing (wrapped in _())?
• Are new CLI commands documented in docs/user/cli-commands.md?
• Are new modules added to architecture docs?
• Are CHANGELOG entries added for user-facing changes?
• Are translations added for new user-facing strings?

3. Architecture

• Does the code follow the repo's layer separation? (no business logic in CLI, no direct subprocess in CLI)
• Are new dependencies justified? (no unnecessary new packages)
• Is configuration via env vars / config.py, not hardcoded?
• Are new modules placed in the correct directory? (ci/ vs tools/ vs molecule/)
• Does the code reuse existing utilities? (no reimplemented helpers)
• Are imports circular? (check import chains)
• Is the code testable? (injectable dependencies, no hidden global state)
• Does the code follow existing patterns in the codebase?

4. Reliability

• Are external API calls retried with backoff?
• Are timeouts set on all network operations?
• Are file operations atomic? (write to temp, rename)
• Are database operations transactional where needed?
• Are there race conditions? (check shared mutable state)
• Are resources cleaned up in all paths? (finally blocks, context managers)
• Can the code handle partial failures? (one service down, others up)
• Are idempotency guarantees maintained? (safe to retry)

5. Robustness

• Does the code fail gracefully? (meaningful error messages, not stack traces)
• Are unexpected inputs handled? (type checking, validation)
• Are there any crash-on-bad-input paths?
• Does the code degrade under load? (backpressure, queue limits)
• Are there resource leaks? (file handles, connections, memory)
• Does the code survive network partitions? (retry, circuit breaker)
• Are there any unhandled exceptions that could crash the process?
• Is logging sufficient to diagnose production issues?

6. Security

• Are there hardcoded secrets, tokens, or passwords?
• Is shell=True used with user input? (command injection)
• Is eval() or exec() used? (code injection)
• Are SQL queries parameterized? (no string concatenation)
• Are file paths validated? (no path traversal)
• Are user inputs sanitized before display? (XSS in web contexts)
• Are SSL/TLS verifications disabled without justification?
• Are secrets logged in error messages or debug output?
• Are permissions checked before privileged operations?
• Is sensitive data in memory longer than necessary?

7. Technical Excellence

• Are functions under 50 lines? (refactor if longer)
• Is cyclomatic complexity reasonable? (no deeply nested if/else chains)
• Are names meaningful? (no single-letter vars, no misleading names)
• Is dead code removed? (no commented-out blocks, no unused imports)
• Are comments explaining WHY, not WHAT?
• Is the code DRY? (no copy-pasted blocks that should be shared)
• Is the code SOLID? (single responsibility, open/closed)
• Are magic numbers extracted to named constants?
• Is the code formatted per the repo's linter config?
• Are type hints present on all function signatures?

8. Test Quality

• Do tests actually test the behavior? (not just that code runs)
• Are tests independent? (no shared mutable state, no order dependency)
• Are tests fast? (no real sleeps, no real network calls, mocked)
• Are edge cases tested? (empty, None, boundary, error paths)
• Are test names descriptive? (test_what_condition_expected_result)
• Are mocks set up correctly? (mocking the right object, not too broad)
• Is coverage 100% for new code? (every branch, every line)
• Are integration tests added for cross-module changes?
• Do tests clean up after themselves? (tmp_path, fixtures)

Review Procedure

Step 1: Gather Context

1. Read the PR spec (if exists): docs/specs/<TASK-ID>.md
2. Fetch PR details via Gitea MCP: pull_request_read (get_pr, list_pr_files)
3. Read the full diff: git diff origin/master...HEAD
4. Read the PR description and any existing review comments
5. Identify the repo's task prefix (OBL-INFRA, GRM, SSO, DEVX)

Step 2: Review Each File

For each changed file in the PR:

1. Read the full file (not just the diff) to understand context
2. Go through all 8 review categories
3. For each issue found, note: file path, line number, category, severity, description, suggested fix

Step 3: Post Inline Comments

For each issue found, post an inline review comment using the Gitea MCP:

mcp_call_tool: gitea / pull_request_review_write
  method: create
  owner: <owner>
  repo: <repo>
  pull_number: <PR number>
  state: PENDING  (accumulate comments before submitting)
  body: ""  (empty for now, summary added on submit)
  comments: [
    {
      path: "<file path>",
      new_line_num: <line number>,
      body: "**[<category>] [<severity>]** <description>\n\n**Suggested fix:**\n```<lang>\n<fixed code>\n```"
    }
  ]

Comment format:

**[Security] [error]** `shell=True` used with user input — command injection risk.

**Suggested fix:**
```python
subprocess.run(["git", "log", commit], check=True)

Severity levels:
- `error` — must fix before merge (security, correctness, crash)
- `warning` — should fix before merge (reliability, best practice)
- `info` — consider fixing (style, minor improvement)

### Step 4: Auto-Fix Issues

For each issue that can be safely auto-fixed:

1. Edit the file using the `edit` tool
2. Commit with message: `fix: address review comment — <short description>`
3. Push to the PR's head branch: `git push origin HEAD`
4. Wait for CI to re-run on the push

Auto-fix ALL issues unless:
- The fix requires an architectural decision (ask the user)
- The fix changes public API behavior (ask the user)
- The fix is ambiguous (multiple valid approaches, ask the user)

### Step 5: Resolve Discussion Threads

After auto-fixing an issue and CI passes:

1. Find the review comment thread for that issue
2. Post a reply: `Fixed in <commit-sha>. Closing this thread.`
3. Resolve the discussion (if Gitea supports it via API)
4. If resolving via API is not available, the reply comment serves as resolution

### Step 6: Submit Final Review

After all issues are addressed (fixed or discussed):

mcp_call_tool: gitea / pull_request_review_write method: submit owner: repo: pull_number: review_id: <from step 3 create> state: COMMENT (or APPROVED if no blocking issues remain) body: <summary — see below>

### Step 7: Post Summary

Post a brief summary as a PR comment (via `issue_write / add_comment`):

Deep Review Summary

• Files reviewed: N
• Issues found: N (N auto-fixed, N require attention)
• Categories: security (N), correctness (N), architecture (N), ...

Outcome: ✅ Ready to merge — all issues addressed. OR Outcome: ⚠️ N blocking issue(s) remain — see inline comments.

Keep the summary to 5-10 bullet points. Do not paste the full review.

### Step 8: Mark PR Ready

If all issues are addressed and no blocking issues remain:

mcp_call_tool: gitea / issue_write method: add_labels owner: repo: issue_number: labels: [<label_id for "ready-to-merge">]

If blocking issues remain, do NOT add the label. Post a comment
explaining what needs to be resolved before the PR can merge.

## Gitea MCP Tools Reference

| Action | MCP tool | Method |
|--------|----------|--------|
| Get PR details | `pull_request_read` | `get_pr` |
| List PR files | `pull_request_read` | `list_pr_files` |
| Get PR diff | `pull_request_read` | `get_pr_diff` |
| Create review (pending) | `pull_request_review_write` | `create` (state: PENDING) |
| Submit review | `pull_request_review_write` | `submit` (state: APPROVED/COMMENT/REQUEST_CHANGES) |
| Post PR comment | `issue_write` | `add_comment` |
| Add label | `issue_write` | `add_labels` |
| List labels | `label_read` | `list_repo_labels` |
| Merge PR | `pull_request_write` | `merge` (do NOT use — auto-merge handles this) |

## Important Rules

- **Never merge the PR yourself.** Add the `ready-to-merge` label and let
  the auto-merge workflow handle it. This ensures CI passes and the
  commit message follows the `<PREFIX>-N: <conventional>` format.
- **Never approve your own PR.** If the agent created the PR, post
  COMMENT state, not APPROVED.
- **Always push fixes to the PR branch**, not directly to master.
- **Wait for CI after each push** before resolving the discussion thread.
- **Post one review with all comments**, not multiple reviews.
- **The summary must be brief** — 5-10 bullet points max.
- **Severity matters**: only `error` severity blocks the `ready-to-merge` label.