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: <owner>
  repo: <repo>
  pull_number: <PR 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: <owner>
  repo: <repo>
  issue_number: <PR 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.