Review — task/json-report contra main (merge readiness)

FIXES NEEDED
Alcance: 10 commits: informe JSON, comparacion por carpetas, suite de tests y pipeline. pdf_compare/batch.py, cli.py, comparator.py · Foco: correctness · 2026-08-24
1 bloqueante verificado: un solo documento cifrado aborta el lote entero

La rama esta en muy buen estado: 87 tests, tres sistemas operativos en verde, y una arquitectura donde el PDF y el JSON no pueden discrepar porque comparten una sola implementacion. Lo que queda no esta en la logica de comparacion, que esta bien cubierta, sino en la robustez del modo lote: procesa N documentos sin ningun aislamiento entre ellos, asi que el fallo de uno se lleva por delante el trabajo de todos. Es la diferencia entre una operacion sobre un par y una operacion por lotes, y es justo lo que no se hereda al pasar de una a otra.

1
Bloqueante
3
Mejora
2
Opcional

Bloqueante 1

BloqueanterobustezF1

Un documento ilegible aborta el lote entero y pierde todo el trabajo

Ubicacion: pdf_compare/batch.py:81-96
python
83
        path_b = os.path.join(dir_b, name_b)
84
 
85
        comparator = PDFComparator(path_a, path_b)
86
        summary, pdf_bytes = comparator.compare(build_pdf=bool(output_dir))
87
 
88
        diff_name = None
89
        if pdf_bytes is not None:
Que pasa
El bucle llama a comparator.compare() sin ningun aislamiento por pareja. Si un documento esta cifrado, corrupto o ilegible, la excepcion sube hasta el manejador generico de la CLI, que imprime una traza y termina con codigo 1. Ademas el modo lote no ejecuta validate_pdf() sobre los ficheros, que es justo lo que el modo de un solo par si hace.
Impact
Verificado: una carpeta con tres parejas donde la de en medio esta protegida con contrasena falla con 'ValueError: document closed or encrypted' y no produce ni informe ni diffs. Las dos parejas buenas se comparan y su resultado se tira. Sobre las carpetas de polizas reales que motivaron esta funcion, un solo documento protegido invalida la ejecucion completa y el usuario solo ve un traceback.
Por que importa
Una operacion por lotes tiene un contrato distinto al de una operacion sobre un elemento: se espera que un elemento malo se registre y se siga, no que anule a los demas. La robustez no se hereda al envolver una operacion en un bucle; hay que anadirla explicitamente, y el sitio natural es el limite de cada elemento.
Sugerencia
Aislar cada pareja: capturar el fallo, registrarlo en su entrada del resultado y continuar. El informe ya tiene sitio para mostrarlo junto a los sin pareja y los escaneados, que son la misma categoria de 'esto necesita un humano'. Vale la pena decidir tambien si un lote con fallos debe terminar con codigo distinto de cero para que un script se entere.
Recommended change
Current code
comparator = PDFComparator(path_a, path_b)
summary, pdf_bytes = comparator.compare(build_pdf=bool(output_dir))
Suggested code
comparator = PDFComparator(path_a, path_b)
try:
    summary, pdf_bytes = comparator.compare(build_pdf=bool(output_dir))
except Exception as error:
    results.append(failed_pair(name_a, name_b, score, error))
    continue
Tests
El diff no cubre este caso: ningun test mete un documento ilegible en una carpeta. Hace falta uno que compare dos carpetas donde una pareja esta cifrada y afirme que las demas si se comparan y que el informe registra el fallo.

Mejora 3

MejoraescalabilidadF2

El emparejado aproximado es cuadratico y se materializa entero en memoria

Ubicacion: pdf_compare/batch.py:48-54
python
46
            remaining_b.remove(match)
47
 
