26bd096a06
Major changes across backend, frontend, infrastructure: - Provider system with model selection (Google, DeepL, OpenAI, Ollama, Google Cloud) - Admin panel: user management, pricing, settings - Glossary system with CSV import/export - Subscription and tier quota management - Security hardening (rate limiting, API key auth, path traversal fixes) - Docker compose for dev, prod, and IONOS deployment - Alembic migrations for new tables - Frontend: dashboard, pricing page, landing page, i18n (en/fr) - Test suite and verification scripts Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
130 lines
8.4 KiB
Markdown
130 lines
8.4 KiB
Markdown
# 🔥 CODE REVIEW FINDINGS, Sepehr!
|
||
|
||
**Stories:** 2-5 (Provider OpenAI), 2-6 (Provider Fallback Chain)
|
||
**Git vs Story Discrepancies:** Voir détail ci‑dessous
|
||
**Issues Found:** 3 High, 4 Medium, 5 Low
|
||
|
||
---
|
||
|
||
## Contexte
|
||
|
||
- **Story 2-5:** `2-5-provider-openai-llm-cloud.md` (Status: review)
|
||
- **Story 2-6:** `2-6-provider-fallback-chain.md` (Status: review)
|
||
- **Git:** `services/providers/` et `tests/` sont **non suivis** (??). Fichiers listés dans les stories existent bien sur le disque. Pas de divergence "fichier dans story mais pas dans git" pour ces stories car les dossiers sont nouveaux/non commités.
|
||
- **Tests:** 58 tests (OpenAI + fallback) passent.
|
||
|
||
---
|
||
|
||
## 🔴 CRITICAL / HIGH ISSUES
|
||
|
||
### 1. [2-6] **Task 4.1 / Intégration API non faite – CRITICAL**
|
||
**Fichiers:** `main.py`, `utils/exceptions.py`
|
||
|
||
- La story affirme : *"Defined AllProvidersFailedError ... **mapped to HTTP 502 in API layer** (exception handler can catch and convert)"* et la File List indique **Modify: utils/exceptions.py (ALL_PROVIDERS_FAILED), exception handler in main/app**.
|
||
- **Constat:** `utils/exceptions.py` ne gère **pas** `AllProvidersFailedError` ni le code `ALL_PROVIDERS_FAILED`. Aucun handler dans `main.py` pour cette exception. Si du code lève `AllProvidersFailedError`, l’API renverra une 500 ou une erreur non structurée au lieu d’une **502** avec corps JSON (AC2, AC5, NFR12).
|
||
- **Preuve:** `utils/exceptions.py` lignes 43–80 : uniquement `TranslationProviderError` et autres ; pas d’import ni de branche pour `AllProvidersFailedError`.
|
||
|
||
### 2. [2-6] **AC1 / Fallback non utilisé par le flux de traduction – HIGH**
|
||
**Fichiers:** `main.py`
|
||
|
||
- AC1 : *"when the translation service catches the error, **then it tries the next provider**"*.
|
||
- **Constat:** L’endpoint `POST /translate` choisit **un seul** provider (lignes 551–597) et appelle `translation_service.provider = ...` puis plus bas les traducteurs (Excel/Word/PPT). Aucun appel à `translate_with_fallback` ni `translate_with_fallback_by_mode`. En cas d’échec d’un provider, il n’y a **pas** d’essai sur le provider suivant.
|
||
- **Preuve:** `main.py` pas d’import de `translate_with_fallback` / `translate_with_fallback_by_mode` ; pas de `meta.provider_used` dans la réponse (AC3).
|
||
|
||
### 3. [2-6] **AC3 – meta.provider_used absent de la réponse API – HIGH**
|
||
**Fichiers:** `main.py`
|
||
|
||
- AC3 : *"The successful provider name is returned in **meta.provider_used** in the API response"*.
|
||
- **Constat:** La réponse de traduction (ex. retour de fichier traduit / métadonnées) ne contient pas `meta.provider_used`. Même après intégration du fallback, il faudra l’ajouter ; pour l’instant la base (réponse structurée avec `meta`) n’est pas en place pour ce champ.
|
||
|
||
---
|
||
|
||
## 🟡 MEDIUM ISSUES
|
||
|
||
### 4. [2-5] **Task 3.2 – Health check timeout incohérent – MEDIUM**
|
||
**Fichier:** `services/providers/openai_provider.py` (ligne 419)
|
||
|
||
- Le constructeur expose `health_check_timeout` (défaut 5s) et l’utilise dans `is_available()` (ligne 388 : `timeout=self._health_check_timeout`), mais `health_check()` utilise **en dur** `timeout=5` au lieu de `self._health_check_timeout`.
|
||
- **Risque:** Comportement et configuration divergents ; évolution (ex. env) du timeout de health non reflétée dans `health_check()`.
|
||
|
||
### 5. [2-5] **Documentation File List – README non listé – MEDIUM**
|
||
**Fichier:** `2-5-provider-openai-llm-cloud.md`
|
||
|
||
- Task 7.1 : *"Update services/providers/README.md with OpenAI section"*. La section OpenAI est bien présente dans le README.
|
||
- **Constat:** La section **File List** de la story ne mentionne pas `services/providers/README.md` dans "Files Modified". Incohérence pour traçabilité et revue.
|
||
|
||
### 6. [2-6] **Config – ProviderSettings.openai sans base_url – MEDIUM**
|
||
**Fichier:** `services/providers/config.py` (lignes 153–158)
|
||
|
||
- `get_provider_settings("openai")` retourne `base_url=None` alors que `ProvidersConfig.OPENAI_BASE_URL` existe (ligne 64). Les autres providers (ex. Ollama) ont `base_url` renseigné.
|
||
- **Impact:** Tout code qui s’appuie sur `ProviderSettings` pour construire une URL OpenAI ne verra pas la base URL personnalisable (ex. Azure/proxy).
|
||
|
||
### 7. [2-5] **Réponse 429/400 non-JSON – robustesse – MEDIUM**
|
||
**Fichier:** `services/providers/openai_provider.py` (lignes 269–302)
|
||
|
||
- En 429 et 400, le code fait `response.json().get("error", {})` sans try/except. Si l’API renvoie du HTML ou un corps non-JSON, `response.json()` lève `JSONDecodeError`. Celle-ci est rattrapée plus bas par `except Exception`, donc pas de 500, mais l’erreur est mappée en `OPENAI_SERVICE_ERROR` au lieu d’un message plus précis (ex. "réponse invalide").
|
||
- **Recommandation:** Encadrer le parsing JSON (429/400) et remonter un code/message dédié si le corps n’est pas du JSON valide.
|
||
|
||
---
|
||
|
||
## 🟢 LOW ISSUES
|
||
|
||
### 8. [2-5] **AC3 – Libellé du code d’erreur – LOW**
|
||
**Fichier:** Story 2-5, AC3
|
||
|
||
- AC3 dit : *"API rate limits return error **PROVIDER_RATE_LIMITED**"*. L’implémentation utilise `OPENAI_RATE_LIMITED`. Les Dev Notes et l’architecture utilisent bien `OPENAI_*`. Pas de régression fonctionnelle, mais écart par rapport au libellé exact de l’AC (à aligner en doc ou en AC).
|
||
|
||
### 9. [2-5] **.env.example – Story 2-5 File List – LOW**
|
||
**Fichier:** `2-5-provider-openai-llm-cloud.md`
|
||
|
||
- Task 5.3 demande la mise à jour de `.env.example`. Le fichier contient bien les variables OpenAI.
|
||
- **Constat:** "Files Modified" dans la story ne liste pas `.env.example`, seulement `__init__.py` et `config.py`. À ajouter pour cohérence.
|
||
|
||
### 10. [2-6] **.env.example – Story 2-6 File List – LOW**
|
||
**Fichier:** `2-6-provider-fallback-chain.md`
|
||
|
||
- La File List indique "Files Modified: ... .env.example". C’est cohérent avec le contenu actuel. Aucun correctif nécessaire ; mention pour exhaustivité.
|
||
|
||
### 11. [2-5] **Logs – tokens_used non logué – LOW**
|
||
**Fichier:** `services/providers/openai_provider.py` (lignes 334–342)
|
||
|
||
- Les Dev Notes suggèrent de logger `tokens_used=response.usage.total_tokens`. En cas de succès, le code logue `latency_ms`, `retries`, etc., mais **pas** `tokens_used` (la réponse Chat Completions contient `usage`). Utile pour coûts et observabilité.
|
||
|
||
### 12. [2-6] **Tests – pas d’assertion explicite "no document content in logs" – LOW**
|
||
**Fichier:** `tests/test_providers/test_fallback.py`
|
||
|
||
- AC6 et Dev Notes : *"No document content in logs"*. Les tests vérifient les événements de log (ex. `test_logs_failed_attempts`) mais ne vérifient pas explicitement que le **contenu du texte traduit** n’apparaît pas dans les messages. Une assertion sur les arguments passés au logger (pas de `request.text` ou de contenu document) renforcerait la garantie NFR11/NFR16.
|
||
|
||
---
|
||
|
||
## Résumé par sévérité
|
||
|
||
| Sévérité | Count | Stories |
|
||
|----------|-------|--------|
|
||
| CRITICAL/HIGH | 3 | 2-6 (intégration API, fallback, meta.provider_used) |
|
||
| MEDIUM | 4 | 2-5 (health timeout, File List README, JSON 429/400), 2-6 (ProviderSettings openai base_url) |
|
||
| LOW | 5 | 2-5 (AC3 libellé, File List .env, tokens_used), 2-6 (assertion logs) |
|
||
|
||
---
|
||
|
||
## Recommandations d’actions
|
||
|
||
1. **Obligatoire pour 2-6 :**
|
||
- Ajouter dans `utils/exceptions.py` la gestion de `AllProvidersFailedError` (import depuis `services.providers.fallback`) et le mapping vers HTTP 502 avec corps `error.to_dict()`.
|
||
- Enregistrer un exception handler dans `main.py` pour `AllProvidersFailedError` → 502 + JSON (ou centraliser dans `handle_translation_error` si utilisé par l’API).
|
||
- Intégrer `translate_with_fallback_by_mode` (ou équivalent) dans le flux `/translate` (ex. selon mode classic/llm/auto) et exposer `meta.provider_used` dans la réponse.
|
||
|
||
2. **Recommandé pour 2-5 :**
|
||
- Utiliser `self._health_check_timeout` dans `health_check()` au lieu de `timeout=5`.
|
||
- Mettre à jour les File Lists des stories 2-5 et 2-6 pour inclure tous les fichiers modifiés (README, .env.example).
|
||
- Optionnel : parsing JSON défensif pour 429/400 et log `tokens_used` en succès.
|
||
|
||
3. **Optionnel :**
|
||
- Aligner AC3 (2-5) avec le code (`OPENAI_RATE_LIMITED` vs `PROVIDER_RATE_LIMITED`).
|
||
- Ajouter `OPENAI_BASE_URL` dans `ProviderSettings` pour openai dans `config.py`.
|
||
- Renforcer les tests fallback pour interdire tout contenu document dans les logs.
|
||
|
||
---
|
||
|
||
*Revue exécutée selon le workflow BMAD code-review. Fichiers exclus : _bmad/, _bmad-output/, .cursor/, .claude/, etc.*
|