# Task 2 Review — Fingerprint del contenuto DVR + setup vitest **Diff:** ce399e5..58122a5 **Reviewer:** code-reviewer subagent **Data:** 2026-06-24 --- ## Spec Compliance ### TEMPLATE_VERSION e FingerprintInput ✅ `export const TEMPLATE_VERSION = 'v11'` — `fingerprint.ts:10` ✅ `export type FingerprintInput` con tutti i campi richiesti — `fingerprint.ts:12-24` ✅ `templateVersion: string` (non `typeof TEMPLATE_VERSION`, appropriato) ### hashFingerprintInput ✅ `export function hashFingerprintInput(input: FingerprintInput): string` — `fingerprint.ts:36` ✅ sha256 hex via `node:crypto` — `fingerprint.ts:37` ✅ Deterministico: confermato dai test + analisi `stableStringify` ✅ Indipendente dall'ordine delle chiavi: `stableStringify` ordina ricorsivamente le chiavi degli oggetti — `fingerprint.ts:28-33` ✅ Include `templateVersion` nell'hash: `templateVersion` è un campo di `FingerprintInput` e `stableStringify(input)` la serializza in toto — `fingerprint.ts:37` ### loadFingerprintInput ✅ Firma: `export async function loadFingerprintInput(client: Client, agenziaId: number): Promise` — `fingerprint.ts:51` ✅ Risolve gemella via `resolveDvrAgenziaId` locale per le query DVR — `fingerprint.ts:52, 58` ✅ `agenziaId` proprio per `agenzie` — `fingerprint.ts:53-57` ✅ `agenziaId` proprio per `agenzie_allegati` — `fingerprint.ts:66` ✅ Tutte le query figlie con `ORDER BY id` per determinismo — `fingerprint.ts:60-65` ⚠️ **Cannot verify**: sanity-check colonne contro schema reale (`app/src/lib/server/db/schema.ts`) non incluso nel diff. Il report dichiara tutto OK. Non confutabile dal diff, ma il claim è plausibile dato che l'implementer descrive la verifica tabella per tabella. ### computeDvrFingerprint ✅ `export async function computeDvrFingerprint(client: Client, agenziaId: number): Promise` — `fingerprint.ts:75` ### TDD (4 test unitari sulla funzione pura) ✅ Test 1: deterministico — `fingerprint.test.ts:15-17` ✅ Test 2: ordine-chiavi-indipendente — `fingerprint.test.ts:18-21` ✅ Test 3: cambia se cambia un valore DVR — `fingerprint.test.ts:22-25` ✅ Test 4: cambia se cambia templateVersion — `fingerprint.test.ts:26-29` ✅ `loadFingerprintInput` / `computeDvrFingerprint` non unit-testate (corretto per spec) ✅ Test reali, zero mock ### Setup vitest ✅ `app/vitest.config.ts` creato, environment node, include corretto — `vitest.config.ts:1-8` ✅ Script `"test": "vitest run"` in `package.json` — `package.json` diff ✅ `vitest@^4.1.9` in devDependencies --- ## Strengths - **`stableStringify` è robusta**: gestisce array (ordine preservato, semanticamente corretto), oggetti annidati (ricorsione), `null`/`undefined` (normalizzati a `null` via `JSON.stringify(value ?? null)`), primitivi. L'ordinamento delle chiavi è applicato a ogni livello di ricorsione — garantisce davvero l'indipendenza dall'ordine delle chiavi. - **Gemelle gestite correttamente**: `dvrAgenziaId` per il contenuto DVR, `agenziaId` proprio per `agenzie` e `agenzie_allegati`. Separazione chiara e fedele alla spec. - **Determinismo nelle query**: ogni query sulle tabelle figlie ha `ORDER BY id`. Non ci sono query senza ordinamento esplicito. - **`rows()` helper pulito**: astrae `client.execute` e il cast `as never` è localizzato, non diffuso. Incapsula il boilerplate libsql. - **Fallback sensati**: `agenzia ?? {}`, `dvr ?? {}`, `settings ?? {}` — non fanno crashare per sedi senza DVR ancora compilato; al Task 4 si vedrà se serve distinguere "nessun DVR" da "DVR vuoto". - **Commento sul rischio allegati**: la nota in fondo al brief è stata rispecchiata implicitamente dalla scelta di includere `storage_path` nella SELECT — se la chiave cambia, l'hash cambia. Scelta corretta. - **TDD evidence nel report**: RED/GREEN documentati con output vitest reale, non inventato. - **Fix apostrofo nel test**: il problema del carattere tipografico risolto con `"non dipende dall'ordine…"` è un fix legittimo e minimo. --- ## Issues ### Critical Nessuno. ### Important #### `SELECT *` su `dvr` — schema instabile nell'hash **File:** `fingerprint.ts:58` **Cosa:** La query su `dvr` usa `SELECT *` invece di enumerare le colonne esplicitamente. **Perché è importante:** `SELECT *` include automaticamente qualsiasi colonna aggiunta in futuro allo schema `dvr` (es. `pdf_hash`, `pdf_url`, colonne di stato interne). Una migrazione che aggiunge una colonna operativa alla tabella `dvr` cambierebbe l'hash di tutti i DVR senza che sia cambiato nulla nel *contenuto*, invalidando tutte le cache R2. Questo è il contrario dello scopo del fingerprint. **Come risolvere:** Enumerare esplicitamente le colonne DVR rilevanti per il contenuto, come fatto per tutte le altre tabelle. Esempio: `SELECT id, agenzia_id, rev_corrente, data_rev, note_rev, ...` (le colonne concrete vanno lette dallo schema). #### `resolveDvrAgenziaId` duplicata — divergenza silente futura **File:** `fingerprint.ts:46-49` **Cosa:** La funzione `resolveDvrAgenziaId` è reimplementata localmente invece di importare quella esistente in `$lib/server/dvr` (o equivalente). **Perché è importante:** Il brief specifica esplicitamente *"risolve la gemella via la stessa logica di `resolveDvrAgenziaId`"*. Due implementazioni indipendenti con la stessa semantica divergeranno silenziosamente se la logica di risoluzione gemelle viene aggiornata in un solo punto. La query SQL identica oggi non è garanzia di allineamento futuro. **Come risolvere:** Verificare se `resolveDvrAgenziaId` è già esportata da `$lib/server/dvr`; se sì, importarla. Se non è esportata o se il modulo non è importabile da `fingerprint.ts` per ragioni di dipendenze circolari, esportarla da un modulo condiviso e importarla da lì. Il commento `// Stessa regola di $lib/server/dvr.resolveDvrAgenziaId` riconosce il problema ma non lo risolve. ### Minor #### `dvrId = -1` come fallback — silently broken **File:** `fingerprint.ts:59` **Cosa:** Se `dvr` è `undefined` (nessun DVR ancora per quella sede), `dvrId` viene impostato a `-1`. Tutte le query figlie con `WHERE dvr_id = -1` restituiranno array vuoti senza errore. **Perché è minor:** Comportamento funzionalmente accettabile per ora (fingerprint di un DVR non ancora compilato = hash di tutto vuoto), ma la semantica è opaca. Un futuro lettore potrebbe non capire che `-1` è "nessun DVR". Un `null` esplicito con short-circuit avrebbe semantica più chiara. **Come risolvere (opzionale):** `const dvrId: number | null = dvr ? (dvr.id as number) : null;` e poi saltare le query se `dvrId === null`, oppure lasciare come è aggiungendo un commento che esplicita il comportamento. #### Test: assenza di test per array annidato con oggetti con chiavi non ordinate **File:** `fingerprint.test.ts` **Cosa:** I test verificano l'ordine delle chiavi a livello top-level dell'oggetto e nel campo `agenzia` (livello 2), ma non verificano che l'ordinamento ricorsivo funzioni per oggetti dentro array (es. `pericoli: [{ colD: 'S', id: 1 }]` vs `pericoli: [{ id: 1, colD: 'S' }]`). **Perché è minor:** Il codice di `stableStringify` è corretto e gestisce questo caso per costruzione (la branch `typeof value === 'object'` viene raggiunta per ogni elemento dell'array). Ma la spec richiede che l'hash sia "indipendente dall'ordine delle chiavi" senza qualificare il livello, e nessun test lo dimostra per oggetti dentro array. **Come risolvere:** Aggiungere un quinto test case che verifica che `pericoli: [{ colD: 'S', id: 1 }]` produce lo stesso hash di `pericoli: [{ id: 1, colD: 'S' }]`. --- ## Assessment **Task quality: Needs fixes** L'implementazione è solida nella struttura e corretta nel comportamento osservato (4 test green, logica `stableStringify` rigorosa, separazione gemelle). Il `SELECT *` su `dvr` (Important) è però un difetto funzionale che minerebbe l'affidabilità del fingerprint cache a ogni migrazione schema — va corretto prima che il Task 4 eserciti `loadFingerprintInput`. La duplicazione di `resolveDvrAgenziaId` (Important) crea rischio di drift silente e va eliminata o giustificata con un workaround strutturale (modulo condiviso). --- ## Re-review (post-fix) — diff ce399e5..1a594fb ### Important #1 — SELECT * su dvr → ✅ Risolto `fingerprint.ts:869-873` sostituisce `SELECT *` con 13 colonne esplicite di contenuto DVR: `id, rev_corrente, data_emissione, descrizione_immobile, rischio_sismico_descr, rischio_idro_si_no, rischio_idro_descr, rischio_ambientale_si_no, rischio_ambientale_descr, piano_emergenze_testo, rischio_incendio_testo, conclusione_testo, luogo_sicuro_esterno_descr`. Escluse `updated_at`, `created_at`, `imported_at`, `source_file`, `source_format`. Le colonne incluse sono tutte contenuto del PDF — `luogo_sicuro_esterno_descr` in particolare è un campo del DVR, non operativo. Fix corretto e completo. ### Important #2 — resolveDvrAgenziaId duplicata → ✅ Accettabile (vincolo architetturale documentato) Il controller ha declinato il fix "importa quella esistente" con giustificazione valida: `fingerprint.ts` deve restare framework-agnostic perché usato anche da `app/scripts/pdf/generate.ts` su GitHub Actions (fuori da SvelteKit, dove `$env/dynamic/private` non è disponibile). Importare il `db` Drizzle dall'implementazione SvelteKit violerebbe questo vincolo strutturale. Il fix applicato (`fingerprint.ts:851-856`) documenta esplicitamente la duplicazione deliberata con il vincolo, la logica identica, e il `⚠️` warning di sync. Dato il vincolo, questa è la mitigazione corretta — nessuna soluzione strutturale migliore è disponibile senza introdurre un terzo modulo condiviso (che richiederebbe un refactor più ampio non pertinente a questo task). **Closing questo finding come accettabile con documentazione.** ### Minor — 5° test (ordine chiavi annidate in array) → ✅ Aggiunto `fingerprint.test.ts:801-805`: verifica che `pericoli: [{ colD: 'S', id: 1 }]` produce lo stesso hash di `pericoli: [{ id: 1, colD: 'S' }]`. Test corretto e coprente. ### Minor — commento dvrId=-1 → ✅ Chiarito `fingerprint.ts:874`: commento esplicito aggiunto. Semantica ora leggibile senza ambiguità. ### Nuovi finding introdotti dai fix Nessuno. Il fix al `SELECT dvr` non introduce regressioni. Le altre righe del file restano invariate. ### Verdetto finale **Task quality: Approved** Tutti i finding Important sono stati risolti o chiusi con giustificazione architetturale accettabile. I due Minor sono stati corretti. Nessun nuovo finding rilevante. Il codice è pronto per essere esercitato al Task 4.