/plugin marketplace add https://jesusprodriguez.com/skills/marketplace.json
/plugin install ingenieria@jprodriguez-toolkit

curl -fsSL https://jesusprodriguez.com/skills/github-pr-review/SKILL.md --create-dirs -o .claude/skills/github-pr-review/SKILL.md

New-Item -ItemType Directory -Force ".claude/skills/github-pr-review" | Out-Null
Invoke-WebRequest "https://jesusprodriguez.com/skills/github-pr-review/SKILL.md" -OutFile ".claude/skills/github-pr-review/SKILL.md"

curl -fsSL https://jesusprodriguez.com/skills/install.sh -o install.sh
sh install.sh github-pr-review

PR=123
gh pr view $PR --json title,body,labels,milestone
gh pr checks $PR                 # si la CI está roja, no revises todavía
gh pr diff $PR --name-only
gh pr diff $PR > /tmp/pr-$PR.diff

# ¿Quién importa esto?
grep -rl "from ['\"].*modulo-cambiado" src/ --include="*.ts"

# ¿Cruza la frontera de un servicio?
gh pr diff $PR --name-only | cut -d/ -f1-2 | sort -u

# ¿Toca contratos compartidos?
gh pr diff $PR --name-only | grep -E "types/|interfaces/|schemas/|models/"

D=/tmp/pr-$PR.diff

grep -nE "(password|secret|api_key|token|private_key)\s*=\s*['\"][^'\"]{8,}" $D
grep -nE "AKIA[0-9A-Z]{16}" $D                      # claves de AWS
grep -n  "dangerouslySetInnerHTML\|innerHTML\s*=" $D # XSS
grep -nE "\beval\(|\bexec\(" $D
grep -nE "md5\(|sha1\(" $D                          # hash inseguro
grep -nE "path\.join\(.*req\.|readFile\(.*req\." $D  # path traversal
grep -n  "query\|execute\|raw(" $D | grep -E '\$\{|f"|%s'  # SQL por interpolación

# Rutas o tipos eliminados
grep "^-" $D | grep -E "router\.(get|post|put|delete|patch)\(|^-\s*(export\s+)?(interface|type) "

# Migraciones destructivas
grep -E "DROP TABLE|DROP COLUMN|ALTER.*NOT NULL|DROP INDEX" $D

# Variables de entorno nuevas: ¿están en producción?
grep "^+" $D | grep -oE "process\.env\.[A-Z_]+" | sort -u

gh pr diff $PR --name-only | grep -vE "\.test\.|\.spec\.|__tests__" | wc -l
gh pr diff $PR --name-only | grep -E  "\.test\.|\.spec\.|__tests__" | wc -l

Radio de impacto: ALTO — toca lib/auth, usada por 5 servicios
Seguridad: 1 hallazgo (medio)
Tests: +2% de cobertura
Rompe compatibilidad: no detectado

--- BLOQUEANTE ---
1. Inyección SQL en src/db/users.ts:42
   Interpolación directa en el WHERE.
   Arreglo: db.query("SELECT * WHERE id = $1", [userId])

--- DEBERÍA ARREGLARSE ---
2. POST /api/admin/reset sin comprobación de rol

--- SUGERENCIAS ---
3. N+1 en src/services/reports.ts:88 — findUser() dentro de un map()

--- BIEN ---
- La migración trae su down()
- El flujo nuevo de auth está bien cubierto

---
name: github-pr-review
description: Revisa una pull request de GitHub o una merge request de GitLab midiendo primero el radio de impacto - qué más se rompe si esto se rompe - y después seguridad, cambios de contrato y cobertura. Úsala antes de mergear algo que toca una librería compartida, un contrato de API o el esquema de la base de datos, o cuando una PR sea demasiado grande para leerla de arriba abajo.
version: 1.0.0
license: CC-BY-4.0
updated: 2026-08-17
---

# GitHub: revisar una pull request

Un cambio de cinco líneas en una utilidad compartida puede romper veinte
servicios. Uno de quinientas en un componente de interfaz, ninguno. **El tamaño
del diff no es la medida del riesgo**; el radio de impacto sí.

Por eso el orden de esta revisión no es el del diff: es el del riesgo.

## 1. Contexto antes que código

```bash
PR=123
gh pr view $PR --json title,body,labels,milestone
gh pr checks $PR                 # si la CI está roja, no revises todavía
gh pr diff $PR --name-only
gh pr diff $PR > /tmp/pr-$PR.diff
```

En GitLab: `glab mr view`, `glab mr diff`.

Lee la issue enlazada **antes** que el código. Sin saber qué se pretendía, la
mitad de tus comentarios serán falsos positivos sobre decisiones deliberadas.

Y mira la lista de ficheros antes de abrir ninguno: 40 ficheros con 38 de
traducciones y 2 de lógica son una PR de 2 ficheros.

## 2. Radio de impacto

