Revisar más no siempre sirve

Nico BelloHead de Ingeniería · Qurable

Con agentes, una revisión de código cuesta minutos y la tentación es pedir siempre una más. En un fix de concurrencia el equipo corrió siete revisiones en tres rondas, y cada ronda encontró errores introducidos por la anterior. Lo que terminó de medir la calidad del cambio fue romper el código a propósito.

Un fix de concurrencia en un proceso de sincronización pasó por siete revisiones en contexto limpio, repartidas en tres rondas. Cada revisión la hizo un agente que leía el diff sin haber visto la conversación en la que se escribió el cambio. Las tres rondas encontraron problemas reales, y en las tres la mayoría de esos problemas había nacido en los arreglos de la ronda anterior.

El cambio era acotado. Un proceso que vuelca los resultados de una consulta hacia otro almacenamiento decidía si una tarea ya estaba en curso mirando un contador que solo avanzaba cuando el consumidor confirmaba un lote. Mientras el productor preparaba los datos, el contador seguía en cero, y cada ejecución periódica concluía que nadie se estaba ocupando de la tarea y arrancaba otra vez desde el principio. En una sola noche, el mismo trabajo llegó a repetirse más de veinte veces. La corrección agregaba un reclamo explícito: un productor toma la tarea, la renueva mientras avanza y la libera al terminar. Eso cambiaba un invariante del sistema, el orden en que ciertas cosas tienen que quedar registradas antes de que otras ocurran.

Este post trata de lo que pasó durante la revisión de ese cambio, más que del bug.

La primera ronda fue la más productiva. Encontró un reclamo que se tomaba en un lugar y se liberaba en otro, de modo que cinco caminos de error podían dejarlo tomado hasta que expirara. También encontró una transición de estado capaz de marcar como fallida una tarea recién completada, lo que provocaba justo la reconstrucción completa que el cambio quería evitar. Eran bugs que habrían llegado a producción.

La segunda ronda se pidió con un foco explícito: buscar defectos introducidos por los arreglos de la primera. Encontró varios. Uno de ellos era una renovación del reclamo que se había movido “a cada fila” y no cubría las filas que se salteaban, que era precisamente uno de los casos que el comentario nuevo decía cubrir. Las revisiones seguían encontrando cosas, pero lo que encontraban empezaba a ser de otro tipo.

Desde la segunda ronda, buena parte de los hallazgos no estaba en el código, sino en la prosa que lo describía: comentarios, documentación, el encabezado de la migración. Una sección hablaba de “tres columnas” sobre una tabla de cuatro filas. Un comentario de configuración nombraba la clase equivocada.

La explicación es sencilla. Un fix de concurrencia cambia qué es verdad sobre el sistema, y en un repositorio hay muchos lugares que afirman cosas sobre el sistema. Cada ronda de arreglos edita algunos y deja los demás como estaban.

El código tiene tests que lo cubren, mejores o peores; la prosa no tiene ninguno.

El caso más serio apareció en la tercera ronda. El repositorio tiene un catálogo de funcionalidades que la guía de revisión designa como fuente de verdad, el archivo que se supone que hay que leer antes de tocar una feature. Nadie lo había modificado en toda la rama, y seguía afirmando que cierto valor tenía que estar registrado antes de despachar los lotes, que era exactamente lo que el PR había invertido.

Ese documento, en ese estado, invitaba a reabrir el bug. Alguien que llegara meses después, leyera el catálogo y viera que el código “no cumplía” con lo documentado, tendría motivos de sobra para “corregirlo”, y con eso devolvería el sistema al estado en que repetía el trabajo.

De ahí salió una práctica. Después de cada ronda de arreglos, revisar los archivos tocados no alcanza: hay que buscar en todo el repositorio la huella de la afirmación que se corrigió (la frase, el nombre del campo, el orden de los pasos) y mirar cada lugar donde todavía aparece. La pregunta útil es qué otros lugares siguen diciendo lo contrario de lo que ahora es cierto.

Con revisores humanos, el costo pone el límite. Nadie le pide a un compañero una cuarta revisión del mismo PR sin una buena razón. Con agentes ese freno desaparece: una revisión en contexto limpio cuesta unos minutos y unos centavos, y siempre devuelve algo.

Cada lectura adicional encontraba menos, y lo que encontraba se parecía cada vez más a ruido generado por las rondas anteriores. Al mismo tiempo, cada lectura disparaba una nueva tanda de arreglos, con sus propios errores.

Pasado cierto punto, revisar otra vez no acercaba el cambio a estar terminado; lo mantenía en movimiento.

