Feat/tool hang logs

[Kaiohz]

No description provided.

[Kaiohz]

test

[Kaiohz]

🤖 Code Review — PR #29 "Feat/tool hang logs"

Note : Cette PR a déjà été mergée (commit e850c63c). Review postée a posteriori pour traçabilité.

---

🎯 Résumé exécutif

Gros refacto structurant (68 fichiers, +1100/-700) qui traite 4 problèmes réels du runtime agent :

1. Outils MCP qui hangent → per-tool timeout (60s) qui convertit un hang en ToolException recoverable par le LLM
2. Graph qui stalle → idle/invoke timeout sur ainvoke/astream (120s) qui abort proprement
3. Logs non-structurés et dupliqués → centralisation dans LogMessage (StrEnum, 249 lignes) + format uniforme avec request_id
4. Erreurs domain non typées → split du monolithe domain/exceptions.py en 9 modules domain/errors/* avec status_code typé (ErrorCode IntEnum)

Le titre "Feat/tool hang logs" sous-estime largement la portée. Le vrai titre serait Refactor: Centralized errors/logs + agent tool/invoke timeouts.

---

✅ Points forts

1. Per-tool MCP timeout (le fix cœur du "tool hang")

async def timed_coro(*args, _oc=original_coro, _to=timeout, _name=tool_name, **kwargs):
    try:
        return await asyncio.wait_for(_oc(*args, **kwargs), timeout=_to)
    except TimeoutError as e:
        logger.warning(LogMessage.MCP_TOOL_TIMEOUT, _name, _to)
        raise ToolException(ErrorMessage.MCP_TOOL_CALL_TIMEOUT.format(...)) from e

Excellent choix : ToolException est capturé par handle_tool_errors=True (patché dans _patch_tool_node_error_handling) → ToolMessage(error) → l'agent peut retry/skip. Pas de kill du process. C'est exactement la bonne sémantique.

2. Idle timeout sur astream est la vraie killer feature pour le bug des tool results perdus. Le commentaire dans le code explique bien : "a tool result was likely lost (flaky transport)".

3. Typage des erreurs avec status_code: int = ErrorCode.X sur chaque classe + handler unique _error_response() dans main.py. Élimine le mapping registry qu'on avait avant.

4. RequestIdMiddleware + ContextVar : corrélation par requête sans avoir à threader le request_id partout. X-Request-ID echoed back, c'est propre.

5. Tests d'intégration test_deepagent_real_graph.py : utilise GenericFakeChatModel + MemorySaver sans API key, et surtout assert le role ToolMessage dans le cycle. C'est ce qui manquait pour catcher les breaking changes de deepagents>=0.6.

6. Migration .trivyignore documentée avec justification (perl-base 32-bit only, Archive::Tar postponé). C'est le bon niveau de commentaire.

7. virtual_mode=False explicite sur FilesystemBackend pour silencer le deprecation warning deepagents>=0.6 et pin le comportement.

8. Suppression du handler ad-hoc _handle_http_error dans routes/prompt.py au profit des domain errors typées. Bonne cleanup.

---

⚠️ Suggestions (post-merge car déjà mergée, à traiter en follow-up)

1. 🚨 CD workflow pointe maintenant sur prd/ au lieu de dev/

- DEPLOYMENT_FILE="flux-repo/dev/composables/composable-agents/deployment.yaml"
+ DEPLOYMENT_FILE="flux-repo/prd/composables/composable-agents/deployment.yaml"

C'est intentionnel (passage dev → prd) mais c'est dans une PR qui s'appelle "Feat/tool hang logs". À vérifier que c'est bien voulu et pas un accident de rebase.

2. 🐛 asyncio.get_event_loop() dans sync_wrapper (mcp/adapter.py)

def sync_wrapper(*args, _tc=timed_coro, **kwargs):
    return asyncio.get_event_loop().run_until_complete(_tc(*args, **kwargs))

get_event_loop() est deprecated quand il n'y a pas de loop courante (Python 3.12+ émet un DeprecationWarning, et ça crash en 3.14 si aucune loop n'existe). Préférer :

try:
    loop = asyncio.get_running_loop()
except RuntimeError:
    loop = asyncio.new_event_loop()
    asyncio.set_event_loop(loop)

Ou mieux : garder les tools 100% async et ne pas wrapper en sync (mais je sais que deepagents appelle en sync, donc le wrapper est nécessaire).

3. 🧪 Tests timeout : valeurs trop serrées (0.05s)

runner = DeepAgentRunner(mock_graph, stream_idle_timeout=0.05)
with pytest.raises(AgentError, match="idle"):

0.05s dans un runner de CI chargé → flaky test. Passer à 0.5s minimum, idéalement 1s. Idem pour invoke_timeout=0.05.

4. 🧹 .DS_Store modifié est commité. À ajouter au .gitignore du repo (pas dans cette PR mais à noter).

5. 📦 mcp>=1.27.0 ajouté aux deps mais le commentaire upstream dit "Keep aligned with the server (mcp-raganything / fastmcp) SDK version" — vérifier que mcp-raganything et composable-agents utilisent bien la même version. Risque de mismatch runtime.

6. 🏗️ delete_agent_config.py invalide le registry APRÈS avoir supprimé du store/repository
Pas de la PR actuelle mais à noter : si une de ces étapes fail, on a un état inconsistant. Pas critique ici car les 3 ops sont rapides et sur le même agent.

7. 📝 src/application/routes/prompt.py : les 3 endpoints n'ont plus de gestion d'erreur HTTP explicite
Bien que _error_response() catch tout via le handler centralisé, le _handle_http_error retiré faisait un mapping plus fin (404 vs 409 vs 400). Le nouveau code délègue tout à ConfigError/StorageError via domain — vérifier que les tests d'intégration couvrent bien les 3 codes de retour HTTP pour les endpoints prompt.

8. 🔍 src/domain/logging/messages.py : 249 lignes, 1 seule classe
Le catalogue est gros. Pour le futur, splitter en sous-modules (logging/messages/agents.py, logging/messages/chat.py, etc.) pourrait aider à la navigation. Pas urgent.

9. ⚙️ agent_invoke_timeout=120.0 par défaut
Pour les agents qui font beaucoup de tool calls (RAG multi-source par exemple), 120s peut être court. À exposer en variable d'env dans une prochaine itération pour tuning par déploiement.

---

📊 Score

Critère	Note	Commentaire
Architecture / Hexagonal	9/10	Respect des boundaries domain/infrastructure parfait
Qualité code	8/10	Lisible, centralisé, mais get_event_loop() deprecated
Tests	7/10	Couverture ajoutée, mais timeouts trop serrés = flaky potentiel
Sécurité	8/10	Bumps de deps OK, trivyignore documenté
Doc / Comments	9/10	Excellent — chaque décision a un commentaire explicatif
Scope discipline	6/10	PR fait 4x plus que le titre ne dit (refacto + fixes + deps)
Risque régression	7/10	Beaucoup de fichiers touchés, mais tests d'intégration compensent

Score global : 8/10

C'est du très bon travail de fond. Les 4 problèmes adressés sont réels (j'ai vu les hangs MCP sur le cluster PickPro) et la solution est solide. Le seul vrai reproche c'est la discipline de scope : une PR qui mélange refacto erreurs + timeouts + deps bumps + .trivyignore est plus difficile à reviewer et à revert en cas de problème. Un split en 4 PRs (errors-refactor, logging-refactor, tool-timeout, deps-bump) aurait été plus chirurgical.

Mais vu la qualité du code et la documentation inline, c'est mergeable en l'état. Les suggestions ci-dessus sont à traiter en follow-up tickets.

---

Review postée par SoluBot 🤖 (review automatique via webhook CI)