48
    scored = sorted(
49
        (
50
            (difflib.SequenceMatcher(None, stem(a), stem(b)).ratio(), a, b)
Que pasa
Tras casar los nombres identicos, el resto se puntua construyendo el producto cartesiano completo de los que quedan, se ordena entero y despues se recorre. Con nombres que coinciden exactamente esto no se nota, porque no queda casi nada. El problema aparece cuando las dos carpetas usan convenciones de nombre distintas y no coincide ninguno.
Impact
Dos carpetas de 500 documentos con nomenclaturas distintas generan 250.000 tuplas en memoria y 250.000 llamadas a SequenceMatcher.ratio(), cada una cuadratica sobre la cadena. Ademas el emparejado ocurre antes de la primera comparacion, y on_progress solo se dispara por pareja comparada: durante todo ese tiempo el comando no imprime nada y parece colgado.
Por que importa
El coste esta acotado por lo que NO casa exactamente, que es precisamente el caso que el usuario no controla. Un algoritmo cuyo peor caso depende de la entrada del usuario necesita, como minimo, que ese peor caso este medido y dicho.
Sugerencia
Un prefiltro barato antes de puntuar (descartar pares cuya diferencia de longitud ya impide superar el umbral, o comparar solo dentro de bloques por primera letra) recorta la mayoria del producto sin cambiar el resultado. Como minimo, medir con carpetas grandes y documentar el limite, y emitir progreso durante el emparejado.
Tests
No requiere test funcional; requiere una medicion con carpetas grandes antes de comprometer un limite.
Mejoraperdida-silenciosaF3

Dos diffs pueden pisarse en sistemas de ficheros que ignoran mayusculas

Ubicacion: pdf_compare/batch.py:90
python
87
 
88
        diff_name = None
89
        if pdf_bytes is not None:
90
            diff_name = f"{os.path.splitext(name_b)[0]}-diff.pdf"
91
            with open(os.path.join(output_dir, diff_name), "wb") as handle:
92
                handle.write(pdf_bytes)
93
 
Que pasa
El nombre del diff se deriva del nombre del documento modificado. Dos documentos que solo difieran en mayusculas, como 'report.pdf' y 'REPORT.PDF', producen 'report-diff.pdf' y 'REPORT-diff.pdf'.
Impact
En Linux son dos ficheros distintos y no pasa nada; verificado. En macOS y en Windows, cuyos sistemas de ficheros por defecto ignoran mayusculas, el segundo sobrescribe al primero sin aviso: el informe enlaza dos entradas y una de ellas apunta al diff equivocado. Es raro, pero es perdida de datos silenciosa y multiplataforma, y el propio CI corre en esos dos sistemas.
Por que importa
Derivar un nombre de fichero de otro dato hereda todas las colisiones de ese dato mas las que anade el sistema de ficheros de destino. Cuando el nombre de salida debe ser unico, hay que garantizarlo al escribir, no confiar en que la entrada lo sea.
Sugerencia
Detectar la colision al construir el nombre y desambiguar con un sufijo, o prefijar con el indice de la pareja. Sea cual sea la via, que el nombre elegido sea el que se guarda en diff_file, para que el enlace del informe siga siendo correcto.
Tests
Falta un test que compare dos carpetas con nombres que solo difieren en mayusculas y afirme que se escriben dos diffs distintos y que cada entrada enlaza el suyo.
MejoraconsistenciaF4

El modo lote no tiene salida legible por maquina

Ubicacion: pdf_compare/cli.py:139-152
python
145
    if both_directories:
146
        try:
147
            run_batch(args.file_a, args.file_b, args.out, args.report)
148
        except Exception as e:
149
            print(f"An error occurred: {e}")
Que pasa
Un par suelto puede emitir JSON con --json; un lote solo emite HTML. La estructura completa del lote ya existe en memoria, con la misma forma que el JSON de un par mas el emparejado y los sin pareja.
Impact
Quien quiera encadenar la comparacion por carpetas en un flujo automatico, que es el caso de uso que motivo el JSON, tiene que parsear el HTML. La funcionalidad util para automatizar es justo la que solo habla con humanos.
Por que importa
Dos modos que resuelven el mismo problema deberian ofrecer las mismas formas de salida, o la asimetria se convierte en una pregunta recurrente. Aqui ademas el coste es minimo porque el dato ya esta construido.
Sugerencia
Aceptar --json tambien en modo lote, volcando la estructura que ya devuelve compare_directories. Decidir si convive con --report o si son excluyentes.
Tests
Si se implementa, hace falta un test que compruebe que el JSON del lote incluye las parejas, los recuentos y los documentos sin pareja.

Opcional 2

Opcionalcodigo-inalcanzableF5

_iter_page_events trata un caso que align_pages nunca produce

Ubicacion: pdf_compare/comparator.py:145-153
python
148
                    if idx_a is not None and idx_b is not None:
149
                        yield 'compared', idx_a, idx_b
150
                    elif idx_b is not None:
151
                        yield 'added', None, idx_b
152
                    else:
153
                        yield 'removed', idx_a, None
154
 
155
            elif tag == 'delete':
Que pasa
Dentro de un bloque 'equal' o 'replace', las ramas que manejan una pagina presente en un solo lado no se ejecutan nunca: align_pages siempre emite esos bloques uno a uno, asi que ambos indices existen siempre. La medicion de cobertura lo confirma: son las unicas lineas sin cubrir del comparador.
Impact
Ninguno en ejecucion. Es generalidad defensiva por si align_pages cambiase de forma, a cambio de codigo que ningun test puede ejercitar y que el proximo lector tendra que analizar para descubrir que no hace falta.
Por que importa
El codigo inalcanzable no se puede verificar ni refactorizar con confianza. Si se conserva a proposito, conviene que lo diga un comentario; si no, sobra.
Sugerencia
O eliminarlo, simplificando el generador, o dejar constancia de que es deliberado y por que.
Tests
Por definicion no se puede cubrir con la implementacion actual de align_pages.
OpcionalestiloF6

lambda asignada a un nombre

Ubicacion: pdf_compare/batch.py:31
python
29
    never dropped silently: a document with no counterpart is a finding.
30
    """
31
    stem = lambda name: os.path.splitext(name)[0].lower()
32
 
33
    remaining_a, remaining_b = list(names_a), list(names_b)
Que pasa
stem se define como una lambda asignada a una variable, algo que PEP 8 desaconseja explicitamente a favor de def.
Impact
Cosmetico. La unica diferencia real es que en una traza aparece como '<lambda>' en lugar de 'stem'.
Por que importa
def documenta el nombre en el propio objeto funcion y admite docstring, que aqui vendria bien para explicar que se normaliza a minusculas a proposito.
Sugerencia
Convertirla en una funcion anidada con una linea de docstring que explique la normalizacion.
Recommended change
Current code
stem = lambda name: os.path.splitext(name)[0].lower()
Suggested code
def stem(name):
    """El nombre sin extension y en minusculas: el emparejado ignora mayusculas."""
    return os.path.splitext(name)[0].lower()

Lo que esta bien resuelto