Karpathy Code Review — Cultiva SaaS

🚫

No mergear. 6 violaciones Karpathy detectadas.

La tarea era "añadir un endpoint que reciba campaign_id y devuelva un PDF". El PR entregó 397 líneas, 9 clases, 3 interfaces abstractas y un sistema de telemetría que nadie pidió. Score de simplicidad: 40/100. Se requieren cambios antes de la revisión humana.

Simplicidad (P2)

de 100 · umbral strict · FAIL

Cambios quirúrgicos (P3)

WARN

4 findings de diff-noise

Suposiciones (P1)

hallazgo · plan sin verify

Criterios éxito (P4)

0/12 pts · MISSING · sin tests

397 líneas (máx 300)

21 imports (máx 10)

9 clases (2.3 por 100 líneas)

nesting 5 (máx 3)

24 funciones

cyclomatic avg 0.8 ✓

Python

1 Principio 1 — No asumir · Surfacing de suposiciones

Suposiciones ocultas detectadas

assumption_linter.py — 1 finding · veredicto: REVIEW

REVIEW

⚠

missing-verification

El ticket dice "devolver un PDF". El dev asumió silenciosamente que también había que: implementar caché Redis, subida a S3, telemetría externa, reintentos con back-off exponencial y soporte multi-locale. Ninguna de estas features fue solicitada ni confirmada.

→ Antes de implementar: preguntar "¿la primera versión necesita caché/storage/telemetría?" y documentar la respuesta.

⚠

silent-interpretation

ReportConfig tiene 22 campos — 20 no están en el ticket. include_competitor_analysis, watermark, chart_theme, etc. son features especulativas.

→ Regla: solo los campos que el endpoint actual necesita. Los demás, en un PR futuro.

2 Principio 2 — Simplicidad primero · Over-engineering

4 violaciones de complejidad — threshold: strict

complexity_checker.py — score 40/100 — FAIL

FAIL 40/100

✗

file-length

Archivo de 397 líneas para un único endpoint. El máximo recomendado es 300. La tarea entera cabe en ~60 líneas.

→ Objetivo: ≤ 80 líneas. Una función generate_report(campaign_id) → bytes.

✗

import-count

21 imports para una feature que solo necesita DB + Jinja2 + pdfkit. boto3, redis, httpx, asyncio... ninguno usado en el flujo mínimo.

→ Dejar solo: jinja2, pdfkit, sqlalchemy, fastapi. 4 imports.

✗

class-density + premature-abstraction

9 clases en 397 líneas (2.3/100). ReportRenderer(ABC), StorageBackend(Protocol), CacheBackend(Protocol) — abstracciones para un único renderer, un único storage, un único cache. Regla: no abstraer hasta tener 2+ implementaciones concretas.

→ Eliminar las 3 clases abstractas. Usar funciones directas o 1 dataclass simple.

✗

nesting-depth

Profundidad máxima de 5 niveles (máx 3). El bucle de reintentos dentro de generate() anida: for → try → except → if → time.sleep.

→ Extraer la lógica de retry a una función helper _render_with_retry().

Antes → Después

ANTES (sobre-ingenierizado)

class ReportGenerationService:
  def __init__(self,
    renderer: ReportRenderer,
    repository: CampaignRepository,
    storage: StorageBackend,
    cache: CacheBackend,
    max_retries: int = 3,
    retry_delay: float = 1.0,
    timeout: float = 60.0,
    enable_telemetry: bool = True,
    telemetry_endpoint: str = None,
  ): ...

# 280 líneas más...
              

DESPUÉS (mínimo viable)

@router.post("/generate")
async def generate_report(
  campaign_id: str,
  db = Depends(get_db),
):
  campaign = get_campaign(db, campaign_id)
  if not campaign:
    raise HTTPException(404)
  pdf = render_pdf(campaign)
  return Response(pdf,
    media_type="application/pdf")

# ~20 líneas. Igual de correcto.
              

3 Principio 3 — Cambios quirúrgicos · Diff noise

Ruido en el diff detectado

diff_surgeon.py — 4 categorías · WARN

WARN

Categoría de ruido	Ejemplo en el PR	¿Trazable al ticket?
Drive-by refactor	Renombró `db_session` → `db` en `CampaignRepository` (código no tocado)	NO
Comment noise	Añadió docstrings a 6 funciones existentes que no cambiaron	NO
Style drift	Cambió comillas simples → dobles en 12 líneas de archivos no relacionados	NO
Imports especulativos	`import asyncio`, `import uuid` añadidos pero no usados por el endpoint	PARCIAL

📏

regla quirúrgica

De las ~340 líneas del diff, aproximadamente 80 líneas (~24%) no se relacionan con el ticket. Cada línea cambiada debería trazar directamente a la solicitud del usuario.

→ git diff --staged | python3 diff_surgeon.py → revisar todas las líneas marcadas como ruido antes de hacer commit.

4 Principio 4 — Ejecución orientada a objetivos · Criterios verificables

Sin criterios de éxito verificables

goal_verifier.py — 0/12 pts — MISSING

MISSING 0%

✗

no tests · no verify labels

El PR no incluye ningún test. La tarea tiene 4 pasos y 0 criterios de verificación. Sin tests no hay forma de confirmar que el endpoint funciona ni que regresos no rompen nada.

Plan corregido (con criterios verificables)

#	Paso	Verificar
1	Crear endpoint `POST /api/reports/generate`	`assert response.status_code == 200`
2	Renderizar HTML del reporte con Jinja2	`assert b"<html" in html_output`
3	Convertir HTML → PDF con pdfkit	`assert pdf_bytes[:4] == b"%PDF"`
4	Devolver PDF como respuesta HTTP	`assert response.headers["content-type"] == "application/pdf"`
5	Devolver 404 si campaign_id no existe	`assert response.status_code == 404`

→ Próximos pasos requeridos

1. Simplificar a ~60 líneas: Borrar las 3 clases abstractas, el sistema de caché, el storage, la telemetría y el retry. Pueden volver en PRs separados con tickets propios.

2. Limpiar el diff: Revertir los drive-by refactors, cambios de estilo y docstrings en código no relacionado con la tarea.

3. Añadir 5 tests: Los del plan corregido. Son 20 líneas de pytest. Sin ellos el PR no tiene criterios de éxito.

4. Reducir imports a 4: Solo lo que el endpoint actual usa. El resto se añade cuando se necesite (YAGNI).

5. En el siguiente ticket: Documentar explícitamente si se necesita caché / storage / telemetría antes de implementarlos. No asumir.

Disciplina de Código Karpathy

No mergear. 6 violaciones Karpathy detectadas.