Hubo además un efecto lateral. Algunos revisores leían una versión anterior a las últimas correcciones y reportaban como pendientes cosas ya resueltas. En una de las pasadas, once de veinticinco hallazgos no resistieron una verificación posterior. Por eso, ante cada hallazgo, lo primero es confirmar que el problema existe en la versión actual. Arreglar dos veces lo mismo es una de las formas más directas de meter un bug nuevo.

Los revisores no eran el problema. Una lectura, por cuidadosa que sea, responde si el código parece correcto, y después de dos rondas el código parecía correcto. Lo que faltaba saber era si los tests se darían cuenta en caso de que no lo fuera.

Para esa pregunta existe el mutation testing. Se introduce un defecto deliberado en el código (se invierte una condición, se elimina un guard, se cambia un throw por un log, se reordenan dos escrituras) y se corre la suite. Si algún test falla, la mutación “muere”: ese comportamiento está protegido. Si la suite pasa con el defecto puesto, la mutación “sobrevive”, y ese comportamiento no tiene protección, diga lo que diga el reporte de cobertura.

La primera campaña aplicó doce mutaciones y sobrevivieron siete, sobre un cambio que ya había pasado varias revisiones.

Los tests existían, ejecutaban esas líneas y sumaban cobertura. Lo que no hacían era fallar cuando el código estaba mal.

El equipo cerró esas brechas y corrió una segunda campaña, más amplia, de treinta y ocho mutaciones. Sobrevivieron seis nuevas, y dos de ellas reproducían el modo de falla que el PR venía a corregir: una tarea podía darse por terminada con un tamaño de resultado que nunca se había guardado de forma durable, lo que habría publicado un resultado truncado. Ninguna de las siete lecturas lo había señalado, y tenía sentido que así fuera: el código hacía lo correcto. Lo que faltaba era un test que comprobara que lo siguiera haciendo.

Las mutaciones sobrevivientes repetían patrones que aparecen en cualquier suite.

El más común es verificar que una función se llamó sin mirar con qué argumentos. Un test que comprueba que se invocó la actualización de estado pasa igual si se invoca con la condición correcta o sin ninguna, y en un cambio de concurrencia la condición es lo que importa.

// Pasa con el bug puesto:
expect(repo.updateStatus).toHaveBeenCalled();

// Falla si la escritura deja de ser condicional:
expect(repo.updateStatus).toHaveBeenCalledWith(
  jobId, FAILED, { expectedStatus: RUNNING },
);

Una variante es usar toHaveBeenCalled() cuando la función se invoca desde dos lugares y solo uno importa para el caso. El test queda satisfecho con la llamada equivocada.

El tercer patrón cuesta más verlo. Un mock compartido devolvía siempre una fila válida y ningún test sobrescribía ese valor. Cada guard del tipo “si la escritura no devolvió nada, abortar” quedaba sin cubrir: se lo podía borrar o convertir en una advertencia y la suite seguía en verde.

Un mock que siempre devuelve algo razonable decide de antemano qué ramas no se van a ejercitar.

Y una práctica que conviene no saltear: cuando se escribe un test para matar una mutación, hay que comprobar que la mata. Se vuelve a aplicar la mutación, se corre la suite, se confirma que falla y recién entonces se restaura. Un test escrito “para este caso” puede pasar con el bug puesto por cualquiera de los motivos anteriores, y sin esa comprobación no vale mucho más que un comentario.

Las revisiones siguen siendo necesarias. Las dos primeras rondas encontraron bugs serios que ningún test habría marcado, y leer en contexto limpio sigue siendo la mejor forma de detectar un diseño equivocado o un caso no contemplado.

Cada instrumento mide algo distinto, y repetir una medición no la hace más precisa.

Una revisión mide si el cambio se entiende y parece correcto. Una campaña de mutación mide si la suite va a sostener ese cambio cuando otra persona, u otro agente, lo toque sin conocer la historia. A partir de la segunda ronda, la segunda pregunta pesa más que la primera.

Con IA esto importa más que antes. Cuando revisar no cuesta casi nada, deja de tener sentido preguntarse si alcanza el tiempo para otra revisión, y pasa a importar si otra revisión es lo que el cambio necesita en ese momento. Pedir una más es fácil y da la sensación de estar avanzando. Notar que el instrumento dejó de rendir y cambiarlo por otro es una decisión de criterio, y ese criterio sigue estando del lado del ingeniero.

La forma de trabajo que quedó es esta: una o dos rondas de revisión en contexto limpio; después de cada ronda de arreglos, una búsqueda de la afirmación corregida en toda la documentación; y desde ahí, mutaciones reales en lugar de más lecturas, con cada test nuevo verificado contra la mutación que dice cubrir. Lleva más trabajo que pedir otra revisión, y es lo que muestra si los tests protegen el cambio o solo pasan por sus líneas.