Feat/tool hang logs

[Kaiohz]

No description provided.

[Kaiohz]

PR Review — Feat/tool hang logs

Score : 7.5/10

Le cœur de la PR (timeout par outil MCP + timeout sur stream/ainvoke) est solide et bien testé. La refacto erreurs/logs est propre et améliore la traçabilité. Mais le scope a largement dépassé le titre, et il y a quelques points à corriger avant de merger.

---

✅ Ce qui est bien

• Vrai fix anti-hang : LangchainMcpToolLoader._patch_sync_support enveloppe désormais chaque coroutine dans asyncio.wait_for(tool_timeout) → ToolException → ToolMessage(error). C'est exactement la bonne approche : le LLM peut récupérer (retry/skip) au lieu d'avoir un agent tué. Le test test_hung_tool_raises_tool_exception couvre le cas réel.
• Stream idle detection : DeepAgentRunner._yield_chunks consomme les chunks un par un avec asyncio.wait_for(stream_idle_timeout) sur chaque anext(). Couvre le cas "tool result perdu" (SSE qui drop). aclose() sur le stream iter dans le finally est une bonne hygiène.
• Wall-time timeout sur ainvoke + timeout dédié sur les 3 calls HITL (approve/reject/edit) → cohérent.
• Timeouts configurables via Settings (mcp_tool_timeout, agent_stream_idle_timeout, agent_invoke_timeout) avec défauts sensés (60s/120s/120s).
• Centralisation des messages d'erreur et de log dans domain/errors/messages.py et domain/logging/messages.py (StrEnum) — uniformise, évite la duplication, facilite l'audit et la future i18n.
• Refacto main.py : _error_response(exc) qui lit exc.status_code/exc.detail au lieu d'avoir 8 handlers presque identiques — gros gain en DSU.
• Tests : test_hung_tool_raises_tool_exception, test_fast_tool_succeeds, test_stream_idle_timeout_raises_agent_error, test_invoke_timeout_raises_agent_error — couvrent les vrais chemins de panne.

⚠️ À corriger avant merge

1. .DS_Store est commité (diff Binary files a/.DS_Store and b/.DS_Store differ). C'est un artefact macOS qui n'a rien à faire dans le repo, et .gitignore ne le contient pas. → git rm .DS_Store + ajouter **/.DS_Store dans .gitignore + idéalement un pre-commit hook.

2. Scope creep vs titre : la PR fait 1679+/721- sur 67 fichiers alors que le titre annonce "tool hang logs". Le vrai fix tient en 4 fichiers (logging.py, deepagent/adapter.py, mcp/adapter.py, config.py). Tout le reste (refacto domain/errors/, domain/logging/messages.py, tous les swaps exceptions.py → errors/*.py, les changements de wording dans 25 fichiers) devrait être dans une PR séparée "Refactor: centralize errors & log messages". Ça facilitera le revert si un des deux pose problème.

3. PR sans description : aucun body n'a été renseigné. Pour une PR de cette taille, c'est rédhibitoire. Il faut au minimum : le problème résolu, les breaking changes, comment tester, le lien vers le ticket/incident qui a motivé les timeouts.

4. _patch_sync_support est maintenant @staticmethod → méthode d'instance (diff mcp/adapter.py ligne ~94). Pas un bug, mais ça veut dire qu'on instancie LangchainMcpToolLoader(tool_timeout=…) même quand on n'a aucun serveur MCP configuré — les tests qui font LangchainMcpToolLoader() sans kwarg sont cassés silencieusement s'il n'y a pas de default (il y en a un : 60.0, donc OK, mais à documenter).

💡 Suggestions d'amélioration

• asyncio.get_event_loop() dans sync_wrapper (mcp/adapter.py) — déprécié en Python 3.12+ quand il n'y a pas de loop courante. Préférer asyncio.run_coroutine_threadsafe(_tc(...), ...) si on est dans un thread séparé, ou documenter que cette branche n'est appelée que depuis deepagents (thread principal).
• Pas de log quand un tool timeout déclenche un retry par le LLM : on a MCP_TOOL_TIMEOUT (warning) mais pas de compteur/métrique. Si tu veux piloter la stabilité MCP, expose un compteur (Prometheus/StatsD) ou au moins un log agrégé toutes les N occurrences.
• Stream idle timeout est par chunk : un LLM qui réfléchit 90s entre deux chunks (réponse longue, tool complexe) déclenchera un faux positif. 120s par défaut est OK mais expose-le en config par agent (certains tools RAG sont lents). Le setting existe déjà au niveau global, mais pas overridable par agent.
• LogMessage et ErrorMessage sont des StrEnum : pratique pour le typage, mais ça veut dire qu'on perd l'info de "ce message est utilisé 3 fois, ce warning est mort" qu'aurait donnée une fonction. OK pour ce use case, mais ajoute un test qui vérifie qu'aucun message n'est dupliqué entre LogMessage et les anciennes chaînes hardcodées restantes (il en reste probablement quelques-unes dans les fichiers non touchés).
• src/domain/exceptions.py est supprimé mais le commit ne le mentionne pas dans le message → vérifier qu'aucun import externe ne pointe encore dessus (un grep -r "from src.domain.exceptions" . dans le repo mergé confirmera).

🧪 Manque côté tests

• Pas de test sur le nouveau RequestIdMiddleware (vérifier qu'il génère un id, propage un X-Request-ID entrant, et que l'id apparaît dans les logs).
• Pas de test sur la propagation de mcp_tool_timeout depuis Settings → LangchainMcpToolLoader.
• Pas de test d'intégration qui simule un vrai hang SSE entre un agent et un serveur MCP (le test unitaire mock coroutine à sleep(10), pas la couche transport). Le tests/integration/test_deepagent_real_graph.py ajouté va peut-être couvrir une partie — à vérifier.
• Le test test_hung_tool_raises_tool_exception ne vérifie pas la branche sync (sync_wrapper), qui est celle que deepagents utilise réellement.

📋 Verdict

Le fix principal est bon et prêt à merger après : suppression du .DS_Store, ajout dans .gitignore, description de PR. Le reste de la refacto (errors + log centralization) est propre mais gagnerait à être splitté en une PR dédiée pour faciliter le review et le revert.

Score 7.5/10 — fix utile et bien testé, mais portée trop large + hygiene (.DS_Store, no description) à corriger.