The whole file
This is exactly what you download: no summaries, nothing trimmed.
Heads-up: the skill file itself is written in Spanish. Agents read it fine and answer in your language, but the prose below is not translated.
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
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:
# ¿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.
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
# 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
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.