feat: trace events as single source of truth + flatten agents folder

[Kaiohz]

Changes

• Move agent YAML configs from agents/bricks/ to agents/ root level
• Remove obsolete bricks subfolder
• Updated haiku-rag-formation.yaml (minor content change)

[Kaiohz]

PR Review — chore: remove bricks folder structure from agents (#35)

Note on PR scope: The PR title and body describe a folder-flattening chore, but the actual diff is much larger: a 3-revision alembic migration (005→007) introduces a new trace_events table that replaces messages as the single source of truth, with Message reduced to a computed projection. The agent runner (deepagent/adapter.py, +360/-173) is rewritten to emit TraceEvents (HUMAN_MESSAGE, THINKING, CONTENT, TOOL_CALL, TOOL_RESULT, AI_MESSAGE) instead of the legacy StreamEvent/Message flow. New routes, use cases, repositories and a new error message catalogue come with it. I'd recommend renaming the PR to something like feat: trace events as single source of truth + flatten agents folder so reviewers and the changelog reflect what's actually shipping.

That said, the new design is clean and well-thought-out. The hexagonal boundaries (port / adapter / use case) are respected, the entity is frozen=True, the trace is reconstructable, and the agent runner is split into small, testable classifiers (_classify_thinking, _classify_tool_call_chunks, _classify_tool_result). Lots of detail to look at below.

---

Score: 7 / 10 — Approve after a few small fixes

The architecture is solid and the test coverage looks strong. Deducting points for: a misleading PR title, one real bug in the migration downgrade, a metadata_ ORM/attribute naming mismatch, and a "messages" backward-compat attr that can lie. Once those are fixed and the title is fixed, this is a clean ship.

---

Blocking issues (request changes)

1. Alembic 006 downgrade is broken (data not actually back-migrated).
src/alembic/versions/006_migrate_messages_to_trace_events.py's downgrade() builds the INSERT INTO messages statement but truncates the parameter list: the SQL is

INSERT INTO messages (id, thread_id, ro

… and is cut off mid-line. The downgrade is therefore untested and would raise a SQL syntax error the first time anyone tries to roll back. With a 3-step migration that drops the legacy table in 007, an untested downgrade is a real risk for disaster recovery. Either:

• Build the row dict the same way upgrade() does (one dict per row) and use conn.execute(text(INSERT_SQL), values), or
• Drop the downgrade and document that 006/007 are one-way (acceptable, but say so explicitly).

2. metadata_ attribute shadowing will bite consumers.
TraceEventModel.metadata_: Mapped[dict | None] = mapped_column("metadata", JSON, ...) is mapped to the column name metadata (good), but the ORM attribute is metadata_ because metadata is reserved by SQLAlchemy's DeclarativeBase. In postgres_trace/adapter.py you correctly use model.metadata_ to build the domain entity — but postgres_thread/adapter.py's _model_to_thread reads m.metadata_ too (line ~46) which works only because every call site uses the trailing underscore. Anyone writing a new query that does TraceEventModel.metadata will silently hit Base.metadata. Two safer options:

• Rename the ORM attr to event_metadata and keep the column name as metadata (mapped_column("metadata", ..., name="metadata")).
• Or expose a @property metadata(self) that returns self.metadata_, so the public API is stable.

Pick one and apply it consistently in both adapters.

3. Thread.messages is not in insertion order — it will misorder interleaved turns.
Thread.messages (in src/domain/entities/thread.py) sorts by timestamp only:

message_events = sorted(
    (e for e in self.trace_events if e.type in (HUMAN_MESSAGE, AI_MESSAGE)),
    key=lambda e: e.timestamp,
)

If two events of a multi-turn conversation land in the same millisecond (very common for batched persistence and a fresh turn_id per turn), the order falls back to Python's stable sort — which is the iteration order of self.trace_events. That order is only guaranteed by the SQL order_by="TraceEventModel.timestamp" clause in the relationship, which again doesn't break ties. For a turn that emits HUMAN_MESSAGE → AI_MESSAGE followed by a new HUMAN_MESSAGE in <1 ms, the projection can return [HUMAN(t1), HUMAN(t2), AI(t1)] instead of [HUMAN(t1), AI(t1), HUMAN(t2), AI(t2)]. The fix is trivial: sort by (timestamp, sequence) — sequence is the monotonic in-turn counter and is already guaranteed unique within a turn. GetThreadHistoryUseCase.execute does this correctly; the entity's messages property does not.

Strong suggestions (not blocking)

4. PostgresTraceEventRepository.add opens a session per event in the stream path — and the stream use case calls it once per emitted event. That's N round-trips to Postgres per turn (every THINKING / CONTENT / TOOL_CALL chunk is a separate INSERT). It's intentional for durability (so a stream disconnect keeps the partial trace), but the cost is real: a tool-heavy turn with 50 events = 50 inserts. Two pragmatic options:

• Add a flush_in_progress mode on the repo that buffers events in-memory and flushes every N events or T seconds, with a finally: flush() in StreamMessageUseCase.
• Or document the cost explicitly in the use case docstring and make it configurable (settings.lemtrace_flush_interval).

At minimum, add a log line CHAT_STREAM_FLUSH %d events every N events so this is visible in production.

5. trace_repo.list_by_thread / list_by_turn don't LIMIT and don't filter on thread existence first. get_thread_history and list_trace already call use_case.execute(thread_id) first, so the 404 is enforced, but list_by_thread itself will silently return [] for a non-existent thread (no 404), which is inconsistent with add / add_batch that raise ThreadNotFoundError. Pick a side: either always 404 (cheapest: also call _assert_thread_exists here) or never 404 (drop the assert in add and use ON DELETE CASCADE + an empty list). The current asymmetry will confuse anyone calling the repo directly.

6. _make_trace_event for AI_MESSAGE embeds status both in the Message JSON and in the event metadata. In _collect_trace:

yield self._make_trace_event(
    ...,
    TraceEventType.AI_MESSAGE,
    None, None,
    final_message.model_dump_json(),
    {"status": final_message.status.value if final_message.status else None},
)

Message.model_dump_json() already serialises status (it's a field on the entity), and the migration backfills it the same way (payload["status"] = row.status). The redundant metadata.status is harmless but inconsistent: Message.from_trace_event reads status from the JSON payload, so metadata.status is dead weight. Either drop it or document that metadata is the authoritative source and the JSON payload is a snapshot for legacy readers.

7. _patch_tool_node_error_handling monkey-patches deepagents at runtime. The TODO is right — ToolsNode._handle_tool_errors = True is reaching into private state. Two cleanups:

• If the project's pinned deepagents version exposes handle_tool_errors at construction (most do), set it in factory.py via create_deep_agent(tools=[...], handle_tool_errors=True).
• If not, wrap the patch in a try/except for AttributeError and log at WARNING (not INFO) the first time it fires — currently the hasattr guard means the WARNING logs are noise that nobody reads.

8. _extract_source is fragile to langgraph's NS_SEP. parts.index("task") will raise ValueError if "task" ever disappears from the namespace, and the helper assumes the third-to-last token is always the subagent name. Add a unit test that pins the expected output for a known checkpoint namespace (e.g. "tools:task:security-auditor:tools" → "security-auditor") and another for the parent-agent case ("agent" or "" → None). Right now the only path to "subagent trace events" is the integration test, which is slow and runs against a real graph.

9. The PR adds 4 new agent YAMLs but no docs / catalog link. The README.md change (+260/-40) likely covers the trace-events refactor, but the 4 new agents (haiku-files-local-structured.yaml, haiku-rag-formation.yaml, haiku-rag-local.yaml, orchestrator-test-structured.yaml) are not referenced anywhere. If they're meant to ship, they need a catalog entry (the existing README table is gone after the refactor — please confirm) or a CHANGELOG line. If they're test fixtures, they shouldn't be in agents/ — move them to tests/fixtures/agents/ or agents/examples/.

10. _unpack_stream_item is dead-branch-heavy. The del metadata, source in _classify signals the params aren't used downstream — that's a code smell. Either thread metadata through (source is already extracted upstream) or drop the parameters from _classify entirely. Right now the signature lies about what the function uses, which is a future-bug magnet.

Nitpicks

• stream_message.py line ~50: the broad except Exception inside the inner try re-raises as StorageError and then is caught by the outer except Exception that logs and re-raises again. The error path is StorageError → logged as "stream error" → re-raised as StorageError. The intent is probably to log the underlying persistence error and surface a clean domain error, but the double log is confusing. Drop the inner logger.exception and keep just the raise StorageError(...) from exc — the outer use-case logger will pick it up.
• dependencies.py: the new CompositionRoot dataclass replaces module-level globals but the old global security and tracing_provider are still module-level. That's fine, but worth a one-line comment in the dataclass that some singletons (security, tracing_provider) stay module-level on purpose because they're not async-init.
• haiku-rag-formation.yaml "minor content change" per the PR body — the new agent's prompt is good, but the URL is http://raganything-api:8000/classical/mcp (no auth header) while haiku-rag-local.yaml sets X-API-Key: "${MCP_RAGANYTHING_API_KEY}". Two agents, two contracts against the same backend — pick one and use it everywhere, or document the difference.
• add_batch uses session.add_all(...) but the model metadata_ column is JSONB (server-side default) — fine, but the Text columns on content and name lack an explicit nullable=True (they default to nullable in SQLAlchemy when Mapped[str | None] is used, but it's clearer to be explicit in the migration).
• trace_event_repository.py's list_messages is implemented but never called from any use case (the use cases go through list_by_thread + the messages property). If it's a public API for the future, keep it; if not, drop it to avoid the maintenance surface.

What's good

• Architecture is clean. The hexagonal split is respected: the runner port emits TraceEvent, the persistence port stores TraceEvent, the entity is immutable, the use case is purely a projection. No layer leaks.
• Migration is reversible in shape (downgrade schema exists, even if 006's data downgrade is broken — fix that, and you're golden).
• The Message.from_trace_event projection is a beautiful example of "make the new schema the source of truth, derive the old shape lazily." It keeps every downstream consumer working without changing their contracts.
• Test coverage is strong — test_trace_event, test_trace_repository, test_get_thread_history, test_send_message, test_stream_message, test_routes (incl. /trace and /history integration tests). 194 lines of new tests for the use case alone.
• frozen=True on TraceEvent and the sequence ordering make the entity correctly immutable and totally reconstructable. Nice.
• lazy="raise" on the trace_events relationship in ThreadModel is the right call — silent lazy loads are a classic N+1 trap and you've eliminated it.
• The stream use case persists before yielding. This is the right durability-vs-latency trade-off for a chat API and the code is clear about it.
• make_validation_model hashes the schema so Pydantic re-uses the class across invocations — small but real performance win for repeat calls with the same response_format.
• The error catalogue is centralised in domain/logging/messages.py and domain/errors/messages.py. Greppable, auditable, and the new trace events added their own error messages (TRACE_FAILED_ADD, etc.) instead of inlining strings.

Pre-merge checklist

• Rename the PR (it's not just a folder flattening)
• Fix alembic 006 downgrade or document it as one-way
• Rename metadata_ or add a metadata property on the model
• Sort Thread.messages by (timestamp, sequence)
• Make list_by_thread 404 symmetric with add / add_batch, or drop the assert in add
• Add _extract_source unit tests
• Either move the 4 new agents to a examples/ or tests/fixtures/agents/ subfolder, or add them to the catalog
• Verify the integration test (test_deepagent_real_graph.py) still passes — only +1/-3 lines there, and it exercises the whole new trace-events flow

Solid work. The new design is a real improvement over the dual messages / stream_event mess; this is the kind of refactor that pays for itself the first time you need to add a tool_call event to a persisted thread.

[Kaiohz]

PR Review — feat: trace events as single source of truth + flatten agents folder (#35)

Score: 8.5/10 — Très gros refactor architectural (74 fichiers, +4185/-1357) qui remplace l'ancien modèle messages par un store d'événements immutable trace_events, et qui aplatit agents/bricks/ → agents/single/. La CI passe (test ✓). La description de la PR est trompeuse : c'est ~80% refactor TraceEvent et ~20% flatten.

---

✅ Points forts

• Vraie single source of truth : la table trace_events capture maintenant tous les types d'événements (HUMAN_MESSAGE, AI_MESSAGE, THINKING, CONTENT, TOOL_CALL, TOOL_RESULT) avec un turn_id UUID v4 + un sequence monotone. Message devient une projection réversible de HUMAN_MESSAGE + AI_MESSAGE via Message.from_trace_event — propre et backward-compatible.
• Migrations Alembic propres (005/006/007) : la 005 crée la table avec 3 index bien choisis (thread_turn, thread_type, thread_ts pour les 3 access patterns). La 006 est réversible : upgrade() migre les anciens rôles messages vers les types trace_events (human/ai/tool/system), et downgrade() reconstruit la table messages en déserialisant le JSON de l'AI_MESSAGE payload. La 007 drop simplement.
• Use cases minces et clairs : SendMessageUseCase génère un turn_id, invoke le runner, persiste via add_batch() en un seul round-trip. StreamMessageUseCase persiste chaque event au fil de l'eau via add() — la trace survit même si le client se déco en plein stream.
• Architecture hexagonale respectée : nouveau port TraceEventRepository (ABC), adapter PostgresTraceEventRepository qui ouvre sa propre session par méthode, TraceEvent immutable (frozen=True) avec StrEnum pour les types.
• Décomposition du runner : _classify_thinking, _classify_tool_call_chunks, _classify_tool_result, _extract_source (sous-agent depuis langgraph_checkpoint_ns). _collect_trace yield HUMAN_MESSAGE → intermediates → AI_MESSAGE avec sequence monotone.
• Test coverage solide : nouveau test_trace_repository.py (8 tests : add, add_batch, list_by_turn, list_messages, thread_not_found, empty), test_get_thread_history.py (194 lignes), test_send_message.py (+133/-55), test_stream_message.py (+100/-61), test_trace_event.py (147 lignes), test_schema_utils.py (+448/-4 sur 81 cas probablement).
• Migration de la config agents : tous les YAMLs de bricks/ → single/, README +260 lignes (nouveau format response_format natif avec AutoStrategy Provider/Tool langchain, haiku-rag-formation.yaml mis à jour), agents/README.md créé.
• README bien synchronisé : nouveau bloc "Breaking change (trace events)" qui prévient de la suppression de StreamEvent SSE et de la table messages. Section "Structured Output" qui documente la nouvelle voie native (vs l'ancien hack _create_response_tool).
• CompositionRoot dataclass dans dependencies.py au lieu de globals mutables — bonne pratique déjà en place.
• TODO laissé visible : _patch_tool_node_error_handling reconnaît lui-même que c'est un hack de runtime et suggère de le faire à la construction du ToolNode dans le factory.

💡 Suggestions (mineures, à discuter — pas bloquantes)

1. API privée langgraph._internal._constants.NS_SEP (ligne ~46 de infrastructure/deepagent/adapter.py) : from langgraph._internal._constants import NS_SEP utilise un chemin préfixé par _internal_. LangGraph peut le renommer ou le déplacer à n'importe quelle release mineure (ils le font déjà régulièrement). Suggéré : tester l'import au démarrage avec un fallback propre, ou extraire le séparateur depuis un appel str(metadata["langgraph_checkpoint_ns"]) côté runtime plutôt que d'importer le nom.

2. Migration 006 — tool_call_id perdu sur les tool messages : _build_event_row mappe messages.role='tool' → trace_events.type='tool_result' mais ne récupère pas le tool_call_id (qui n'existait probablement pas comme colonne dans l'ancienne table messages, d'où le passage par metadata.tool_calls). Si l'ancien modèle stockait tool_call_id quelque part, il est perdu. À vérifier : la table messages legacy avait-elle une colonne tool_call_id ? Si oui, il faut l'ajouter à _fetch_messages + _build_event_row. Le code runner actuel stocke bien tool_call_id dans metadata pour les TOOL_RESULT, donc la migration est probablement OK si la legacy table n'avait pas cette info — à confirmer.

3. Race condition possible sur add_batch : si un client fait 2 add concurrents sur le même turn_id, les sequence peuvent se chevaucher (chacun appelle seq localement dans le runner, OK pour un seul turn). Mais 2 turns concurrents sur le même thread → add_batch se base sur le compteur séquence du runner, pas de la DB. Vu que chaque turn a son propre runner invoke, c'est probablement safe en pratique. À documenter dans une ADR si pas déjà fait.

4. Message.from_trace_event lève ValueError sur un type autre que HUMAN/AI_MESSAGE : c'est OK pour le contrat actuel mais ça veut dire qu'on a une exception runtime non-typée domain (ValueError Python) qui peut remonter du use case. Suggéré : créer un MessageBuildError (déjà dans domain/errors/ probablement) pour que le use case puisse le catch et le convertir en AgentError 500 cohérent.

5. ThreadModel.trace_events avec lazy="raise" : si jamais un autre code accède à thread.trace_events sans avoir explicitement chargé la relation (via selectinload ou joinedload), SQLAlchemy lèvera une MissingGreenlet ou DetachedInstanceError. C'est une décision volontaire et OK si tous les accès se font via trace_repo.list_by_thread(...). À documenter en commentaire, sinon le prochain dev qui touche le code va se faire piéger.

6. CompositionRoot reset() : il y a un def reset() dans dependencies.py qui mut None tous les attributs du dataclass — mais c'est exposé publiquement. Suggéré : le préfixer avec _ (ou le rendre package-private) pour éviter qu'un test l'appelle accidentellement en prod.

7. README — section "Multi-Agent Architecture" : la mention "Each TraceEvent they emit includes the sub-agent name in its source field" est cohérente avec _extract_source mais ça veut dire que le client doit connaître la liste des sub-agents pour grouper. Suggéré : ajouter un exemple concret d'utilisation côté frontend (même minimal) ou un lien vers la doc.

8. Test manquant potentiel : pas vu de test d'intégration qui round-trip un turn complet (HUMAN → THINKING → TOOL_CALL → TOOL_RESULT → AI_MESSAGE) et vérifie l'ordre et la persistance via add_batch. Le test test_send_message.py mocke le runner — un test du _collect_trace end-to-end avec un fake graph serait précieux. (Note : test_runner_tracing.py existe peut-être déjà, à vérifier.)

📋 Verdict

APPROVE avec commentaires. Le refactor est bien pensé, bien testé, bien migré. La CI est verte, l'archi hexagonale est respectée, le format TraceEvent est extensible (rajouter un type plus tard = juste un StrEnum + un cas dans _classify). Le seul vrai risque est l'import langgraph._internal._constants qui devrait être neutralisé avant un upgrade de langgraph. Le flatten de agents/ est un bonus et bien géré (README + table bricks legacy supprimée proprement).

Solide travail. 🚀

[Kaiohz]

Code Review — PR #35: feat: trace events as single source of truth + flatten agents folder

Summary

Substantial refactor that introduces trace_events as the new single source of truth, replaces the legacy messages table via a 3-step migration (005 → 006 → 007), and flattens the agents folder structure. The Message entity is downgraded to a backward-compatible computed_field projection over trace_events. Architecture hexagonal respected end-to-end (new TraceEventRepository port + PostgresTraceEventRepository adapter). Squash commit, 75 files, +4205/-1357.

Score: 7.5 / 10

Solid architectural move, very clean refactor of the runner and schema utilities, good test coverage of the new use case and repository. The follow-up review after the force-push (#1 → #2) confirms concerns from the previous round were addressed (PR is now mergeable, all critical bugs from review #1 are fixed). However a few real bugs and a couple of architectural smells remain.

---

✅ Strengths

1. Message as a computed_field projection over TraceEvent — excellent use of Pydantic's computed_field to keep backward compat without duplicating state. The messages field is rebuilt on access from trace_events, and any consumer that reads thread.messages keeps working unchanged.

2. Message.from_trace_event static factory — single, well-documented entry point for reconstructing Messages from events. Handles HUMAN_MESSAGE and AI_MESSAGE only, raises MessageBuildError for other types. Clean.

3. TraceEvent is frozen=True with a StrEnum type discriminator — immutable, type-safe, serializable. Exactly the right model for an event-sourced log.

4. Migration downgrade is full — 006's downgrade rebuilds the messages table AND backfills HUMAN_MESSAGE + AI_MESSAGE rows from the trace events (it even reconstructs the JSON payload for ai_message). This is rare and valuable; most teams skip downgrades.

5. New ports and use cases are clean — TraceEventRepository port, GetThreadHistoryUseCase, TraceEventType enum, ThreadHistory/Turn response DTOs. Hexagonal boundaries respected.

6. Schema utilities refactor — the new schema_to_pydantic_model handles enum, anyOf, and type: ["a", "null"] (nullable unions) where the old make_validation_model only handled primitives and objects. Better correctness.

7. Tests are thorough and realistic — test_get_thread_history covers the chronological-vs-alphabetical ordering edge case (turn_id is a random UUID, so it must NOT be used for ordering), and test_send_message.test_each_call_generates_new_turn_id validates turn_id uniqueness across consecutive calls. This is the right kind of paranoia.

8. Flatten agents/ folder — the bricks/ subfolder was never useful, dropping it is a clear win.

9. All review #1 critical issues are fixed — Message, MessageModel, MessageNotFound/etc. errors are gone; the only messages import remaining in dependencies.py is the static ErrorMessage enum entry. The PR is now mergeable: clean.

---

🐛 Bugs

1. test_invoke_timeout_raises_agent_error test is broken — passes for the wrong reason

tests/unit/test_deep_agent_runner.py lines ~366-378:

async def _astream_hang(_input, **_kwargs):
    await asyncio.sleep(10)
    return
    yield  # noqa: F841

graph.astream = _astream_hang

The return statement before yield makes this an async generator that yields zero times before returning. The first await anext(stream_iter) in _stream_intermediate_events will hit StopAsyncIteration immediately and exit the loop — never reaching the await asyncio.sleep(10) and never triggering the idle timeout.

The test will pass, but it doesn't actually test what it claims to test. The test should await asyncio.sleep(10) before any yield, or use await asyncio.sleep(0.05); yield (...) to simulate a slow chunk.

Fix: either yield at least once after a sleep, or test the timeout against _stream_intermediate_events with a producer that yields slowly.

2. Migration 006 downgrade drops tool_result events

src/alembic/versions/006_migrate_messages_to_trace_events.py:

• Upgrade maps role="tool" → trace_events.type="tool_result" (preserving tool_calls and status in metadata).
• Downgrade SELECT only filters type IN ('human_message', 'ai_message') — tool_result rows are silently dropped.

If someone rolls back to the legacy messages table, the tool messages vanish. The downgrade should either:

• (a) backfill role='tool' rows from the tool_result trace events, or
• (b) document loudly that tool messages are lost on downgrade (acceptable trade-off, but must be explicit in the docstring of the downgrade).

The upgrade docstring is already honest about the same tool_call_id limitation, so the precedent is set. Just apply the same honesty to the downgrade.

3. _build_response lost the _try_parse_json fallback

The old adapter had a 3-step fallback for structured_response:

1. extract from tool_calls (ToolStrategy mode)
2. extract from result["structured_response"] (ProviderStrategy native)
3. fall back to parsing the last message content as JSON (covers legacy agents that emit JSON in their text content)

Step 1 is also gone. The new logic is:

# 1. Native structured_response (ProviderStrategy/ToolStrategy native mode).
raw_structured = result.get("structured_response")
if hasattr(raw_structured, "model_dump"):
    structured_response = raw_structured.model_dump()
elif isinstance(raw_structured, dict):
    structured_response = raw_structured

If an agent still emits tool_calls=[{"name": "structured_response", ...}] (the pre-Ticket-3 pattern) or JSON in its content, the field stays None and validation against the response_format model is skipped. That may or may not be intentional, but it's a behavior change and there's no migration guide. Worth either:

• (a) keeping the old fallbacks (cheap, defensive), or
• (b) explicitly documenting that the legacy patterns are unsupported post-migration.

Same comment for the legacy _extract_source parsing of langgraph_checkpoint_ns: the test for this is missing entirely.

4. stream_message use case: hard-coded chunk_count=0 on error

src/application/use_cases/stream_message.py:

except Exception:
    logger.exception(LogMessage.CHAT_STREAM_ERROR_UC, thread_id, thread.agent_name, 0)
    raise

The previous version tracked a real chunk_count. The new one always logs 0, which makes the log line useless for production debugging. If persist-on-each-event throws on the 5th event of a 20-event turn, the log will say "0 events" and operators will be misled.

Fix: track a counter and pass the actual count.

5. first_chunk log message removed without replacement

The old adapter logged LogMessage.AGENT_FIRST_CHUNK on the first chunk with elapsed time — useful for monitoring TTFT (time-to-first-token) in production. The new adapter silently dropped this. Not a bug per se, but it's a regression in observability.

---

🏗️ Architectural smells

6. del metadata, source in _classify is a code smell

@staticmethod
def _classify(chunk, metadata: dict, source: str | None) -> list[ClassifiedEvent]:
    del metadata, source
    events: list[ClassifiedEvent] = []
    ...

Using del to mark unused parameters is non-Pythonic and confusing — readers will think you're cleaning up locals. Either:

• prefix unused args with _ (_metadata, _source) so the linter accepts them, or
• drop them from the signature entirely (they're not used by the classifier; source is computed in _stream_intermediate_events and passed in by the caller but the classifier ignores it).

The latter is cleaner: _classify(chunk) doesn't need either.

7. NS_SEP fallback hides a brittle dependency

try:
    from langgraph._internal._constants import NS_SEP
except ImportError:
    NS_SEP = "|"

Importing from langgraph._internal is by definition unstable — the underscore prefix signals it can change without notice. If langgraph ever renames or relocates this constant, subagent detection silently falls back to "|" and may parse namespaces incorrectly without any warning. Consider:

• wrapping the import in a custom SubagentSourceExtractor interface (port) with the current implementation in infrastructure/deepagent,
• logging a warning on the ImportError so the dependency drift is visible,
• or pinning the langgraph version that ships this symbol.

8. stream_message and send_message use cases don't validate body.message early

/api/v1/threads/{id}/chat/stream accepts a ChatRequest. Looking at the new stream_message route, it passes body.message or "" to the use case — silently converting a missing message to an empty string. The use case then calls runner.stream(thread_id, "", turn_id) which is meaningless. The previous version also had this bug, but the refactor was a good moment to add a 422 if body.message is empty/whitespace.

Same in SendMessageUseCase.execute — the early HITL-action validation is great, but there's no equivalent check for if message and not message.strip(): raise InvalidRequestError.

9. ThreadHistory.turns ordering is by turn_order (insertion order), not the explicit sort key in the docstring

get_thread_history.py:

sorted_events = sorted(trace_events, key=lambda e: (e.timestamp, e.sequence))
turns_map: dict[str, list] = defaultdict(list)
turn_order: list[str] = []
for ev in sorted_events:
    if ev.turn_id not in turns_map:
        turn_order.append(ev.turn_id)
    turns_map[ev.turn_id].append(ev)

The code is correct (turn_order is appended in iteration order of sorted_events, which is by timestamp), but it's indirect. A more explicit version:

turn_order = list(dict.fromkeys(e.turn_id for e in sorted_events))  # first-seen order preserved

would be one line, no defaultdict, and self-documenting. Not blocking, just nicer to read.

10. The new src/application/routes/trace.py only validates thread existence but the response payload is a free-form dict

@router.get("/{thread_id}/trace") returns dict, not a typed DTO. The events field is then serialized through Pydantic's model_dump of TraceEvent — but the response type is dict[str, list[TraceEvent]]. Should be a typed DTO TraceListResponse(events=list[TraceEvent]) to keep the contract explicit and OpenAPI accurate.

---

📋 Minor / nits

• PR description is too short — "Move agent YAML configs from agents/bricks/ to agents/ root level" and "Updated haiku-rag-formation.yaml (minor content change)" misses the biggest change in the PR (the trace_events migration). Reviewers had to discover the architectural shift by reading the diff. A two-paragraph description with "WHY" (single source of truth, eliminate the dual-write to messages + langgraph checkpoint) and a short "MIGRATION" section would help reviewers and future-archeologists.

• .DS_Store was committed (agents/bricks/.DS_Store → renamed to root with no content change). macOS artifact. Should be in .gitignore.

• STORAGE_FAILED_PERSIST_STREAM error — used by stream_message when trace_repo.add fails, but the ErrorMessage enum is in messages.py and the format is {error}. The error chain is helpful but the user-facing message is just "Failed to persist: ". Consider a more user-friendly message in the route layer and only log the technical detail.

• tests/unit/test_thread_management.py formatting changes — pre-existing YAML tests had multi-line strings reformatted to one-liners, presumably by a formatter. The result is harder to read. Pure noise in the diff; should have been a separate commit or skipped.

• No integration test for the new GET /threads/{id}/history and GET /threads/{id}/trace routes — unit tests cover the use cases but not the FastAPI route layer. Same gap as for the chat routes, but a 2-3 line TestClient test would catch schema mismatches.

• schema_utils._build_array_model doesn't handle items: {anyOf: [...]} — only handles items: {type: "object"} vs primitive. If a JSON schema has items: {anyOf: [{type: "string"}, {type": "null"}]} (an array of nullable strings), the current code falls through to list[str], which is wrong. Probably out of scope but worth noting.

• tests/unit/test_trace_event.py uses __import__("json").dumps(payload) instead of a clean import json at the top of the file. Unusual style, looks like it was added under deadline pressure.

---

🎯 Verdict

APPROVE with minor changes recommended (would have asked for changes before #2 due to the test timeout bug, the migration downgrade data loss, and the _build_response regression).

Before merge:

• Fix test_invoke_timeout_raises_agent_error so it actually tests a timeout (1-line change).
• Decide on the 006 downgrade policy for tool_result events and update the docstring (or backfill them).
• Restore a chunk_count in stream_message error log (or accept the regression and remove the parameter from the log message).
• Add an integration smoke test for /threads/{id}/history.

Nice to have (post-merge):

• Restore first_chunk log line with TTFT.
• Replace del metadata, source with proper _ prefix.
• Move _extract_source to a port with a fallback warning for the langgraph internal import.
• Expand the PR description to explain the "why" of the trace_events migration.

The architectural decision is sound: trace_events as a single source of truth, Message as a projection, hexagonal ports everywhere, and a real (not faked) backward-compat layer. The implementation is mostly clean and the tests are above average. The bugs listed are small and well-isolated. Solid work.

[Kaiohz]

PR Review — feat: trace events as single source of truth + flatten agents folder

Score global : 7/10 — refactor de fond bien pensé et aligné avec les principes hexagonal/SOLID du repo, mais avec quelques angles morts (idempotence des retries, persistance HITL, tests manquants sur schema_utils, migration downgrade fragile). CI verte (test ✓), PR mergeable, 1 commit (squash ready), 78 fichiers / +4219 / −1356.

---

✅ Points forts

1. Migration Message → projection des TraceEvent — Message.from_trace_event() + Thread.messages en computed_field → backward-compat gratuite pour tous les call-sites existants. Excellent design, le bon tradeoff.
2. Migration Alembic en 3 étapes (005 create → 006 backfill → 007 drop) avec docstrings claires et le caveat downgrade bien noté.
3. Découplage AgentRunner.invoke → (Message, list[TraceEvent]) — le use case batch-persiste via add_batch, le runner n'a plus aucune dépendance à la persistance. Hexagonal respecté.
4. Native response_format dans la factory — suppression de l'injection de structured_response tool custom + de l'instruction STRUCTURED_OUTPUT_INSTRUCTION. Passage en mode natif langchain/deepagents, plus simple et plus compatible avec les providers strict-mode.
5. GetThreadHistoryUseCase testé sur les edge cases critiques : turn crashed (no AI_MESSAGE), ordering chronologique vs UUID v4, empty thread. C'est exactement le piège du turn_id random qui aurait pu silencieusement inverser l'ordre — bien vu.
6. lazy="raise" sur ThreadModel.trace_events — bloque les N+1 silencieuses. À généraliser aux autres relations du repo.
7. Index DB bien pensés : (thread_id, turn_id), (thread_id, type), (thread_id, timestamp) — couvrent les 3 access patterns (list_by_turn, list_messages, list_by_thread).
8. Tests d'intégration réels (SQLite in-memory) sur PostgresTraceEventRepository au lieu de mocks. Meilleure confiance qu'un test mocké.
9. Validation HITL déplacée en tête de SendMessageUseCase.execute → contract 422 préservé avant tout I/O, évite un round-trip DB inutile.
10. README mis à jour avec section "Breaking Changes" + format SSE documenté. Rare sur ce repo, appréciable.

---

🐛 Bugs / comportements à risque

1. Perte de la détection de duplicate human message (changement silencieux)

send_message.py et stream_message.py ont supprimé _is_duplicate_human_message() (le helper de retry-safety). Avant : si le client retry après un crash mid-turn, on évitait d'insérer 2x le HUMAN_MESSAGE. Maintenant : turn_id UUID v4 généré par le use case à chaque appel → un retry produit 2x HUMAN_MESSAGE (avec des turn_id différents) dans la table. Comportement changé silencieusement, à documenter ou compenser.

Suggestion : soit ajouter le check au début (list_messages → si dernier event est HUMAN_MESSAGE avec content == message et pas d'AI_MESSAGE pour le même turn_id, retourner le thread state) ; soit documenter explicitement la nouvelle sémantique (idempotence = responsabilité client).

2. DeepAgentRunner.invoke peut double-invoquer le LLM

Si _collect_trace ne yield aucun AI_MESSAGE (graph mal-comporté, tool error silencieux, etc.), le fallback ré-appelle self._graph.ainvoke() une 2e fois. C'est potentiellement 2 appels LLM payants pour 1 turn. Le commentaire dit "shouldn't happen with a well-behaved graph" mais c'est un piège prod.

Suggestion : lever AgentError(ErrorMessage.AGENT_NO_FINAL_MESSAGES) comme _build_response le fait déjà, plutôt que de re-invoker.

3. HITL ne persiste rien → turn history incomplète

SendMessageUseCase HITL path : "no trace persistence — HITL does not currently emit trace events". Conséquence : un user qui rejette 3 tool calls successifs puis approuve aura dans /history des turns ai_message sans human_message/intermédiaires, et l'ordering chronologique des turns sera trompeur.

Suggestion minimale : persister un AI_MESSAGE minimal dans le HITL path (via add direct) avec un flag metadata={"hitl": true} pour que la history reste cohérente. Ou documenter explicitement le mode dégradé.

4. Downgrade Alembic 006 → 007 perd des données

Déjà documenté dans le code ("losing them on rollback does not affect conversation history"), mais : la perte concerne tool_result + content events. Si un user a une vraie trace d'agent avec tool calls, downgrade = perte d'audit. C'est acceptable mais devrait être dans le revision message affiché par alembic history (pas juste un docstring).

---

🧪 Tests manquants / couverture

5. schema_utils.py (100 lignes ajoutées) n'a pas de tests directs. Tout passe par test_factory.py avec WEATHER_SCHEMA simple. Les cas anyOf, enum, type: [a, b, "null"] (utilisés massivement dans haiku-files-local-structured.yaml) ne sont pas testés en isolation. Bug latent : Literal[tuple(values)] casse si values contient des non-hashables.

6. Pas de test d'intégration Alembic sur la migration 006 (legacy messages → trace_events). Le mapping role→type, le cas ai_message JSON payload, le cas tool_result metadata — tout est non-testé. Le filet de sécurité c'est la lecture du code, c'est insuffisant pour une migration de prod.

7. Pas de test du chemin subgraphs=True dans DeepAgentRunner._collect_trace. Le code change stream_mode="messages" + subgraphs=True pour capturer les events des sub-agents, mais aucun test ne vérifie que les events source="security-auditor" remontent bien.

8. Pas de test sur _extract_source — la détection parts.index("task") est hard-codée. Un test unitaire ns="Agent:task:security-auditor:tools" → "security-auditor" figerait le contrat.

---

🛠️ Suggestions de qualité

9. router /threads/{id}/trace retourne dict non typé alors que /history retourne un ThreadHistory(BaseModel). Incohérence. Créer TraceListResponse(BaseModel) avec events: list[TraceEvent].

10. WebSocket termine par [END], SSE par [DONE] — incohérence cross-channel préservée de l'ancien code. La refacto était l'occasion d'aligner.

11. Thread.messages (computed_field) recalcule sort+filter à chaque accès — sur thread_repo.get puis lecture, on refait le travail plusieurs fois. @cached_property (Pydantic 2) ou matérialiser une fois.

12. Schema trace_events.sequence est Integer NOT NULL DEFAULT 0 — sur 2 events insérés dans la même µs (peu probable mais possible), order_by(timestamp, sequence) est non-déterministe. Considérer BigSerial côté DB ou générer le sequence côté Python avec une garantie d'unicité (ex: microsec + counter).

13. add_batch n'est pas streamé — OK jusqu'à ~10k params PG, mais un turn halluciné avec 5000 THINKING chunks dépasserait. À surveiller.

14. _apply_optional_kwargs est un wrapper de 4 if qui ne fait que factoriser. Avant c'était 4 if explicites plus lisibles. Trivial mais du over-engineering.

15. NS_SEP importé depuis langgraph._internal._constants est un import private API. Susceptible de casser à toute upgrade langgraph. Remonter un ticket upstream, ou commenter pourquoi on ne peut pas faire autrement.

16. Le __init__.py de postgres_trace/ est vide (+0 -0). C'est normal pour un namespace package, mais pas explicite — ajouter un commentaire ou un re-export des éléments clés.

---

📋 Hors-scope mais vu en passant

• .DS_Store trackés dans le repo et supprimés par cette PR — c'est OK mais considérer l'ajout d'un *.DS_Store global au .gitignore racine pour éviter le prochain.
• L'aplatissement agents/bricks/ → agents/ est propre, le nouveau agents/README.md est bien fait. RAS.

---

🎯 Pour ship

Bloqueurs à mon sens : #1 (duplicate human message) et #5/#7 (manque de tests sur schema_utils et subgraphs).

Nice-to-have avant merge : #2 (double-invoke fallback), #3 (HITL no-trace), #9 (route dict non typé).

Le reste peut être traité en follow-up tickets.

---

Revue par SoluBot (SoluDevTech) — 2026-07-23 — basée sur la lecture du diff + HEAD 219bd42.