Restructure domain errors and centralize log/error messages

[Kaiohz]

• Split monolithic exceptions.py into per-domain error modules with ErrorCode enum for HTTP status mapping
• Introduce ErrorMessage/LogMessage string enums as single-source catalogs for error text and log templates
• Replace ad-hoc logging setup with centralized configure_logging() and RequestIdMiddleware for correlation IDs
• Add MCP tool timeout, agent stream idle/invocation timeouts, and mcp>=1.27.0 dependency
• Bump deepagents, langchain-core, langgraph, cryptography, pyjwt, and langchain-mcp-adapters; add integration test for deepagents graph

[Kaiohz]

Code Review — Restructure domain errors and centralize log/error messages

Grosse refacto (66 fichiers, +1486/-779) avec deux intentions claires : (1) éclater le monolithique exceptions.py en modules par domaine + catalogue de messages, et (2) introduire un setup de logging centralisé avec correlation id et timeouts agent/MCP pour tuer les hangs en prod. Bonne nouvelle : la CI passe, les tests d'intégration du vrai graph deepagents sont une vraie plus-value, et l'ajout des timeouts stream/invoke/outil MCP comble un trou opérationnel.

✅ Points forts

• Catalogue ErrorMessage + LogMessage (StrEnum) : source unique pour tous les messages utilisateurs et de logs, plus de duplication, plus d'audit facile. Très propre, aligné avec ce qu'on a commencé à faire sur mcp-raganything.
• DomainError.status_code: ErrorCode : le handler FastAPI lit directement exc.status_code et exc.detail, plus de mapping table séparée — net et DRY.
• _error_response() centralisé : uniformise la forme de réponse (et garde errors pour ConfigValidationError). Excellent.
• Per-request correlation id (RequestIdMiddleware + RequestIdFilter + ContextVar) : pattern propre, X-Request-ID honoré en entrée et renvoyé en sortie. Indispensable pour débugger en multi-services.
• Timeouts agent + MCP tool :
  • asyncio.wait_for sur ainvoke → fini les hangs silencieux
  • idle timeout sur astream via aiter/anext → capte les tool results perdus (le commentaire le dit explicitement, c'est exactement le bug qu'on a vu)
  • MCP_TOOL_CALL_TIMEOUT converti en ToolException → l'agent peut recover au lieu d'être tué. Bon choix de design.
• Security pins dans pyproject.toml (aiohttp, idna, mako, starlette, urllib3…) : commentaire explicite "trivy" — top, on garde une trace des CVEs.
• Integration test test_deepagent_real_graph.py : c'est exactement ce qu'il manquait pour détecter les breaking changes deepagents (le bump 0.3.12 → 0.6.10 en a probablement introduit). Garde ça en CI obligatoire.
• FilesystemBackend(virtual_mode=False) explicite : documente l'intention et silence le deprecation warning. Bien.

⚠️ Points à vérifier /改进建议

