BRIC-18: Fix duplicate HUMAN messages and tool error handling

[Kaiohz]

Jira

BRIC-18

Changes

• Fix 1 (duplicate messages): Add _is_duplicate_human_message() to SendMessageUseCase and StreamMessageUseCase to prevent duplicate HUMAN messages when stream crashes and client auto-retries. Also added openWhenHidden: true in chatApi.ts (composable-ui).
• Fix 2 (tool error crash): Patch ToolNode._handle_tool_errors=True so all tool errors (including Pydantic ValidationError from hallucinated params) are caught and surfaced to the LLM instead of crashing the graph.
• Fix 3 (QA test params): Update test_mcp_bricks_endpoints.py to use document_id + project_unique_id instead of deprecated file_url.
• Added docstring warnings on MCP tools to discourage hallucinated params.
• 19 new unit tests for dedup logic and tool error patching.

Tests

• 284 unit tests pass
• 17 MCP bricks QA tests pass
• Linter clean (ruff + eslint)
• Docker stack rebuilt and healthy

[Kaiohz]

🔍 Code Review — BRIC-18

Score : 8/10 — Bonne PR, des fixes ciblés et bien testés. Quelques points à améliorer ci-dessous.

---

✅ Ce qui est bien

1. Logique de déduplication (_is_duplicate_human_message)

• La logique est simple, correcte et bien documentée. Le pattern last.role == HUMAN && last.content == message && last.status is None couvre bien le cas crash/retry.
• Le NOTE dans le docstring est crucial : il explique le trade-off LangGraph checkpoint vs DB projection. 👍
• Les 6 cas de test couvrent les edge cases principaux (empty, same content, different content, status set, AI role, mixed).

2. Patch ToolNode error handling

• Le monkey-patch est défensif (vérifie nodes.get, getattr, hasattr avant de patcher). Les warnings logués sont utiles pour le debug.
• Le TODO dans le docstring est une bonne pratique → idéalement configurer handle_tool_errors=True à la construction du ToolNode.
• Les 4 tests de patch couvrent les cas nominaux et les cas où le nœud/tools/bound n'existe pas.

3. Nouveau fichier de tests test_stream_message.py

• Bonne séparation des tests par classe (TestIsDuplicateHumanMessage + TestStreamMessageUseCase).
• Utilisation propre de fixtures avec AsyncMock(spec=AgentRunner).

4. Import ordering

• Le déplacement de import json en premier dans stream_message.py suit la convention PEP 8.

---

⚠️ Points à améliorer

1. Duplication de code entre SendMessageUseCase et StreamMessageUseCase

Les deux classes ont une copie exacte de _is_duplicate_human_message() (même docstring, même logique). Si la logique évolue, il faudra synchroniser les deux endroits.

👉 Suggestion : Extraire la méthode dans un utilitaire partagé ou dans une classe mixin/base commune dont les deux use cases héritent. Exemple :

# src/application/use_cases/mixins.py
class DuplicateMessageMixin:
    @staticmethod
    def _is_duplicate_human_message(messages: list, message: str) -> bool:
        ...

2. Monkey-patching _handle_tool_errors — fragilité

Le patch accède à tools_node.bound._handle_tool_errors, ce qui dépend de l'implémentation interne de LangGraph (bound est un attribut privé de la node wrapper). Si LangGraph refactor cet attribut, le patch silencieux ne fera plus rien (juste un warning log).

👉 Suggestion :

• Ajouter un test d'intégration qui vérifie que le patch est bien appliqué sur un vrai graphe LangGraph (pas juste des Mocks).
• Ou mieux, passer handle_tool_errors=True au ToolNode constructor dans la factory de l'agent, comme le TODO le suggère. Le monkey-patch serait alors un fallback de sécurité.

3. Le test test_patch_tool_error_handling_sets_true est partiellement trompeur

tool_node_impl._handle_tool_errors = MagicMock()  # ← init à MagicMock
DeepAgentRunner(graph)
assert tool_node_impl._handle_tool_errors is True  # ← vérifie que c'est True

L'assertion passe car le code écrase _handle_tool_errors avec True, ce qui est correct. Mais le fait d'initialiser à MagicMock() peut laisser penser qu'on teste la valeur initiale. Ce n'est pas un bug, juste un peu confus.

👉 Suggestion : Initialiser à False pour que le test soit plus lisible :

tool_node_impl._handle_tool_errors = False

4. Pas de test d'intégration pour le flux complet de déduplication

Les tests unitaires sur _is_duplicate_human_message sont bons, mais il manque un test qui simule le scénario end-to-end : un crash en milieu de stream → retry du client → vérifier que la DB ne contient qu'un seul message HUMAN.

👉 Suggestion : Ajouter un test d'intégration qui :

1. Crée un thread
2. Envoie un message (simule un crash partiel)
3. Retente le même message
4. Vérifie len(human_msgs) == 1 dans la DB

C'est déjà partiellement couvert par test_execute_skips_duplicate_human_message_when_last_is_pending_human, mais sans le scénario de crash.

5. import json dans stream_message.py

L'import json est présent mais pas utilisé dans les changements de cette PR. Est-ce un résidu d'un changement futur ou déjà utilisé ailleurs dans le fichier ? Si inutilisé, le supprimer (ruff devrait le signaler).

---

📋 Résumé

Critère	Note
Correction des bugs	✅ Les 2 bugs sont bien fixés
Couverture de tests	✅ 19 nouveaux tests, bonne couverture unitaire
Documentation	✅ Docstrings claires, NOTE sur le trade-off
Architecture	⚠️ Duplication de la méthode dedup (à refactor)
Robustesse	⚠️ Monkey-patch fragile (TODO à traiter vite)
Style	✅ Propre, cohérent

Verdict : LGTM avec suggestions mineures. La duplication de _is_duplicate_human_message et le monkey-patch sont les deux points à adresser en follow-up. Pas bloquant pour le merge.

🚀 Score : 8/10