Nadie revisa bien su propio código, ni un agente

La idea circula bastante: si una IA escribió el código, que lo revise otra IA, porque la que lo escribió viene sesgada. El curso de subagentes de Claude Academy lo plantea así: el que escribió algo no lo revisa bien, porque lo lee con el recuerdo de haberlo escrito. Yo lo repetí en mi post anterior, y tengo casos donde funcionó.

Ver la imagen en tamaño completo

×Cerrar la imagen

Es cierto, pero incompleto. Un estudio de Microsoft sobre code review encontró que el review detecta menos defectos de lo que uno espera, y todavía menos problemas profundos, sutiles o de nivel macro. Y me quedó dando vueltas por qué.

Fui a buscar en el repo del trabajo bugs que ya se arreglaron y que tuvieran esta forma: el código se ve correcto leyéndolo línea por línea, los tests en verde, y el bug viviendo en la costura entre dos capas. Encontré tres. Los tres pasaron por review, y ninguno se encontró leyendo el diff.

Van anonimizados: nombres genéricos, sin nada del dominio.

Uno: el test que documentaba el bug#

Un proceso de ingesta guarda códigos externos con el cero a la izquierda, "012345". Otro proceso, el que busca por ese mismo código, se lo quita antes de buscar:

for line in lines:
    code = line.code
    if len(code) == 6 and code.startswith("0"):
        code = code[1:]
    item = Item.objects.filter(ext_code=code).first()
    if not item:
        log.warning("not found: %s", code)
        continue

Ningún código con cero a la izquierda encontraba nada. Cada línea se saltaba con ese continue, así que los registros se creaban, pero sin ninguna de sus líneas. El único rastro era un warning en los logs.

Lo que hace especial a este caso es el fixture del test:

Item(ext_code="12345")  # leading zero removed

No es que faltara un test. El test estaba en verde documentando el bug como si fuera lo esperado, con comentario explicándolo. Michael Feathers tiene nombre para tests así: characterization tests, que documentan lo que el sistema hace, no lo que debería hacer. El nuestro era uno sin querer. Y los demás fixtures usaban códigos no numéricos, así que el caso del cero no se probaba nunca.

El origen fue un refactor que cambió la búsqueda de un campo viejo, donde quitar el cero sí era correcto, a uno nuevo donde no lo era. Ni ese cambio ni su review lo detectaron.

Lo encontró QA. La pista estaba en los logs: el servicio externo devolvía 012345 y el log decía que no había encontrado 12345.

Qué necesitaba saber el reviewer: cómo guarda el dato la otra capa. Leyendo este diff no hay nada raro que ver. El autor, su test y cualquiera que lo revisara compartían el mismo supuesto.

Dos: el fallback que nunca corría#

Una función pide un dato remoto dentro de un try/except y, si falla, manda la línea a una lista de respaldo:

if line.has_detail:
    try:
        d = client.get_detail(line.id)
        if d:
            attach(line, d)
    except Exception as e:
        log(e)
        pending.append(line)
elif line.code:
    pending.append(line)

Se ve defensivo y correcto. El problema es que la capa HTTP de abajo nunca lanza: cuando recibe un 4xx lo registra en el log y devuelve None. Entonces la línea entra al if, no lanza, no adjunta nada, y el elif nunca se evalúa. Desaparece sin llegar a la lista de respaldo.

El arreglo es mover la decisión a una bandera:

attached = False
if line.has_detail:
    try:
        d = client.get_detail(line.id)
        if d:
            attach(line, d)
            attached = True
    except Exception as e:
        log(e)
if not attached and line.code:
    pending.append(line)

Justo los registros más avanzados de su ciclo, los que tenían detalle disponible, se guardaban vacíos y sin error visible. Se vio en producción, cuando el monitoreo siguió disparando después de un fix anterior que ya estaba desplegado. Salió de investigar ese incidente con ayuda de un agente, ya sabiendo dónde buscar.

Y acá está la parte incómoda: no había test para el caso en que falla la consulta del detalle. Pero si alguien lo hubiera escrito, probablemente habría pasado igual, porque al mockear uno sigue el contrato que se imagina, "si falla, lanza", y no el real, "si falla, devuelve None". Fowler lo plantea en su nota sobre contract tests: probar contra un doble siempre deja la duda de si el doble representa de verdad al servicio.

