fix(hitl): approve/reject continues conversation, multi-tool-call panel, last-message gating

[Kaiohz]

Problem

The HITL approve/reject flow was broken in the UI: clicking Approve or Confirm Reject did not continue the conversation. Root causes on the frontend:

1. Wrong React Query key. useSendMessage invalidated [\"messages\", threadId] but the message list is driven by useThreadHistory whose key is [\"history\", threadId]. After a successful approve/reject nothing was refetched: the UI stayed frozen on the awaiting_hitl message.
2. Only toolCalls[0] handled. Multi-tool-call interrupts were not supported (the backend now requires one decision per interrupted tool call).
3. Panel rendered on every awaiting_hitl message, not only the last one, so a stale panel reappeared on older messages after continuation.
4. Mutation errors silently swallowed (no sendMessage.error display) — a 422/500 was indistinguishable from "approve did nothing".

Changes

• useSendMessage: invalidate [\"history\", threadId] (the correct key). ✅ root cause BRIC-4: Init composable-ui React frontend #1.
• ChatRequest: add HitlDecisionInput + decisions field (legacy tool_call_id+action kept for backward compat).
• HITLReviewPanel: render one decision row per tool call (Approve/Reject per call + optional reject reason), a single Submit button sends one mutation with { decisions: [...] }. Display sendMessage.error when the mutation fails. Extracted ToolDecisionRow subcomponent to keep function nesting shallow (SonarQube S2004). ✅ root causes feat(config): migrate to runtime config.json loading #2 + feat: add file upload UI in RAG page (BRIC-12) #4.
• ChatMessage: add isLast prop; render the HITL panel only on the last message. Render discreet HITL_DECISION badges in the timeline (✓ Approved / ✗ Rejected: reason / ✎ Edited). Update memo comparator. ✅ root cause BRIC-11: Add RAG File Browser section for MinIO folder/file exploration #3.
• MessageList: pass isLast={idx === entries.length - 1} to each entry.
• traceEvent: add HITL_DECISION to the TraceEventType enum (flows into chatApi VALID_EVENT_TYPES automatically).

Tests

• Frontend suite: 801 passed (TDD red→green; useSendMessage invalidation key + decisions payload; HITLReviewPanel multi-tool-call rows + error display; ChatMessage isLast gating + HITL_DECISION badges; MessageList last-message gating; traceEvent enum).
• SonarQube: 0 new issues in changed files (3 pre-existing S4325 in untouched code).
• Trivy: 0 new vulnerabilities (17 HIGH/22 MEDIUM pre-existing in deps — none introduced; no dependency files changed).
• eslint + tsc: clean on changed files.

Acceptance criteria

• Clicking Approve refetches history and the conversation continues.
• Reject with reason refetches history and continues; the reason is sent.
• Multi-tool-call panel: one decision row per tool call, one Submit.
• HITL panel only on the last message (no stale panel on older messages).
• Mutation errors are surfaced in the panel.
• HITL_DECISION badge renders in the timeline.

Linked PRs

• Backend: fix(hitl): approve/reject continues conversation, persists trace, multi-tool-call decisions composable-agents#40
• E2E QA tests: SoluDevTech/soludev-compose-apps#…

Notes

• QA e2e tests (soludev-compose-apps/bricks/e2e/specs/hitl.spec.ts) are written but must be run after rebuilding the containers with this code.

[Kaiohz]

PR Review — fix(hitl): approve/reject continues conversation, multi-tool-call panel, last-message gating

Summary

This PR tackles three real bugs at once:

1. Stale HITL panel on an older awaiting_hitl message once the conversation continued (isLast gate).
2. Stale query key in useSendMessage (["messages"] → ["history"]) — the list actually reads from useThreadHistory, so the old invalidate was a silent no-op.
3. Single-tool-call panel — bumped to N tool calls with a single batched decisions: [...] payload, plus error visibility (was previously swallowed).

Plus a HITL_DECISION event type to surface approve/reject badges in the message timeline.

Architecture respected (domain HitlDecisionInput in domain/, UI in application/), test contracts are explicit and self-documenting, and the diff stays inside application/ + domain/ + tests. README/CHANGELOG left untouched (intentional, looks like the project keeps those elsewhere).

✅ Strengths

• Test contracts read like specs. The block comments above each test describe what must hold and why (e.g. "no stale panel on older messages"), not the implementation. Exactly the style ai-driven enforces.
• Solid component split. ToolDecisionRow extracted, props are explicit, HITLReviewPanel reads as orchestration only.
• Defensive edge cases covered: empty toolCalls (renders "Unknown tool"), non-threadId/non-awaiting_hitl combinations on ChatMessage, missing legacy key no longer invalidated.
• Query key fix is well-tested — both the positive (['history', threadId] is invalidated) and the negative (['messages', threadId] is not) assertions are explicit.
• HITL_DECISION enum test plus the byType lookup test ensure the chatApi will accept this new event.

⚠️ Suggestions (non-blocking)

1. Silent default to "approve" is a UX trap

buildDecisions() falls back to "approve" when no choice has been made:

const action = decisions[tc.id] ?? "approve";

A user who opens Review Data, doesn't touch anything, then clicks Submit, will silently auto-approve every tool call — including destructive ones (delete, write, …).

Two options:

• Make it explicit: add a per-row "Pending" choice / a triple-state (Approve / Reject / Pending) and disable Submit until each row has a decision. This is the safer default for a HITL flow.
• Or, keep the default but rename Submit → Approve all & submit and show a confirmation toast/modal when no per-row choice was made.

A test for this behavior (e.g. "Submit without any per-row decision auto-approves all") would also document the contract either way.

2. Error state has no recovery affordance

The error block now renders sendMessage.error.message — good, that's the bug fix. But there's no Retry button and no way to reset isError from the mutation. After a failure, the user can edit reasons and resubmit (the submit button stays enabled when isPending is false, even if isError is true), so the only fix path is type new reason + click Submit again. Worth confirming this is the intended UX, or adding a small Retry / Dismiss action.

3. Minor: no test for "Submit while pending is disabled"

There's a disabled={sendMessage.isPending} on Submit but no test asserting it. Tiny, but it's exactly the kind of contract a future refactor will silently break.

4. Minor: badges should use a small mapper

let label = "✎ Edited";
let className = "border-accent text-accent";
if (action === "approve") { … }
else if (action === "reject") { … }

Three branches inline inside JSX. A mapDecisionActionToBadge(action, content) near the component would (a) make the contract testable in isolation and (b) let you handle HITLAction = "edit" explicitly with its own label/icon (currently the edit case shares the generic "✎ Edited" path with any unknown action).

5. No test for the edit action badge

The HITLAction enum includes "edit", the UI renders "✎ Edited" for it, but no test asserts it. Same for the missing action (the default generic branch).

6. Trailing newlines

Three test files end with \ No newline at end of file:

• tests/unit/components/chat/HITLReviewPanel.test.tsx
• tests/unit/domain/entities/chat/traceEvent.test.ts
• tests/unit/hooks/chat/useSendMessage.test.tsx

If your lint config enforces final newlines (POSIX-friendly), this'll trip a warning. Trivial printf '\n' fix.

7. decisions state lives in three places

decisions, reasons, rejectingId are three useStates holding related values. A single useReducer or a per-row child component with its own state would collapse handleApprove / handleReject / handleReasonChange / buildDecisions into one place — and make per-row state pure (a key key={tc.id} reset between tool calls would happen for free). Not blocking; just cleaner.

🐛 Real bug? — none found, but one sanity check

• useSendMessage.ts: invalidation change is correct (matches useThreadHistory's key).
• MessageList.tsx: isLast={idx === entries.length - 1} is computed from entries, which from the test fixture looks like completed entries only (the pending user message and streaming AI message are managed elsewhere). Worth a one-line comment so a future reader doesn't worry about the streaming message case.

🧪 Test coverage score

Strong. Contracts are explicit, positive + negative cases for the key fix. Gaps to close:

• HITL_DECISION badge for the edit action and the default branch.
• Submit-while-pending disabled state.
• Silent-default-to-approve behavior (either lock it behind tests + docs, or fix it).
• Retry path on error.

📊 Score: 8 / 10

Architecture / contracts / typing: 9/10. Domain types are clean, components are correctly split, tests read like specs.

Production readiness: 7/10. The silent-auto-approve default is the main thing holding this back from a 9. Once that's either tested-and-documented or replaced with explicit per-row selection, this is a clean ship.

Suggested follow-up issue: flip the silent default to an explicit Pending state + disable Submit until each row is decided, and add a Retry affordance on the error block. Both are isolated to HITLReviewPanel.tsx and don't touch the API contract.

---

Posted by SoluBot on behalf of Yohan — auto-review of CI-green PR #25.