Por cada fichero tocado, tres preguntas:

```bash
# ¿Quién importa esto?
grep -rl "from ['\"].*modulo-cambiado" src/ --include="*.ts"

# ¿Cruza la frontera de un servicio?
gh pr diff $PR --name-only | cut -d/ -f1-2 | sort -u

# ¿Toca contratos compartidos?
gh pr diff $PR --name-only | grep -E "types/|interfaces/|schemas/|models/"
```

| Nivel | Qué lo provoca |
|---|---|
| CRÍTICO | Librería compartida, modelo de datos, middleware de auth, contrato de API |
| ALTO | Servicio del que dependen otros, configuración compartida, variables de entorno |
| MEDIO | Cambio interno de un servicio, función de utilidad |
| BAJO | Componente de interfaz, tests, documentación |

El nivel decide cuánto miras, no si apruebas.

## 3. Seguridad

Un barrido sobre el diff encuentra lo evidente. Lo que encuentre, se verifica a
mano: son pistas, no veredictos.

```bash
D=/tmp/pr-$PR.diff

grep -nE "(password|secret|api_key|token|private_key)\s*=\s*['\"][^'\"]{8,}" $D
grep -nE "AKIA[0-9A-Z]{16}" $D                      # claves de AWS
grep -n  "dangerouslySetInnerHTML\|innerHTML\s*=" $D # XSS
grep -nE "\beval\(|\bexec\(" $D
grep -nE "md5\(|sha1\(" $D                          # hash inseguro
grep -nE "path\.join\(.*req\.|readFile\(.*req\." $D  # path traversal
grep -n  "query\|execute\|raw(" $D | grep -E '\$\{|f"|%s'  # SQL por interpolación
```

**Si aparece un secreto, el comentario no es «quítalo»: es «rótalo, ya está en el
historial».** Borrarlo en un commit posterior no lo borra de ningún sitio.

Y lo que ningún `grep` ve: secretos que se filtran por un mensaje de error o una
traza de log. Búscalos leyendo, no buscando.

## 4. Cambios que rompen a otros

```bash
# Rutas o tipos eliminados
grep "^-" $D | grep -E "router\.(get|post|put|delete|patch)\(|^-\s*(export\s+)?(interface|type) "

# Migraciones destructivas
grep -E "DROP TABLE|DROP COLUMN|ALTER.*NOT NULL|DROP INDEX" $D

# Variables de entorno nuevas: ¿están en producción?
grep "^+" $D | grep -oE "process\.env\.[A-Z_]+" | sort -u
```

Tres preguntas que se olvidan siempre:

- ¿La migración es **reversible**? ¿Bloquea la tabla mientras corre?
- ¿El despliegue admite versión antigua y nueva a la vez, o hay una ventana en la
  que la mitad de las instancias falla?
- Una configuración nueva sin valor por defecto: el entorno que no la tenga
  arranca bien y falla en la primera petición.

## 5. Cobertura

```bash
gh pr diff $PR --name-only | grep -vE "\.test\.|\.spec\.|__tests__" | wc -l
gh pr diff $PR --name-only | grep -E  "\.test\.|\.spec\.|__tests__" | wc -l
```

La pregunta no es «¿hay tests?», es: **¿existe un test que falle sin este
arreglo?** Si el bug puede volver sin que nadie se entere, la PR no está terminada.

Señales: función pública nueva sin test; tests borrados sin borrar el código que
cubrían; caminos de auth o de pagos con cobertura parcial.

## 6. El informe

```
Radio de impacto: ALTO — toca lib/auth, usada por 5 servicios
Seguridad: 1 hallazgo (medio)
Tests: +2% de cobertura
Rompe compatibilidad: no detectado

--- BLOQUEANTE ---
1. Inyección SQL en src/db/users.ts:42
   Interpolación directa en el WHERE.
   Arreglo: db.query("SELECT * WHERE id = $1", [userId])

--- DEBERÍA ARREGLARSE ---
2. POST /api/admin/reset sin comprobación de rol

--- SUGERENCIAS ---
3. N+1 en src/services/reports.ts:88 — findUser() dentro de un map()

--- BIEN ---
- La migración trae su down()
- El flujo nuevo de auth está bien cubierto
```

## Reglas de tono

- Comenta el código, nunca a la persona: «esto se rompe si X», no «no has tenido
  en cuenta X».
- Separa siempre bloqueante de sugerencia. Un revisor que no prioriza obliga al
  autor a adivinar, y adivinar mal.
- Todos los comentarios en una sola ronda. Ir soltándolos a goteo alarga la PR
  días y agota a quien la escribió.
- Si está bien, dilo y aprueba. Inventar pegas para justificar la revisión es la
  forma más rápida de que dejen de pedírtelas.
- Si una PR es demasiado grande para revisarla bien, el comentario correcto es
  «pártela», no una aprobación con la vista gorda.