Qué necesitaba saber el reviewer: el comportamiento real de la capa HTTP, que está en otro archivo.

Tres: el signal que se salta el guard#

Antes de escribir al motor de búsqueda, la función de reindexado se asegura de que el índice exista con su esquema explícito:

def reindex_all():
    ensure_index_with_explicit_schema()
    bulk_write(docs)

El guard está ahí, está testeado, y leyendo el repo se ve completo. Lo que no se ve es que la librería también escribe por su cuenta en cada save(), mediante un signal, y ese camino no pasa por el guard. Si el índice no existe, el motor lo crea solo, infiriendo el esquema de los datos.

Bastaba con editar un registro desde el panel de administración en el momento equivocado para que el índice quedara con los tipos mal. Después de eso, filtros y ordenamiento fallan en todas las búsquedas. Ese mismo modo de falla ya había roto la búsqueda en producción una vez.

El fix fue poner el mismo guard en la capa del signal:

class SafeIndexer(LibraryIndexer):
    def on_save(self, instance):
        ensure_index_with_explicit_schema()
        super().on_save(instance)

En tests no se veía porque el índice siempre existe. Lo encontró una persona, razonando después del incidente y verificándolo a mano contra el entorno de desarrollo. Y ese fix trajo después un bug de segundo orden, porque el guard corría aunque la escritura automática estuviera apagada.

Qué necesitaba saber el reviewer: las entrañas de una librería de terceros.

Lo que sí encuentran unos ojos nuevos#

Para ser justo, en el mismo repo hay un bug donde un reviewer nuevo habría brillado. Un argumento por defecto evaluado al importar:

def fetch(to_date=datetime.today() - timedelta(days=730)):
    ...

Se congela al arrancar el proceso, ningún caller lo pasaba, y encima to_date apuntaba a hace dos años, así que cada request salía con el rango al revés. Ese no necesita contexto de nada. Cualquiera que sepa Python lo ve en dos segundos, humano o agente. Está como advertencia en el tutorial oficial de Python: el valor por defecto se evalúa una sola vez. Y esa es la diferencia.

La distinción#

Un reviewer que llega sin contexto encuentra lo que se ve raro por sí solo: el antipatrón conocido, el default que se evalúa una sola vez, el except vacío, el N+1 evidente. Para eso bastan unos ojos nuevos, y de hecho ayuda, porque el que escribió el código tiene adentro la justificación de por qué lo escribió así y esa justificación le tapa la vista.

Pero los tres bugs de arriba no se ven raros. Se ven normales. Se ven normales porque el supuesto equivocado lo compartíamos todos: el autor, el test y el que revisó. Ahí los ojos nuevos solo encuentran lo que su propia experiencia les enseñó a buscar, y un supuesto que comparten todos es justo el que nadie busca. Addy Osmani lo dice desde otro ángulo en su post sobre code review con agentes: el modelo revisa el código que existe y rara vez marca el requisito que nadie pensó en escribir.

Lo que los encuentra es saber algo que el que escribió no sabía. Cómo guarda el dato la otra capa. Que esa función devuelve None en vez de lanzar. Que la librería escribe por un camino paralelo.

Cuando eso lo hace un agente, ese conocimiento tiene que estar escrito en algún lado. No es magia del contexto limpio: es un archivo que dice "revisa contra el comportamiento que ya existe" o "confirma qué devuelve la capa de abajo antes de asumir que lanza". Sin eso, un agente nuevo revisando el diff llega exactamente a la misma conclusión equivocada que llegamos nosotros.

Lo que no sé#

Ninguno de estos tres lo encontró un agente. Dos los encontró gente y uno salió de investigar un incidente con ayuda de un agente, ya sabiendo dónde buscar. Así que esto no es "los agentes encuentran lo que los humanos no", es una taxonomía de qué tipo de conocimiento hace falta, y vale igual para un reviewer humano nuevo en el equipo.

Lo que me queda pendiente es la prueba de verdad: escribir esas reglas en un agente revisor y contar aquí si el próximo bug de costura lo caza antes que producción. Si ya lo intentaste, escríbeme.

Por ahora me quedo con esto: que el reviewer sea otro es necesario, pero no basta. Tiene que saber algo que el que escribió no sabía. Es primo de lo que me pasó con el threshold de jobfit que nunca medí: un supuesto que nadie cuestiona no lo revisa nadie.