feat: mimise rag response mcp

[Kaiohz]

No description provided.

[Kaiohz]

🔍 Code Review - PR #26

📊 Score: 6/10

---

✅ Points Positifs

1. Tests mis à jour - Les tests unitaires ont été correctement adaptés pour valider les nouvelles structures de réponse
2. Intention claire - La minimisation des réponses MCP est une bonne amélioration pour réduire la payload
3. Séparation des modèles - Création de modèles dédiés pour MCP (McpRagResponse, McpClassicalRagResponse) - bonne pratique

---

⚠️ Problèmes à Corriger

1. 🐛 Bug potentiel dans la logique de transformation

Fichier: mcp_classical_tools.py lignes 49-54

for item in response.queries:
    for chunk in response.chunks:
        if chunk.chunk_id.startswith(item):
            classical_response.rag_response.append(...)

Cette logique semble incorrecte:

• item vient de response.queries (liste de strings = queries générées par multi-query)
• chunk_id.startswith(item) n'a pas de sens : un chunk_id ne peut pas commencer par une query textuelle
• Résultat probable: Liste vide si la logique ne matche jamais

Suggestion: Vérifier l'intention. Si le but est de lier chunks à leurs queries, utiliser plutôt l'index ou un mapping explicite.

---

2. 📝 PR sans description

Aucune description dans la PR. Difficile de comprendre le contexte et la motivation du changement sans lire le diff.

Suggestion: Ajouter une description avec:

• Contexte/Motivation
• Changements principaux
• Impact sur les consommateurs MCP

---

3. 🔤 Typo dans le titre

"mimise" → "minimise"

---

4. 🔧 Inconsistances dans les modèles

query_response.py:

class McpRagResponse(BaseModel):
    rag_response: list[RagResponse]  # Pas de Field()

classical_query_response.py:

class McpClassicalRagResponse(BaseModel):
    rag_response: list[ClassicalRagResponse] = Field(
        default_factory=list, description="..."
    )

Suggestion: Harmoniser les deux modèles pour la cohérence.

---

5. 🔚 Trailing newline manquant

Fichier: classical_query_response.py - pas de newline à la fin du fichier

---

❓ Questions

1. CI: Suppression du build PostgreSQL - est-ce intentionnel? Le PostgreSQL custom n'est plus nécessaire?
2. Breaking change: Cette modification casse-t-elle l'API MCP existante? Les consommateurs doivent-ils être notifiés?

---

📋 Recommandations

• Corriger la logique de transformation dans mcp_classical_tools.py
• Ajouter une description à la PR
• Corriger la typo dans le titre
• Harmoniser les modèles Pydantic
• Ajouter trailing newline
• Confirmer l'intention de la suppression du build Postgres

---

Une fois ces points corrigés, la PR sera prête pour merge. 🚀

[Kaiohz]

🔍 Code Review - PR #26

📊 Score: 7/10

---

✅ Points positifs

1. Architecture propre - Les nouveaux modèles de réponse (McpRagResponse, McpClassicalRagResponse) séparent bien les concerns MCP des responses internes
2. Nommage cohérent - Pattern consistant pour les modèles MCP wrappers
3. Tests à jour - Tous les tests unitaires sont correctement mis à jour
4. Itération visible - Le commit "fix: review" montre une itération rapide

---

⚠️ Points à améliorer

1. Typo dans le titre

Le titre du commit dit "mimise" au lieu de "minimise" (ou "minimize").

2. PR sans description

Aucune description de la PR. Il faudrait ajouter :

• Le contexte/raison du changement
• Les breaking changes pour les clients MCP
• Un exemple de la nouvelle structure de réponse

3. Breaking change non documenté ⚠️

Avant: [{content, file_path}, ...]
Après: {rag_response: [{content, file_path}, ...]}

C"est un breaking change pour tous les clients MCP. Il faudrait :

• Le documenter clairement
• Considérer une version bump
• Ou documenter la migration

4. Problème d"indentation

Dans classical_query_response.py:

class ClassicalRagResponse(BaseModel):
    content: str = Field(
         default="", description="..."  # ← espace en trop avant default
    )

5. Import placement

Dans mcp_classical_tools.py, l"import ajouté est placé après les imports existants :

from fastmcp import FastMCP

from application.responses.classical_query_response import ...  # Devrait être groupé avec les autres imports depuis dependencies

6. Scope de la PR

La suppression des steps PostgreSQL dans .github/workflows/cd.yaml semble non liée au feature "minimise RAG response".

• Créer une PR séparée pour la refacto CD
• Ou expliquer le lien dans la description

---

💡 Suggestions d"amélioration

# Ajouter des docstrings aux nouveaux modèles
class RagResponse(BaseModel):
    """Response model for individual RAG chunks in MCP responses."""
    content: str = Field(default="", description="Textual content retrieved from knowledge base")
    file_path: str = Field(default="", description="Source file path")

class McpRagResponse(BaseModel):
    """Wrapper model for MCP RAG responses. Provides a consistent interface for MCP clients."""
    rag_response: list[RagResponse] = Field(
        default_factory=list, 
        description="List of retrieved chunks from knowledge base"
    )

---

🚀 Recommandations

1. Avant merge:

   • Corriger la typo dans le titre/commit
   • Ajouter une description de PR
   • Documenter le breaking change
   • Corriger l"indentation

2. Post-merge:

   • Considérer un bump de version (semver)
   • Mettre à jour la documentation MCP
   • Notifier les clients utilisant l"API MCP

---

En résumé: Bonne implémentation technique, mais il manque de la documentation et quelques corrections mineures. Le breaking change pour les clients MCP est le point le plus critique à adresser.

🤖 Review automatique par SoluBot