feat(buddy-assist) : Added HITL for chat#827
Open
kanakagrawal-crypto wants to merge 1 commit into
Open
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds Human-In-The-Loop (HITL) tool approval support to the ChatAgent voice system. Users can be prompted to approve or reject pending tool calls before execution. Approvals are persisted in Redis, resolved through the chat API, and executed by the agent in a follow-up turn. Tool approval requirements are configured per template. ChangesHuman-In-The-Loop Tool Approval Workflow
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/schemas/breeze_buddy/chat.py (1)
247-266:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPure HITL responses can't pass this request model.
contentis still mandatory withmin_length=1, so callers cannot send an approval/rejection-only payload even thoughhitlsays message content is ignored. As written, the "resolve now, run later" path insend_chat_message_handler()is unreachable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/schemas/breeze_buddy/chat.py` around lines 247 - 266, The SendChatMessageRequest model requires non-empty content even when hitl is provided, preventing pure HITL approval payloads; change content from a required str to Optional[str] (e.g., content: Optional[str] = Field(None, min_length=1, ...)) and add a validator/root_validator on SendChatMessageRequest that enforces content is present and non-empty when hitl is None but allows content to be omitted/empty when hitl is provided; update the Field description accordingly so send_chat_message_handler() can accept HITL-only requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/ai/voice/agents/breeze_buddy/chat/agent.py`:
- Around line 744-751: The call to store_pending_hitl(...) ignores its boolean
result; update the agent flow at the store_pending_hitl(...) site to check the
returned value and abort HITL gating if it is False: if store_pending_hitl(...)
returns False, log an error including session_id and confirmation_id, do NOT
emit the hitl_confirmation_required event or set the turn to HITL_PENDING
(instead return/raise or emit an error/failure response so the user isn't left
with an unresolved confirmation), and ensure any resources tied to
call.tool_call_id are cleaned up; this prevents continuing the HITL branch when
the Redis write failed.
- Around line 332-367: The reducer output for approved HITL tools (where
apply_state_reducers updates self.agent_state and hitl_tool_results is appended)
is not persisted to storage; after applying reducers and before or after
insert_chat_message you must call and await upsert_agent_session_state(...) to
persist the updated self.agent_state for this session so subsequent turns load
the change; update the code in the same block (around apply_state_reducers /
hitl_tool_results append) to call upsert_agent_session_state with the current
session identifier and the new self.agent_state (and any necessary metadata used
elsewhere) so the in-memory change is saved.
In `@app/ai/voice/agents/breeze_buddy/template/types.py`:
- Around line 1068-1074: default_timeout_seconds is documented as Optional[int]
(None = no auto-expiry) but runtime coerces None to 0; update
ChatAgent._get_hitl_timeout to return None when templates specify no timeout
instead of 0, and change store_pending_hitl (and its parameter and any
downstream storage/code) to accept and persist Optional[int] (allowing
null/None) rather than only int so the end-to-end behavior matches the Field on
the template; modify type hints/signatures for ChatAgent._get_hitl_timeout and
store_pending_hitl and ensure any callers handle None appropriately.
In `@app/api/routers/breeze_buddy/chat/handlers.py`:
- Around line 417-418: The per-session RedisLock created via
RedisLock(_lock_key(session_id), ttl_seconds=_SESSION_LOCK_TTL_SECONDS) is being
released in the outer finally before the HITL follow-up turn (handled by
_handle_hitl_response() and turn_stream consumed by StreamingResponse) actually
runs, allowing interleaving calls to agent.run_turn() that corrupt state; change
the lock release to mirror the lock_handed_off pattern used in
send_chat_message_handler(): do not release the lock in the outer finally if
_handle_hitl_response will hand it off — instead set a lock_handed_off flag when
handing off to turn_stream/_handle_hitl_response and only release the RedisLock
when lock_handed_off is false (and ensure agent.run_turn() after approval runs
while the lock is still held).
- Around line 427-432: The handler currently ignores the boolean result of
resolve_hitl which can cause it to log success and emit the hitl_resolved event
or start a follow-up turn even when resolution failed; update the handler to
capture the return value of resolve_hitl(session_id=session_id,
confirmation_id=hitl_info.confirmation_id, approved=hitl_info.approved), check
for a falsy result, and on failure do not emit hitl_resolved or proceed with the
follow-up turn—instead log an error (including session_id and confirmation_id)
and return/raise an appropriate error response so the failure is not
acknowledged. Ensure references to resolve_hitl, session_id,
hitl_info.confirmation_id, hitl_resolved, and the follow-up-turn path are used
to locate the changes.
- Around line 499-522: blocks_to_llm_context_messages returns
list[LLMContextMessage] but ChatAgent.run_turn is annotated to accept
list[dict[str, Any]], causing a type error; fix by updating ChatAgent.run_turn
signature to accept list[LLMContextMessage] (or a union/Protocol that includes
that type) so the types align, and update any related internal uses to the new
type, or alternatively perform an explicit cast at the call sites where
blocks_to_llm_context_messages is passed (the calls that invoke
ChatAgent.run_turn) to convert to list[dict[str, Any]]; ensure you adjust both
the first call and the second similar call mentioned and update any
imports/annotations for LLMContextMessage to keep static typing consistent.
In `@app/services/redis/hitl.py`:
- Around line 212-241: Check whether the pending confirmation exists before
writing the resolved record: if client.get(pending_key) returns no pending_raw
(i.e., pending_info is None), do not call client.setex on resolved_key and
instead return False (or an appropriate failure), so we don't create a
hitl:resolved:* for an expired/unknown confirmation; update the logic around
pending_raw/pending_info and the function that processes confirmations (the
block using pending_key, pending_raw, pending_info, resolved_key, client.setex,
and client.delete) to bail out early when pending_raw is missing and only
proceed to setex/delete when a valid pending_info exists.
---
Outside diff comments:
In `@app/schemas/breeze_buddy/chat.py`:
- Around line 247-266: The SendChatMessageRequest model requires non-empty
content even when hitl is provided, preventing pure HITL approval payloads;
change content from a required str to Optional[str] (e.g., content:
Optional[str] = Field(None, min_length=1, ...)) and add a
validator/root_validator on SendChatMessageRequest that enforces content is
present and non-empty when hitl is None but allows content to be omitted/empty
when hitl is provided; update the Field description accordingly so
send_chat_message_handler() can accept HITL-only requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: beb471a5-30bd-4743-9522-7fe202a4bb11
📒 Files selected for processing (6)
app/ai/voice/agents/breeze_buddy/chat/agent.pyapp/ai/voice/agents/breeze_buddy/template/types.pyapp/api/routers/breeze_buddy/chat/handlers.pyapp/schemas/breeze_buddy/chat.pyapp/services/redis/__init__.pyapp/services/redis/hitl.py
Comment on lines
+332
to
+367
| # Apply state reducers | ||
| reducer_rules = ( | ||
| self.template.configurations.state_reducers | ||
| if self.template.configurations | ||
| else [] | ||
| ) | ||
| self.agent_state = apply_state_reducers( | ||
| state_data=self.agent_state, | ||
| tool_name=tool_name, | ||
| tool_result=result_payload, | ||
| reducers=reducer_rules, | ||
| ) | ||
|
|
||
| # Store tool result so _run_turn_inner can add it to history for LLM context | ||
| if not hasattr(self, "hitl_tool_results"): | ||
| self.hitl_tool_results = [] | ||
| self.hitl_tool_results.append( | ||
| { | ||
| "tool_name": tool_name, | ||
| "result": result_payload, | ||
| "pending_id": pending_id, | ||
| "tool_call_id": resolved.tool_call_id, | ||
| } | ||
| ) | ||
|
|
||
| # Persist tool result to DB so subsequent turns load it from history | ||
| # (mirrors regular flow in run_turn lines 754-760) | ||
| if resolved.tool_call_id: | ||
| await insert_chat_message( | ||
| session_id=self.session_id, | ||
| role=ChatMessageRole.USER, | ||
| content=None, | ||
| content_blocks=tool_results_to_user_blocks( | ||
| [(resolved.tool_call_id, result_payload)] | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Persist reducer output for approved HITL tools.
This path updates self.agent_state after executing the approved tool, but it never calls upsert_agent_session_state(). If the follow-up turn produces only assistant text, the HITL tool's state changes exist only in memory for that request and are lost on the next turn.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/ai/voice/agents/breeze_buddy/chat/agent.py` around lines 332 - 367, The
reducer output for approved HITL tools (where apply_state_reducers updates
self.agent_state and hitl_tool_results is appended) is not persisted to storage;
after applying reducers and before or after insert_chat_message you must call
and await upsert_agent_session_state(...) to persist the updated
self.agent_state for this session so subsequent turns load the change; update
the code in the same block (around apply_state_reducers / hitl_tool_results
append) to call upsert_agent_session_state with the current session identifier
and the new self.agent_state (and any necessary metadata used elsewhere) so the
in-memory change is saved.
Comment on lines
+744
to
+751
| await store_pending_hitl( | ||
| session_id=self.session_id, | ||
| confirmation_id=confirmation_id, | ||
| tool_name=call.function_name, | ||
| arguments=injected_args, | ||
| timeout_seconds=timeout_seconds, | ||
| tool_call_id=call.tool_call_id, | ||
| ) |
There was a problem hiding this comment.
Abort HITL gating when the pending record isn't stored.
store_pending_hitl() returns bool, but the result is ignored. A Redis write failure currently still emits hitl_confirmation_required and ends the turn with HITL_PENDING, leaving the user with a confirmation UI that can never be resolved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/ai/voice/agents/breeze_buddy/chat/agent.py` around lines 744 - 751, The
call to store_pending_hitl(...) ignores its boolean result; update the agent
flow at the store_pending_hitl(...) site to check the returned value and abort
HITL gating if it is False: if store_pending_hitl(...) returns False, log an
error including session_id and confirmation_id, do NOT emit the
hitl_confirmation_required event or set the turn to HITL_PENDING (instead
return/raise or emit an error/failure response so the user isn't left with an
unresolved confirmation), and ensure any resources tied to call.tool_call_id are
cleaned up; this prevents continuing the HITL branch when the Redis write
failed.
Comment on lines
+1068
to
+1074
| default_timeout_seconds: Optional[int] = Field( | ||
| default=30, | ||
| ge=5, | ||
| le=300, | ||
| description="Default timeout for HITL approval in seconds. " | ||
| "None means no auto-expiry - pending confirmation stays until resolved.", | ||
| ) |
There was a problem hiding this comment.
None timeout is documented, but the runtime doesn't preserve it.
This field says None means "stay pending until resolved", but the new HITL flow immediately coerces that case away: ChatAgent._get_hitl_timeout() returns 0, and store_pending_hitl() only accepts an int. So templates can opt into a mode that the runtime cannot actually represent end-to-end.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/ai/voice/agents/breeze_buddy/template/types.py` around lines 1068 - 1074,
default_timeout_seconds is documented as Optional[int] (None = no auto-expiry)
but runtime coerces None to 0; update ChatAgent._get_hitl_timeout to return None
when templates specify no timeout instead of 0, and change store_pending_hitl
(and its parameter and any downstream storage/code) to accept and persist
Optional[int] (allowing null/None) rather than only int so the end-to-end
behavior matches the Field on the template; modify type hints/signatures for
ChatAgent._get_hitl_timeout and store_pending_hitl and ensure any callers handle
None appropriately.
Comment on lines
+417
to
+418
| lock = RedisLock(_lock_key(session_id), ttl_seconds=_SESSION_LOCK_TTL_SECONDS) | ||
| try: |
There was a problem hiding this comment.
The session lock is released before the HITL follow-up turn actually runs.
StreamingResponse starts consuming turn_stream() only after _handle_hitl_response() returns, but the outer finally releases the lock immediately on return. That leaves the post-approval agent.run_turn() outside the per-session lock, so another /message can interleave and duplicate tool execution or scramble chat/session-state writes. Mirror the lock_handed_off pattern used in send_chat_message_handler() here too.
Also applies to: 556-559, 590-591
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/routers/breeze_buddy/chat/handlers.py` around lines 417 - 418, The
per-session RedisLock created via RedisLock(_lock_key(session_id),
ttl_seconds=_SESSION_LOCK_TTL_SECONDS) is being released in the outer finally
before the HITL follow-up turn (handled by _handle_hitl_response() and
turn_stream consumed by StreamingResponse) actually runs, allowing interleaving
calls to agent.run_turn() that corrupt state; change the lock release to mirror
the lock_handed_off pattern used in send_chat_message_handler(): do not release
the lock in the outer finally if _handle_hitl_response will hand it off —
instead set a lock_handed_off flag when handing off to
turn_stream/_handle_hitl_response and only release the RedisLock when
lock_handed_off is false (and ensure agent.run_turn() after approval runs while
the lock is still held).
Comment on lines
+427
to
+432
| # Resolve the HITL in Redis | ||
| await resolve_hitl( | ||
| session_id=session_id, | ||
| confirmation_id=hitl_info.confirmation_id, | ||
| approved=hitl_info.approved, | ||
| ) |
There was a problem hiding this comment.
Don't acknowledge HITL resolution when resolve_hitl() fails.
This call returns bool, but the result is ignored. If Redis is unavailable or the storage layer rejects the confirmation, this handler still logs success and can emit hitl_resolved or start the follow-up turn against a confirmation that never actually resolved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/routers/breeze_buddy/chat/handlers.py` around lines 427 - 432, The
handler currently ignores the boolean result of resolve_hitl which can cause it
to log success and emit the hitl_resolved event or start a follow-up turn even
when resolution failed; update the handler to capture the return value of
resolve_hitl(session_id=session_id, confirmation_id=hitl_info.confirmation_id,
approved=hitl_info.approved), check for a falsy result, and on failure do not
emit hitl_resolved or proceed with the follow-up turn—instead log an error
(including session_id and confirmation_id) and return/raise an appropriate error
response so the failure is not acknowledged. Ensure references to resolve_hitl,
session_id, hitl_info.confirmation_id, hitl_resolved, and the follow-up-turn
path are used to locate the changes.
Comment on lines
+499
to
+522
| history = blocks_to_llm_context_messages( | ||
| [ | ||
| { | ||
| "role": row.role.value, | ||
| "content": row.content, | ||
| "content_blocks": row.content_blocks, | ||
| } | ||
| for row in history_rows | ||
| if row.role == ChatMessageRole.ASSISTANT | ||
| or ( | ||
| row.role == ChatMessageRole.USER | ||
| and ( | ||
| row.content is not None | ||
| or ( | ||
| row.content_blocks | ||
| and any( | ||
| b.get("type") == "tool_result" | ||
| for b in row.content_blocks | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ] | ||
| ) |
There was a problem hiding this comment.
This history conversion path already breaks the build.
blocks_to_llm_context_messages() returns list[LLMContextMessage], but ChatAgent.run_turn() is still typed as accepting list[dict[str, Any]]; the build is already failing on this call site. Align the ChatAgent.run_turn() history annotation with the actual helper return type, or cast once at the boundary so both callers and callee agree.
Based on pipeline failure: Argument list[LLMContextMessage] is not assignable to parameter history with type list[dict[str, Any]].
Also applies to: 536-540
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/routers/breeze_buddy/chat/handlers.py` around lines 499 - 522,
blocks_to_llm_context_messages returns list[LLMContextMessage] but
ChatAgent.run_turn is annotated to accept list[dict[str, Any]], causing a type
error; fix by updating ChatAgent.run_turn signature to accept
list[LLMContextMessage] (or a union/Protocol that includes that type) so the
types align, and update any related internal uses to the new type, or
alternatively perform an explicit cast at the call sites where
blocks_to_llm_context_messages is passed (the calls that invoke
ChatAgent.run_turn) to convert to list[dict[str, Any]]; ensure you adjust both
the first call and the second similar call mentioned and update any
imports/annotations for LLMContextMessage to keep static typing consistent.
Sources: Linters/SAST tools, Pipeline failures
Comment on lines
+212
to
+241
| # Fetch pending info BEFORE deleting - needed to execute tool after resolution | ||
| pending_raw = await client.get(pending_key) | ||
| pending_info = None | ||
| if pending_raw: | ||
| pending_info = json.loads(pending_raw) | ||
|
|
||
| # Store resolved confirmation for audit, including pending info | ||
| # so _execute_pending_hitl_tool can use it after the pending key is deleted | ||
| resolved_data = { | ||
| "confirmation_id": confirmation_id, | ||
| "session_id": session_id, | ||
| "approved": approved, | ||
| "resolved_at": datetime.now(timezone.utc).isoformat(), | ||
| # Include pending info so it's available after pending key is deleted | ||
| "tool_name": pending_info.get("tool_name") if pending_info else None, | ||
| "original_arguments": ( | ||
| pending_info.get("arguments") if pending_info else None | ||
| ), | ||
| "tool_call_id": pending_info.get("tool_call_id") if pending_info else None, | ||
| } | ||
|
|
||
| await client.setex( | ||
| resolved_key, | ||
| RESOLVED_TTL_SECONDS, | ||
| json.dumps(resolved_data), | ||
| ) | ||
|
|
||
| # Delete pending confirmation, but keep the session's pending ID | ||
| # until the user resumes the session turn and the approval is consumed. | ||
| await client.delete(pending_key) |
There was a problem hiding this comment.
Reject missing pending confirmations before writing resolved state.
If pending_key is already gone, this still creates a hitl:resolved:* record and returns True. That makes an expired or unknown confirmation_id look successfully resolved even though there is no pending tool left to consume.
Suggested fix
# Fetch pending info BEFORE deleting - needed to execute tool after resolution
pending_raw = await client.get(pending_key)
- pending_info = None
- if pending_raw:
- pending_info = json.loads(pending_raw)
+ if not pending_raw:
+ logger.warning(
+ f"[HITL] Cannot resolve missing confirmation {confirmation_id} "
+ f"in session {session_id}"
+ )
+ return False
+ pending_info = json.loads(pending_raw)🧰 Tools
🪛 ast-grep (0.43.0)
[info] 235-235: use jsonify instead of json.dumps for JSON output
Context: json.dumps(resolved_data)
Note: Security best practice.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/services/redis/hitl.py` around lines 212 - 241, Check whether the pending
confirmation exists before writing the resolved record: if
client.get(pending_key) returns no pending_raw (i.e., pending_info is None), do
not call client.setex on resolved_key and instead return False (or an
appropriate failure), so we don't create a hitl:resolved:* for an
expired/unknown confirmation; update the logic around pending_raw/pending_info
and the function that processes confirmations (the block using pending_key,
pending_raw, pending_info, resolved_key, client.setex, and client.delete) to
bail out early when pending_raw is missing and only proceed to setex/delete when
a valid pending_info exists.
kanakagrawal-crypto
force-pushed
the
hitl_chat
branch
from
0ba1d63 to
047c8c7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit