fix(hitl): approve/reject continues conversation, persists trace, multi-tool-call decisions

[Kaiohz]

Problem

The HITL (human-in-the-loop) approve/reject flow was broken: clicking Approve or Confirm Reject in the UI did not continue the conversation. Two root causes:

1. HITL path never persisted trace events. SendMessageUseCase returned the runner Message directly without persisting trace events. Thread history is rebuilt exclusively from the trace_events table, so after approve/reject the refetched history was byte-identical: the AI message kept status=awaiting_hitl, the HITL panel stayed rendered, and the continuation produced by Command(resume=...) was invisible.

2. Multi-tool-call interrupts were broken. The runner approve_hitl/reject_hitl/edit_hitl always sent a single decision, but the langchain HumanInTheLoopMiddleware groups all interrupted tool calls of one AI message into a single interrupt and requires len(decisions) == len(action_requests) (positional). With 2+ interrupted tool calls the resume raised ValueError → 500.

A secondary issue: the default MemorySaver checkpointer lost the pending interrupt across restarts / registry invalidation / multi-worker deployments, turning the resume into a cryptic 500.

Changes

New contract

• HitlDecision entity (tool_call_id, action: approve|reject|edit, reason, edits).
• TraceEventType.HITL_DECISION — new event type persisted for each resume decision.
• AgentRunner.resume_hitl(thread_id, decisions, turn_id) -> (Message, list[TraceEvent]) replaces the 3 old methods. It:
  • reads the pending interrupt state (await aget_state),
  • reconstructs the positional tool_call_id → action_request mapping (matching langchain after_model ordering),
  • validates unknown / missing decisions (clear AgentError instead of cryptic 500),
  • builds Command(resume={decisions: [...]}),
  • streams the resume to collect the trace (HITL_DECISION events + intermediates + trailing AI_MESSAGE),
  • returns the final Message + the full trace.
• SendMessageUseCase HITL path now generates a turn_id, calls resume_hitl, and persists the trace via trace_repo.add_batch. Legacy single-decision shape (action + tool_call_id) is converted to a 1-element decisions list (backward compat).
• ChatRequest accepts a decisions list (mutually exclusive with message / legacy tool_call_id+action).

Checkpointer durability

• Default checkpoint_backend switched from "memory" to "postgres" (the interrupt must survive restarts for resume to work in durable / multi-worker deployments). The factory falls back to MemorySaver with a warning if Postgres is unreachable at build time.
• Adapter get_state calls converted to await aget_state (required for the async Postgres checkpointer).

Deprecated passthroughs

• The 3 old port methods (approve_hitl/reject_hitl/edit_hitl) are kept as concrete deprecated passthroughs delegating to resume_hitl, for backward compatibility with any external caller.

Docs

• README: documents the real endpoint (POST /api/v1/chat/{thread_id}) and the new decisions contract; fixes the non-existent /threads/{id}/hitl docs; updates the checkpoint_backend default to postgres.

Tests

• Backend suite: 691 passed (TDD red→green; new TestResumeHitl covers single/multi/unknown/missing/edit/no-pending; SendMessageUseCase resume + trace persistence; ChatRequest validation; factory checkpointer default + fallback; routes HITL with new resume_hitl mock).
• SonarQube: 0 new issues in changed files.
• Trivy: 0 new vulnerabilities (uv.lock clean).
• ruff: clean.

Acceptance criteria

• Approve continues the conversation (status flips to completed, content non-empty, trace persisted).
• Reject with reason continues the conversation; the reason is persisted in the HITL_DECISION event.
• Multi-tool-call interrupts: one decision per interrupted tool call, positional, 400-style error if a decision is missing/unknown.
• Legacy single {tool_call_id, action} payload still works (converted to a 1-element list).
• No-pending-interrupt resume raises a clear AgentError (no more cryptic 500 on double-click / stale panel).
• Checkpointer survives restarts (postgres default + graceful fallback).

Linked PRs

• Frontend: SoluDevTech/composable-ui#… (UI side of the same fix)
• E2E QA tests: SoluDevTech/soludev-compose-apps#… (new regression gate)

Notes

• QA e2e tests (soludev-compose-apps/bricks/qa/test_hitl_flow.py) are written but must be run after rebuilding the containers with this code. They are expected to fail against the old stack (the regression signal).

[Kaiohz]

Code review — fix(hitl): approve/reject continues conversation, persists trace, multi-tool-call decisions

