Code review asistido por LLM en pull requests: que reglas le dimos y donde preferimos que no opine

Integramos un revisor basado en LLM en el flujo de pull requests hace unos meses, con un objetivo puntual: que los comentarios repetitivos -una importacion sin usar, un patron que ya se corrigio en otro archivo del mismo PR, un cambio de logica de negocio sin test que lo cubra- no consuman el tiempo de revision de una persona. La idea no era reemplazar al revisor humano, sino filtrar antes de que el PR llegue a esa persona, para que lo que quede por discutir sea justamente lo que un modelo no puede evaluar bien: si esta decision tiene sentido dentro del sistema completo.

Ese objetivo obligo a trazar una linea explicita, por escrito, entre lo que el revisor automatico puede senalar y lo que no. No fue una linea que se nos ocurrio de entrada; la primera version no tenia restricciones y tuvimos que revertir parte de eso a los pocos dias.

Lo que le confiamos: estilo, patrones repetidos, cobertura de tests

El revisor corre como un paso mas del pipeline de CI, sobre el diff del pull request, y publica sus comentarios como comentarios normales de revision en GitHub, no como aprobacion o bloqueo del merge.

# .github/workflows/llm-review.yml
name: llm-review
on:
  pull_request:
    types: [opened, synchronize]

jobs:
  review:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0
      - name: Run review
        run: |
          diff=$(git diff origin/${{ github.base_ref }}...HEAD)
          ./scripts/llm-review.sh --diff "$diff" --rules rules/review-scope.yml
      - name: Post comments
        run: ./scripts/post-review-comments.sh

El archivo rules/review-scope.yml es lo que de verdad define el comportamiento del revisor, mas que el prompt en si. Ahi listamos, por categoria, que tipo de hallazgo puede senalar:

allowed_categories:
  - style           # nombres, imports sin usar, codigo muerto, duplicacion local
  - repeated_pattern # una correccion aplicada en un archivo y no en otro similar del mismo diff
  - missing_tests    # logica de negocio nueva sin test que la cubra en el mismo PR
  - null_safety      # accesos sin verificar sobre valores que ya vienen como nullable en el tipo

forbidden_categories:
  - architecture     # eleccion de framework, libreria, limites entre servicios
  - schema_design    # forma de tablas, migraciones, normalizacion
  - dependency_choice # agregar o cambiar una dependencia de terceros
  - naming_of_domain_concepts # nombres de entidades de dominio ya establecidos en el codigo

La categoria repeated_pattern es la que mas valor dio en la practica, porque es exactamente el tipo de cosa que un humano se salta por cansancio: el revisor compara los distintos archivos tocados en el mismo diff y senala cuando una correccion -por ejemplo, agregar una validacion de un campo obligatorio- se aplico en un controlador pero no en otro que sigue la misma estructura dentro del mismo PR. Es un hallazgo local, verificable con solo mirar el diff, sin necesitar contexto de por que el sistema esta armado como esta.

missing_tests funciona con una regla simple: si el diff agrega o modifica un metodo que no es trivial (mas de una linea de logica, con alguna rama condicional) y no hay ningun archivo de test tocado en el mismo PR que referencie ese metodo, lo marca como comentario, no como bloqueo. La decision de si ese metodo en particular necesita test la sigue tomando la persona que revisa; el bot solo evita que el caso pase desapercibido.

Donde decidimos que no debia opinar

La categoria forbidden_categories no es una lista de temas donde el modelo suele equivocarse mas -de hecho, en varias ocasiones sus sugerencias sobre arquitectura eran razonables leidas en aislado-. Es una lista de temas donde la razon detras de la decision no esta en el diff, ni en el codigo del repositorio, sino en una conversacion que paso en otro lado: por que se eligio ese limite entre servicios, que alternativa se descarto y por que, que restriccion del roadmap hizo que una solucion mas simple no fuera viable en ese momento.

Un LLM que revisa un diff no tiene acceso a nada de eso. Ve el codigo resultante, no el proceso que llevo a esa forma. Cuando comenta sobre una decision de arquitectura, esta evaluando esa decision contra un criterio generico de buenas practicas, no contra las restricciones reales del proyecto, y el comentario suena igual de seguro este en lo cierto o no. Esa confianza aparente es el problema: un comentario de estilo mal fundamentado se descarta rapido, pero un comentario de arquitectura mal fundamentado, con tono seguro, empuja a la persona que revisa a dudar de una decision que en realidad ya se habia tomado con informacion que el bot no tenia.

Lo que no funciono: dejarlo opinar sobre todo, aunque fuera solo comentario

La primera version de rules/review-scope.yml no tenia forbidden_categories. Razonamos que, como el bot solo publicaba comentarios y nunca bloqueaba el merge, no habia riesgo real en dejarlo senalar cualquier cosa: si opinaba sobre arquitectura y estaba equivocado, el revisor humano simplemente lo ignoraba.

En la practica no funciono asi. El bot empezo a comentar sistematicamente sobre decisiones que ya estaban tomadas de antes -por ejemplo, sugiriendo extraer un servicio que deliberadamente se habia mantenido junto a otro por una razon operativa concreta, o proponiendo cambiar una libreria por otra sin saber que la eleccion original venia de una restriccion de licencia-. El comentario en si no bloqueaba nada, pero cada uno de esos casos generaba la misma secuencia: alguien del equipo tenia que responder por escrito explicandole el contexto a un comentario que ningun humano habia escrito, y en PRs donde participaban personas menos familiarizadas con el historial del proyecto, esos comentarios generaban dudas genuinas sobre si la decision original seguia siendo valida, incluso cuando lo era.

El costo no era el tiempo que tardaba el bot en equivocarse. Era el tiempo humano que se iba en desactivar, PR tras PR, la autoridad implicita que tiene cualquier comentario escrito con tono seguro, viniera o no de alguien con contexto real. Revertimos eso agregando la lista explicita de categorias prohibidas, y el numero de comentarios bajo de forma notoria, pero los que quedaron eran, en su enorme mayoria, accionables sin necesitar contexto adicional.

El criterio que quedamos usando

La pregunta que aplicamos ahora para decidir si algo entra en allowed_categories no es si el modelo suele acertar en ese tipo de hallazgo. Es si la razon para aceptar o rechazar ese hallazgo esta completa dentro del diff y el codigo del repositorio, o si depende de una conversacion, una restriccion de negocio o una decision previa que no esta escrita en ningun lado que el modelo pueda leer. Lo primero es terreno seguro para un revisor automatico. Lo segundo sigue siendo, y va a seguir siendo, terreno de la persona que tiene el contexto que el diff no puede contener.