# Review d'architecture — specs « Module Imports » et « Socle jobs » (2026-08-31) Reviewer : agent d'architecture (adversarial), avant validation utilisateur. Specs relues : `2026-08-31-module-imports-design.md`, `2026-08-31-socle-jobs-design.md`. Références : flow 2026-08-28 (§0, §5, §6, §8 — déviation « Imports avant Crawl » actée, non contestée), data 2026-08-28 (§3, §4, §7), architecture 2026-08-26 (§7, §8, §9), CLAUDE.md (rituel migrations), code réel du module Sites (`api/app/sites/`, `web/lib/sites.ts`, `app/db/models.py`, `app/core/errors.py`). ## Verdict : **À amender** (2 Critical, 6 Important — tous localisés, aucune refonte structurelle) L'architecture d'ensemble est saine et fidèle au canon : keywords/keyword_metrics conformes à data §7 (provenance empilée, jamais fusionnée entre sources), DuckDB confiné au worker, tsvector + pg_trgm + halfvec sans index, `org_id` présent et indexé sur chacune des dix tables nouvelles, Problem Details et curseur repris du module Sites à la lettre, aucun score ni audit nulle part, une migration par tranche porteuse (T0 `jobs`, T1 `imports`, T8 `byok`, zéro ailleurs — compatible CLAUDE.md). La suppression de l'« étape d'attache » au profit d'une convergence continue est la bonne lecture du problème. Mais deux mécanismes centraux, tels qu'écrits, contredisent des promesses que les specs font elles-mêmes. --- ## Critical ### C1 (Imports §4.3 vs §1.3 / décision n°7 / §3.11) — le re-import correctif ne corrige rien La spec vend trois fois le « re-import après correction d'adaptateur » (warning sha256 *soft*, fichiers conservés 30 jours *pour re-parse*, pas d'UNIQUE dur sur `file_sha256`). Or l'écriture est `ON CONFLICT (keyword_id, source, fetched_at) DO NOTHING`, et `keyword_metrics` ne porte aucun lien vers l'import (canon « zéro colonne ajoutée »). Scénario concret : l'adaptateur Semrush lit `1 200` comme volume `1` (séparateur de milliers) ; on corrige le parser, on force le re-import du même fichier avec le même `fetched_at` → chaque ligne tombe en conflit → **0 insérée, tout « ignorées », les valeurs fausses restent pour toujours** — et il n'existe ni suppression d'import ni rollback (aucune traçabilité import→lignes). Seules les lignes qui étaient *en erreur* profitent du re-parse. **Amendement proposé** : `ON CONFLICT (keyword_id, source, fetched_at) DO UPDATE` — la clé naturelle désigne *la même observation* ; la remplacer par un parse corrigé n'est pas une fusion destructive entre sources (le canon interdit d'écraser *la provenance*, pas de corriger une cellule identifiée par sa provenance). Le rejeu idempotent reste vrai (mêmes valeurs → update sans effet), le test « rejeu forcé = état identique » reste valide en le formulant sur l'état, pas sur « 0 insérée ». ### C2 (Socle jobs §2.5) — pas d'`attempts` : un job empoisonné bloque toute la file, à l'infini Architecture §9 exige « `attempts`, 3 tentatives max, ensuite `failed` ». Le socle ne porte ni colonne ni compteur, et le balayage des zombies requalifie `queued` sans borne. Scénario concret : un CSV corrompu fait crasher DuckDB *en natif* (pas une exception Python — le process meurt, le point §2.3 « jamais un worker mort » ne s'applique pas) → systemd relance le worker → le sweep requalifie le job → re-crash → boucle infinie qui **monopolise l'unique worker et gèle imports et reconciliations de toutes les orgs**. C'est aussi un écart non déclaré à l'architecture (la section « Écarts » du socle n'en liste qu'un, cosmétique). **Amendement proposé** : colonne `attempts INTEGER NOT NULL DEFAULT 0`, incrémentée au claim ; le sweep passe en `failed` au-delà du seuil (3). Une ligne de schéma, un `if` dans le sweep, un test (« zombie 3 fois → failed, la file continue »). --- ## Important ### I1 (Socle §1/§2.3 + Imports §3.9) — `dedup_key` relâchée à `done` : lost-wakeup qui casse l'invariant de convergence `dedup_key` n'est remise à NULL qu'au passage `done`/`failed`. Scénario : le crawl A finit → `map_reconcile:` tourne sur un snapshot des pages ; le crawl B finit *pendant* ce run → l'enfilage du hook tombe en conflit → renvoie le job **en cours** → les pages découvertes par B ne sont jamais rattachées tant qu'un humain ne clique pas « relancer ». L'invariant §3.9 (« converge quel que soit l'ordre d'arrivée ») est faux dans ce scénario, et le texte du socle (« ne s'empile jamais **dans la file** ») décrit en réalité une dédup au niveau *queued*, pas *running*. Attention à l'interaction : relâcher la clé au claim rouvre la course de la garde « un seul crawl actif par site » à N workers (le SKIP LOCKED protège la ligne, pas le prédicat). **La sémantique doit être tranchée par type** : pour `map_reconcile` (idempotent, pas cher), relâcher au claim — ou faire boucler le handler tant que des lignes sont mises à jour — suffit ; pour `crawl`, garder la clé jusqu'à `done` est précisément ce qu'on veut. Documenter les deux, tester le scénario A/B. À tester aussi (mineur mais réel) : conflit → relecture par `dedup_key`, mais le gagnant peut finir et NULLer sa clé entre les deux → relecture vide → prévoir le fallback (ré-insérer). ### I2 (Imports §4.3) — `keyword_page_map` en `DO NOTHING` fige `position`/`seen_at` à la première observation Re-import un mois plus tard, même (cible, keyword, URL, source), position passée de 12 à 5 → conflit → **la position affichée (et consommée par la Matrice) reste 12 pour toujours**. Amendement idempotent : `DO UPDATE SET position=EXCLUDED.position, seen_at=EXCLUDED.seen_at WHERE EXCLUDED.seen_at > keyword_page_map.seen_at` (le rejeu du même fichier reste un no-op strict). ### I3 (Imports §3.11) — `imports.status` « recopie finale » orpheline en cas d'échec C'est le worker *neutre* qui passe le job à `failed` sur exception ; le handler d'import n'est alors plus là pour recopier → la liste des imports montre un import `queued` à perpétuité. Deux issues : le handler enveloppe tout dans son propre try/except et recopie avant de re-lever, **ou** `GET /imports` joint `jobs` par `job_id` (lookup PK — la justification « lister sans join sur la file » ne tient pas : ce n'est pas un scan de la file). La seconde supprime même la dénormalisation. ### I4 (Imports §3.8/§3.9/§11) — le contrat `pages` est pré-figé de fait, en contradiction avec le texte §3.8 affirme que `normalize_url` et `jobs` sont « les seuls contrats pré-figés avec Crawl ». Or §3.9 (résolution à l'insertion, jointure `pages.url_normalized` « de la cible ») et `test_map_convergence` (« table `pages` simulée par une fixture du contrat ») gèlent trois choses : une colonne `pages.url_normalized`, un scoping par cible, un statut 404 lisible (badge flow 1.9). Par ailleurs T2 liste « résolution à l'insertion » alors que `pages` n'existera pas avant Crawl : soit ce code référence une table absente (fragile), soit il n'arrive qu'en T6. **Amendement** : déclarer explicitement le mini-contrat `pages` (id, url_normalized, rattachement cible, status) comme troisième contrat pré-figé, et déplacer la résolution-à-l'insertion en T6 (avec `map_reconcile`, qui a la même dépendance) — T2 n'y perd rien, le reconcile de fin de crawl couvre le rattrapage. ### I5 (Imports §7.2 / décision n°8) — le spike T4 tel que décrit produira un faux positif La limite de corps de requête des Functions Vercel est **documentée (4,5 Mo)** ; un export Semrush Positions de 50 k lignes la dépasse couramment. Surtout : le spike exécuté en dev (serveur node sous d3k) **passera** — la limite n'existe qu'en prod Vercel — et le proxy cassera au premier vrai client. Amender : le spike doit viser un déploiement Vercel (preview suffit), ou acter d'emblée le plan B (upload direct navigateur → FastAPI, JWT + CORS strict sur ce seul endpoint) comme chemin de prod. ### I6 (Imports §1.7/§6) — le prédicat d'appartenance du « réservoir par cible » n'est jamais défini `GET /keywords?site_target_id=` (requis) : quels keywords appartiennent à la cible ? Items des listes de la cible (suggéré par « la liste est le lien cible↔réservoir », §3.4) ? Tous les keywords de l'org sur la locale de la cible (suggéré par la mutualisation, décision n°2, et par « restent dans le réservoir de l'organisation », flow 1.10) ? Un keyword attaché à la main mais retiré de toute liste disparaît-il du réservoir (et son attache avec lui, invisible) ? Ce prédicat conditionne l'implémentation de l'endpoint, `attach_status`, le décompte de suppression et l'écran T5 — il doit être écrit noir sur blanc avant T3. --- ## Minor 1. **Curseur du listing fusionné** `GET /imports` (lignes `imports` + jobs `map_reconcile` entrelacées) : deux tables sous un seul curseur — le keyset sur uuidv7 (union triée par id desc) marche, mais rien n'est spécifié ; à écrire, sinon chaque implémenteur inventera le sien. 2. **Migration T1** : l'autogénération Alembic ne produit ni la vue `keyword_metrics_latest` ni `CREATE EXTENSION` (pg_trgm, pgvector pour `halfvec`) — ops manuelles dans la révision (et `data_test`/db-init à aligner) ; à mentionner dans §3.13 pour que la review du SQL les attende. 3. **Rapport après reprise** (`test_import_resume`) : préciser « état final » = état *base* ; les compteurs inserted/ignorées d'un run repris différeront mécaniquement d'un run ininterrompu. 4. **Doublons intra-fichier** : Semrush Positions émet une ligne par (keyword, URL) — un keyword multi-URLs produit N-1 métriques « ignorées » (normal, les maps portent la cannibalisation) ; le rapport doit distinguer ce cas d'un vrai doublon pour ne pas inquiéter. 5. **`normalize_url`** : politiques bien jugées (trailing slash conservé = fidèle au crawl ; tracking supprimé ; tri de query = clé d'égalité stable ; https forcé pragmatique). Ajouter aux fixtures : clés de query dupliquées (`a=1&a=2`, tri stable préservant l'ordre des valeurs), query vide (`?` seul), espaces/percent-encoding. Assumer dans la spec le coût du slash strict : `/page` vs `/page/` ne se rattacheront pas (taux de résolution) — proposition v2 : suggestion non-automatique sur égalité slash-insensible. 6. **Rétention de `jobs`** : les événements UI (onglet Imports) dépendent d'une table `jobs` jamais purgée ; le jour où une purge arrive, elle devra épargner ou archiver ces événements — noter la dépendance (écart n°3). 7. **`preferred_source`** non validée contre le registre d'adaptateurs (PATCH accepte n'importe quel texte) ; `DELETE /keywords/{id}/map` ne dit pas s'il ne vise que le map manuel ; « date du fichier si extractible » (fetched_at) est vague — préciser la source (nom de fichier Semrush ? jamais fiable → défaut aujourd'hui suffit). 8. **Benchmark T2** : inclure la requête réservoir consolidée (filtre `volume_min` sur valeur calculée + curseur), pas seulement l'import ; « 500 k lignes réelles » est incompatible avec les plafonds d'export (Semrush 10–50 k) — dire « synthétisées par concaténation d'exports réels ». 9. **Jobs orphelins** : `map_reconcile` d'une cible supprimée (CASCADE) doit finir `done` no-op, pas `failed` — un test. 10. **Chemin fichiers** `/.csv` : org_id vient du JWT (opaque Better Auth) — le sanitizer quand même (défense en profondeur contre un séparateur de chemin). --- ## Position sur les 5 écarts déclarés (Imports §12, socle « Écarts ») | # | Écart | Position | |---|---|---| | 1 | `org_settings` ajoutée | **Justifié et propre** : table minimale, PK org_id, seule façon simple de porter la décision 3.d. Sur la question ouverte : recommander **fraîcheur d'abord, préférence en tie-break** — une source préférée « absolue » masquerait des valeurs fraîches avec des valeurs mortes, à rebours de tout le design (filtres last-updated, badges de fraîcheur). À faire trancher par l'utilisateur, comme la spec le demande. | | 2 | Vue + requête paramétrée | **Acceptable mais à resserrer** : tel qu'écrit, ce sont *deux* implémentations de la règle (la vue et la requête du service), pas « une définition, deux points d'entrée » — dérive garantie. Préférer **une fonction SQL** `keyword_metrics_latest(preferred text DEFAULT NULL)` consommée par le service *et* la Matrice ; ou supprimer la vue et ne garder que le builder du service. Un seul artefact. | | 3 | Événements via `jobs` | **Bon choix v1** (zéro table en plus), sous deux réserves : spécifier le curseur du listing fusionné (Minor 1) et acter la dépendance à la non-purge de `jobs` (Minor 6). | | 4 | Table des priorités complète en `CASE` | **Justifié** : la synthèse ne donnait que deux exemples, il fallait bien compléter ; défauts plausibles, `generic_csv` dernier est juste, « position hors consolidation » respecte le canon. À faire valider tel quel. | | 5 | `ON DELETE SET NULL` pour la future FK `page_id` | **Correct** : une page supprimée redevient « en attente » sans perdre l'URL déclarée — c'est exactement l'invariant de convergence. À confirmer dans la spec Crawl, comme prévu. Sur l'écart du socle (nom du worker) : cosmétique ; légère préférence pour `python -m app.jobs.worker` (architecture §7, pas de module à la racine de `app/`), non bloquant. | --- ## Ce qui est bon et doit rester tel quel - La convergence d'attache comme invariant (insertion + reconcile + manuel jamais écrasé, orphelin de premier rang) — une fois I1/I2 corrigés, l'idempotence tient réellement (UPDATE gardés par `seen_at`, ON CONFLICT sur clés naturelles, reconcile ensembliste re-exécutable). - Index partiel `UNIQUE (site_target_id, keyword_id) WHERE source='manual'` + UNIQUE quadruple : la cardinalité « une planification, N observations » est exactement la bonne, et `ON CONFLICT` sur index partiel est supporté par Postgres. - `locale` marché complet avec CHECK, héritée de la cible : cohérente avec les deux colonnes de `site_targets` (recombiner est trivial, la spec Sites l'avait prévu). - Le confinement strict : pas de table `pages`, pas de GSC, `'gsc'` réservé dans le CHECK, zéro migration prévue pour le module 3 — crédible. - Les tranches T0→T8 : au plus une migration chacune (T0, T1, T8), une branche par tranche, T5 livrable sans Crawl avec test d'acceptation — conforme au rituel CLAUDE.md. - La table de vérité des adaptateurs (fixture réelle obligatoire, un par PR, generic_csv de premier rang) : c'est la bonne réponse aux colonnes non vérifiées. ## Conditions de validation Amender avant validation : **C1, C2** (bloquants), **I1–I6** (dont I6 = simple paragraphe de définition, I5 = reformulation du spike). Les Minor peuvent être traités au fil des tranches. Aucun des amendements ne remet en cause le modèle, l'ordre des tranches ni les décisions utilisateur actées.