Solid refactor. The HITL surface moves from a single-action helper per call to a unified resume_hitl(thread_id, decisions, turn_id) -> (Message, list[TraceEvent]) contract that finally makes multi-tool-call decisions first-class, persists the HITL decision into the trace (was dropped before), and reads state via the async aget_state (required by the new default Postgres checkpointer). Legacy approve_hitl/reject_hitl/edit_hitl are kept as deprecated passthroughs that build a one-element decisions list — no breaking change for the API layer.

What I like

• Domain purity. HitlDecision is a Pydantic value object with Literal["approve","reject","edit"] — typed at the boundary instead of leaking stringly-typed enums into the use case.
• Port preserved. Everything still goes through AgentRunner; the factory and adapter changed, the boundary didn't.
• Backward compatibility done right. Legacy callers keep working (use case converts action+tool_call_id+reason+edits into a one-element list) without duplicating the resume logic.
• Default → Postgres with graceful fallback. _create_postgres_checkpointer failure now logs CHECKPOINTER_POSTGRES_UNAVAILABLE and falls back to MemorySaver. Default flips from memory to postgres in BackendConfig, aligned with prod on K3s.
• Edge cases covered. no pending interrupt, unknown tool_call_id, missing decision, resume then new interrupt (AWAITING_HITL on the new one, COMPLETED otherwise), multi-decision positional pairing, edit tool-name resolution from the last AI message tool_calls. Tests assert the actual Command(resume={"decisions": [...]}) payload — that's exactly the contract surface.
• Status semantics. _resolve_status now distinguishes "the prior interruption was just decided" (don't re-report AWAITING_HITL) from "a NEW tool_call appeared after the resume" (do). This was the original bug.

Suggestions

1. _pair_action_requests indexes by tool name — fragile when two interrupted tool_calls share a name. The langchain middleware filters by interrupt_on config and preserves positional order, but the pairing here matches by tc.get("name") == action_requests[i]["name"]. If an agent emits two search(...) tool calls in one AI message and both match interrupt_on, the second tc.get("id") is paired to the second action_request by accident — that's true today because names align, but it would silently swap if the order in the interrupt payload ever differs from the last AI message order. I'd rather pair by position (zip [t for t in tool_calls if t["name"] in {ar["name"] for ar in action_requests}] after an explicit filter), or assert the count.
2. except TypeError in resume_hitl is too broad. Catching TypeError around the async for event in self._collect_trace(...) call to fall back to _build_resume_trace_fallback will also mask real TypeErrors raised inside the generator (bad event.type compare, malformed metadata, etc.) and silently switch to a degraded code path that doesn't emit intermediate trace events. Prefer an explicit capability check: inspect.isasyncgen(self._graph.astream) on a probe, or wrap self._graph.astream at construction time with a to_async_iter helper, so the fallback only triggers on "astream returned a non-async-iterable", not on random generator errors.
3. # type: ignore[arg-type] on the legacy path in send_message.py. When decisions is None, the code builds HitlDecision(tool_call_id=tool_call_id, action=action, reason=reason, edits=edits) with action: str | None being passed to a Literal field. The validator in ChatRequest guarantees action is not None at the HTTP boundary, but the use case is now coupled to that guarantee. Either tighten the type annotation on the use case parameter (action: Literal[...]) or assert action in {...} before the cast so the contract is local.
4. No test for asyncio.TimeoutError on the resume path. invoke has test_invoke_timeout_raises_agent_error. resume_hitl calls asyncio.wait_for(self._graph.ainvoke(...), timeout=...) in the final fallback but doesn't have a symmetric test — if a future refactor drops the timeout it won't be caught.
5. min_length=1 on decisions. The model validator already raises on empty decisions, but using Field(default=None, min_length=1) (with decisions: list[HitlDecision] | None = None) would let pydantic surface a 422 from the OpenAPI schema directly instead of a hand-written message — same end-user UX, less code to maintain.
6. agent_config.py default flip. BackendConfig.checkpoint_backend default moves from "memory" to "postgres". That's correct for prod, but worth a CHANGELOG / migration note: any local dev setup without a reachable Postgres will start hitting the new CHECKPOINTER_POSTGRES_UNAVAILABLE warning. Consider a doc snippet or a smoke-test mention in the README.

Score

8/10. The architecture, the tests, and the backward compat are all clean. The two points I'd want a second pass on are the except TypeError swallow (item 2) and the position-by-name pairing (item 1) — both are subtle enough that a regression could pass CI today and bite in prod next quarter.

Non-blocking

• The agent_crud rename (test naming only) is a no-op reformat — fine.
• Adding HITL_DECISION to TraceEventType is the right move; worth checking downstream consumers (UI trace viewer, log formatters) still tolerate the new value.