# Review Pr

> Review pull requests thoroughly, then deliver a short GitHub-ready comment (2-4 lines) plus optional detailed notes only when asked. Use when the user asks for a PR review, code review on a pull request link, or review feedback to post on GitHub. Also handles security-focused PR reviews when the user asks to "check for bad practices", "security review this PR", or "scan the diff for vulnerabilities" — in that case follow the Security lens section.

- Skill: `miguelpecegueiro/review-pr` (Agent Skill)
- Install (CLI): `npx skillmds@latest add miguelpecegueiro/review-pr`
- Raw SKILL.md: https://api.skillmd.com/api/skills/miguelpecegueiro/review-pr/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Security
- Author: MiguelPecegueiro (https://skillmd.com/u/miguelpecegueiro)
- Updated: 2026-09-22
- Page: https://skillmd.com/skills/miguelpecegueiro/review-pr

---


# PR Review

## Goal

Do a real review (read diff, compare repo patterns, check commits/CI).
Deliver a short comment for GitHub — not an essay.

## Workflow

1. Fetch PR: `gh pr view`, `gh pr diff`, commits; compare with similar code in the repo.
2. Decide: **Approve**, **Approve with nits**, or **Request changes** (only for real blockers).
3. Output in two layers (see below). Default to Layer 1 only.

---

## Output layers

### Layer 1 — GitHub comment (default)

2–4 lines max. What the reviewer will actually paste.

Template:

```markdown
**[Approved | Request changes].** [One sentence: what the PR does + verdict.]

[Optional: one concrete nit or pre-merge check, only if it matters.]
```

**Approve example:**

```markdown
**Approved.** Matches the other wallboards; schema fix in the last commit looks good.

Optional: note hub URL and what triggers `POST /update` in the PR description.
```

**Request changes example:**

```markdown
**Request changes.** Service queries `Foo` but the view is `schema.Foo` — will 500 in prod.

Fix the SQL schema prefix, then good to merge.
```

### Layer 2 — Detailed notes (only if asked)

Use when the user says "full review", "detailed", or "write-up".
Structure: Summary → What looks good → Blockers/nits → Pre-merge checklist → Verdict.
Still skip filler; no tables unless they add clarity.

---

## Security lens (Layer 3)

Activate when the user asks for a security review, vulnerability scan, or bad practice check.
Run this in addition to the standard review, not instead of it.

Check the diff for:

**Critical — block the PR:**
- Credentials, secrets, connection strings, API keys, tokens committed in any file
- SQL built by string concatenation (SQL injection)
- Auth bypass or missing auth on any new endpoint or route
- PII logged to console, file, or Elastic without masking
- New external HTTP calls without input validation (SSRF surface)
- Hardcoded internal IPs, hostnames, or environment-specific values

**High — flag and require fix:**
- Missing input validation on any user-controlled value reaching the DB or filesystem
- New dependency added without a security review note
- Exception swallowed silently (empty catch block) on a security-relevant operation
- CORS misconfiguration on new API endpoints
- JWT or token validation weakened or skipped

**Medium — flag, judgment call:**
- Overly broad exception handlers masking real errors
- Missing audit log entry for a state-changing operation (ISO 27001 relevance)
- Config values that should be in Key Vault but are in appsettings.json
- Dead code left in that contains credential patterns (even if commented out)

**Out of scope for this review:**
- Architectural decisions that were locked in a grill-me session
- Issues already tracked in Asana or Confluence as known debt
- Style preferences not covered by the global Cursor rules

For each finding: file name, line number, severity, one-line explanation, one-line fix.
Do not reproduce credential values — reference by file and line only.

---

## GoConnection context

- ISO 27001 active — security findings are not optional suggestions
- All secrets must be in Azure Key Vault (`goconnection-keyvault`, RBAC mode)
- SQL Server schemas: `Clientes.*`, `GoConnection.*`, `OCGoConnection.*` via linked server
- Deploy target: SRVGOB (IIS) — no container or cloud-native patterns expected
- Multi-tenant risk: changes to GC_OneAgentScripts affect all clients — flag blast radius

---

## Rules

- Never approve a PR with a Critical security finding
- Never approve a PR where CI is failing unless the failure is demonstrably unrelated
- If a 2MB+ binary is in the diff, flag it — it probably should not be in source control
- Do not write long essays by default — Layer 1 is the default output
- Do not flag things that are clearly intentional and consistent with existing patterns