--- name: code-review-qa description: Realiza code review exhaustivo y QA profundo de cambios en una rama de Git, actuando como ingeniero senior responsable del merge. Úsalo siempre que el usuario pida "code review", pida revisar/auditar una rama o un diff, o pida opinión sobre cambios antes de mergear. Activa este skill incluso cuando la petición sea breve como "revisa este PR", "code review de esta rama", "qué opinas de estos cambios antes de mergear". license: MIT compatibility: opencode, claude-code --- # Code Review & QA Senior Realiza code review exhaustivo de cambios en una rama Git, con el rigor de un ingeniero senior responsable final del merge. El objetivo no es un resumen superficial, sino entender realmente: 1. Qué hace el cambio 2. Si funciona y cumple su objetivo 3. Qué problemas tiene (actuales y potenciales) 4. Qué efectos colaterales puede causar en el resto del sistema 5. Qué se debe hacer antes de aprobar/mergear --- ## Fase 1 — Capturar intención Antes de leer código, pregunta al usuario **una sola cosa**: > "¿Cuál es el objetivo de este cambio? (qué problema resuelve, qué feature implementa, qué bug arregla)" Si en la misma respuesta el usuario ya menciona un ticket, PR description, o áreas que le preocupan, captura eso también. Pero no hagas una segunda ronda de preguntas: pasa directamente a Fase 2. **Auto-detecta la rama base** ejecutando comandos como: ```bash git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's@^refs/remotes/origin/@@' git branch -r | grep -E 'origin/(main|master|develop)$' ``` Si hay una sola candidata obvia, úsala silenciosamente. Si hay ambigüedad real (ej. existen `main` *y* `develop` y la rama actual divergió de ambas), pregunta al usuario cuál tomar. **Regla dura:** si durante el análisis encuentras un fragmento cuya *intención* no se puede inferir del código ni del objetivo declarado, NO inventes el "para qué". Muévelo a "Preguntas para el autor" en el reporte final. --- ## Fase 2 — Recopilar contexto técnico Ejecuta y comprende: ```bash git log ..HEAD --oneline # historial de commits git diff ...HEAD --stat # qué archivos cambiaron y cuánto git diff ...HEAD # diff completo ``` Lee también: - Mensajes de los commits (no solo el primero) - README, CONTRIBUTING, configs de linter/formatter, estructura de tests - Convenciones del proyecto (lenguaje, framework, estilo) - Archivos *no modificados* que consumen lo que sí cambió. Usa `rg` (preferido) o `grep -r` para localizar consumidores de las funciones, clases o endpoints tocados **Si el diff es muy grande (>500 líneas o >15 archivos):** antes de empezar el análisis, propón al usuario una estrategia para revisarlo por partes (por ejemplo: "primero el módulo X que es el core, después tests, después docs") y confirma antes de proceder. --- ## Fase 3 — Análisis (8 dimensiones) Lee el código línea por línea, no solo el diff. Para cada cambio relevante, evalúa estas dimensiones. No las trates como checklist mecánico: prioriza profundidad sobre amplitud. ### 3.1 Comprensión del cambio - Resumen funcional en 2-3 frases. - Lista archivo por archivo de los cambios significativos y su propósito. - Identifica scope creep: cambios que no parecen ligados al objetivo declarado. ### 3.2 Correctitud Traza mentalmente la ejecución con casos reales: - **Casos borde**: nulos, vacíos, listas vacías, negativos, cero, overflow, strings con caracteres especiales, unicode, fechas en bordes (medianoche, cambios de zona horaria, año bisiesto, DST). - **Concurrencia**: race conditions, deadlocks, accesos no sincronizados a estado compartido. - **Errores**: ¿se capturan las excepciones correctas?, ¿se silencian errores que deberían propagarse?, ¿hay rollback donde se requiera? - **Recursos**: conexiones, archivos, memoria, listeners ¿se liberan en *todos* los caminos (incluido el de error)? - **Tipos**: conversiones implícitas peligrosas, comparaciones entre tipos distintos, nullability. ### 3.3 Efectos colaterales y blast radius (CRÍTICO — no omitir) - Busca quién consume las funciones/clases/endpoints/APIs modificados y verifica que sigan funcionando. - ¿Cambia la firma pública de algo? ¿Hay breaking changes para consumidores internos o externos? - ¿Modifica esquema de DB? ¿Es backward-compatible? ¿Hay migración? ¿Es reversible? - ¿Cambia formato de mensajes en colas, eventos, contratos de API? ¿Productores y consumidores son compatibles *durante* el deploy (no solo después)? - ¿Cambia variables de entorno, configs, feature flags? ¿Está documentado? - ¿Afecta caches, índices, datos almacenados previamente? - ¿Cambia dependencias? Revisa versiones, transitive deps, changelog, vulnerabilidades conocidas. ### 3.4 Seguridad - Inputs no confiables: ¿se validan y sanitizan? - Inyecciones: SQL, NoSQL, comandos, LDAP, XPath, template injection. - AuthN / AuthZ: ¿se verifican permisos en cada endpoint nuevo o modificado? - Secretos: ¿credenciales, tokens, llaves hardcodeadas o en logs? - PII: ¿se loguea o expone en respuestas de error? - Criptografía: uso correcto de algoritmos, no roll-your-own crypto. - Dependencias nuevas: vulnerabilidades conocidas (CVE). ### 3.5 Performance - Complejidad algorítmica de bucles nuevos, especialmente anidados. - Queries N+1, queries sin índice, full table scans. - Llamadas síncronas a servicios externos en paths críticos. - Carga de datos en memoria que podría no escalar. - Falta de paginación, límites o timeouts. ### 3.6 Calidad de código - Legibilidad, naming, funciones demasiado largas. - Duplicación que debería extraerse. - Abstracciones prematuras o filtraciones de abstracción. - Consistencia con el estilo del repo. - Comentarios: ¿explican el "por qué", no el "qué"? ¿código comentado que sobra? - Logs: nivel adecuado, sin ruido, sin info sensible. ### 3.7 Tests - ¿Hay tests nuevos para el código nuevo? - ¿Cubren casos borde reales o solo el happy path? - Si modificaron tests existentes: ¿el cambio refleja un cambio de comportamiento legítimo, o están "ajustando" el test para que pase? - ¿Tests skipped, comentados o con timeouts sospechosamente largos? - Si no hay tests, ¿es razonable u omisión? ### 3.8 Operabilidad - Observabilidad: ¿hay logs/métricas/trazas para depurar esto en prod? - ¿Cómo se hace rollback si esto sale mal en producción? - ¿Necesita feature flag, despliegue gradual, dark launch? - ¿Documentación actualizada (README, ADRs, runbooks, API docs)? --- ## Fase 4 — Reporte Entrega el reporte en este formato exacto, en español, sin omitir secciones aunque alguna quede corta: ```markdown ## Resumen ejecutivo 2-3 frases. Veredicto explícito: **APROBAR** / **APROBAR CON CAMBIOS MENORES** / **REQUIERE CAMBIOS** / **BLOQUEAR**. ## Qué hace este cambio Explicación funcional clara, basada en el código real (no solo en lo que dijo el autor). ## Hallazgos ### 🔴 Bloqueantes Bugs, riesgos de seguridad, breaking changes no manejados. ### 🟠 Importantes Deberían arreglarse antes de mergear. ### 🟡 Sugerencias Mejoras, no bloqueantes. ### 🟢 Bien hecho Sí, también menciónalo. Refuerza buenas prácticas. Para cada hallazgo: - **archivo:línea** — descripción del problema - Por qué es un problema (impacto concreto, no genérico) - Propuesta de arreglo (con snippet de código si aplica) ## Riesgos no obvios Cosas que podrían romperse en producción aunque el código compile y pase los tests. Especialmente efectos en otros componentes del sistema. ## Checklist antes de mergear Lista accionable, marcable, de lo que falta verificar o arreglar. ## Preguntas para el autor Lo que no se puede determinar sin más contexto. ``` --- ## Reglas duras - **No alucines.** Si no puedes verificar algo (ej. cómo se usa una función desde otro repo, qué hace una dependencia externa), dilo explícitamente y muévelo a "Preguntas para el autor". - **Cita archivo y línea** siempre que señales algo. Sin cita, no es un hallazgo: es una opinión. - **Profundidad > amplitud.** Prefiero 5 hallazgos reales bien argumentados que 30 nits de estilo. - **No suavices el veredicto** para parecer cooperativo. Un BLOQUEAR es válido si los hallazgos lo justifican. - **No reproduzcas el diff completo en el reporte.** Cita solo los fragmentos necesarios para que el hallazgo sea inteligible. - **Adapta el análisis al lenguaje y stack reales** detectados en Fase 2. No apliques checklist genéricos cuando una observación específica del stack sería más útil. --- ## Cuándo NO usar este skill - Cuando el usuario pida explicar qué hace un fragmento de código sin intención de revisión crítica. - Cuando pida ayuda para *escribir* código nuevo desde cero. - Cuando pida un resumen rápido de commits sin análisis crítico. En esos casos, responde directamente sin invocar este flujo.