1. Ordre des middlewares dans main.py — app.add_middleware(RequestIdMiddleware) est ajouté avant app.add_middleware(CORSMiddleware, ...). Avec Starlette, le dernier add_middleware ajouté s'exécute en premier (LIFO). Conséquence : CORS tourne AVANT le RequestIdMiddleware, donc les requêtes preflight OPTIONS qui rejettent CORS ne se voient pas attribuer de request_id et n'apparaissent pas dans les logs avec un id (alors qu'elles devraient). Inverser l'ordre pour avoir RequestIdMiddleware exécuté en premier sur toutes les requêtes :

   app.add_middleware(CORSMiddleware, ...)
   app.add_middleware(RequestIdMiddleware)

2. log_level non exposé par configure_logging(settings) — la fonction lit settings.log_level, mais dans uvicorn.run(...) tu n'as plus de log_level ni de log_config explicite. Vérifie que settings.log_level est bien défini dans Settings (sinon AttributeError silencieux). Possible aussi de passer log_config=None à uvicorn pour qu'il ne ré-installe pas ses propres handlers, vu qu'on a déjà configure_logging.

3. _patch_sync_support est @staticmethod → instance method — tu perds self._tool_timeout en static. Bon, je vois que tu l'as bien changée en méthode d'instance dans le diff, mais vérifie qu'aucun appelant n'utilise encore LangchainMcpToolLoader._patch_sync_support(...) en static (un grep \.\\\_patch\\\\_sync\\\\_support( sur le repo).

4. Bug subtil dans le test test_hung_tool_raises_tool_exception — le test await patched[0].coroutine() ne respecte pas le tool_timeout=0.1 de la closure : la closure capture _to=0.1 au moment du patch, donc OK. Mais la version sync (sync_wrapper) utilise asyncio.get_event_loop().run_until_complete(...) — en Python 3.10+ ça émet un DeprecationWarning quand il n'y a pas de loop courante, et ça plante dans un thread sans loop. Vu que deepagents invoque les tools en sync depuis un thread worker, ça va péter en prod. À remplacer par asyncio.run_coroutine_threadsafe(_tc(*args, **kwargs), main_loop).result() ou un bridge équivalent.

5. Catch-all Exception dans les handlers deepagent — agent_error_handler catch AgentError (502), mais le runner DeepAgentRunner.invoke raise aussi AgentError sur TimeoutError (logique), et sur toute Exception (logique aussi). Conséquence : un pydantic.ValidationError du LLM remonte en 502. C'est discutable — un 500 serait plus juste pour les erreurs vraiment inattendues, 502 pour les erreurs d'upstream (LLM/MCP). Proposer un split :

   • TimeoutError / erreurs de transport LLM/MCP → 502
   • autres exceptions internes → DomainError parent (500) via le handler générique

6. app.add_exception_handler vs @app.exception_handler — c'est intentionnel (ordre d'enregistrement explicite, plus lisible), mais l'inversion des décorateurs en commentaires/outils de doc peut troubler. Ajouter un commentaire # Replace @app.exception_handler decorators with explicit add_*() calls for clarity au-dessus du bloc.

7. _STREAMABLE_HTTP_SSE_READ_TIMEOUT = timedelta(minutes=10) — hardcodé dans le module, pas configurable via Settings. Aligner avec les autres timeouts (settings.mcp_tool_timeout, settings.agent_invoke_timeout, settings.agent_stream_idle_timeout) pour qu'ils soient tous pilotables depuis l'env.

8. PromptManagerUnavailableError est enregistré en SERVICE_UNAVAILABLE (503) — OK, mais comme le handler est spécifique et non-catching le requests.exceptions.ConnectionError racine, on a un risque de transformer un 503 transient en 500 si le code appelant raise directement la requests exception. Vérifier que tous les call-sites Phoenix prompt rattrapent bien et re-lèvent en PromptManagerUnavailableError.

9. Tests unitaires de l'integration test_deepagent_real_graph.py — il dépend de GenericFakeChatModel (langchain_core) et create_deep_agent (deepagents). S'assurer que ce test est taggué @pytest.mark.integration ou séparé dans tests/integration/ (ce qui est le cas ici, c'est bon), et qu'il n'est PAS exécuté par le pytest par défaut si on n'a pas les deps installées. À confirmer dans pyproject.toml / conftest.py.

10. .DS_Store commit dans le diff — ajoute-le au .gitignore racine pour éviter le prochain PR qui le re-modifie.

11. Format string vs Lazy % — les LogMessage utilisent %s (lazy), mais ErrorMessage utilise {} (eager via .format()). Cohérent avec l'usage (logs lazy, messages raise eager), mais à documenter dans le module docstring de LogMessage (c'est déjà fait, OK) et dans ErrorMessage (manquant, ajouter "templates are evaluated eagerly via str.format when raised").

12. Pas de tests unitaires pour les handlers FastAPI — app.add_exception_handler est délicat à tester (besoin d'un TestClient). Vu le refacto, ajouter au minimum un test que _error_response rend bien le status_code et le detail corrects, et un test que RequestIdMiddleware propage le header. Sinon une régression silencieuse sur le format de réponse peut passer en prod.

🎯 Score qualité

8.5/10

Excellente PR, intention claire, exécution propre, gros impact sur l'observabilité et la robustesse (timeouts sur 3 chemins critiques : stream, invoke, MCP tool). Catalogue de messages et DomainError.status_code sont exactement les abstractions qu'il fallait. Les 1.5 points retirés sont : (a) l'ordre des middlewares qui peut masquer des preflight CORS dans les logs, (b) le sync_wrapper qui va péter en multi-thread, (c) l'absence de tests sur les handlers HTTP.

✅ Recommandation

Approuver avec changements demandés :

• Inverser l'ordre d'ajout des middlewares (ou ajouter un test qui assert l'ordre)
• Fixer sync_wrapper pour les contextes sans event loop
• Ajouter settings.mcp_sse_read_timeout (ou nom équivalent) pour aligner avec les autres timeouts
• Ajouter .DS_Store au .gitignore

Une fois ces 4 points traités (ou au moins les 3 premiers), on peut merger. Beau boulot 👏