fix(runner): reassemble AG-UI streaming deltas in acp_get_session_status#1485
Open
quay-devel wants to merge 4 commits intoambient-code:mainfrom
Open
fix(runner): reassemble AG-UI streaming deltas in acp_get_session_status#1485quay-devel wants to merge 4 commits intoambient-code:mainfrom
quay-devel wants to merge 4 commits intoambient-code:mainfrom
Conversation
acp_get_session_status always returned empty messages because the message extraction code expected a "text" field on TEXT_MESSAGE_CONTENT events, but AG-UI uses streaming deltas where the field is "delta" and the role is on the TEXT_MESSAGE_START event. This also adds a fallback to MESSAGES_SNAPSHOT events for completed sessions that only have snapshot-style event storage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Contributor
📝 WalkthroughWalkthroughModified Changes
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/runners/ambient-runner/ambient_runner/bridges/claude/backend_tools.py (1)
400-459:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t swallow parser/runtime errors as “export failure.”
Lines 400-459 currently catch all exceptions from both fetch and parse, then return an empty-success response. A malformed event/delta can therefore hide a real bug and report
totalMessages: 0instead of surfacing the failure path.Proposed fix
- # Try to get recent events for message content - try: - events = api_client.get_session_events( - session_name=session_name, - ) + # Try to get recent events for message content + try: + events = api_client.get_session_events(session_name=session_name) + except Exception as events_err: + logger.warning(f"Could not fetch events for {session_name}: {events_err}") + result["recentMessages"] = [] + result["totalMessages"] = 0 + return _tool_response(result) + + try: # Two event patterns exist depending on session state: # # 1. Streaming deltas (active sessions): @@ - elif etype == "TEXT_MESSAGE_CONTENT" and mid: - if "delta" in event: - msg_deltas.setdefault(mid, []).append(event["delta"]) + elif etype == "TEXT_MESSAGE_CONTENT" and mid: + delta = event.get("delta") + if isinstance(delta, str): + msg_deltas.setdefault(mid, []).append(delta) @@ # Return only the last N messages result["recentMessages"] = messages[-max_messages:] result["totalMessages"] = len(messages) - except Exception as events_err: - logger.warning(f"Could not fetch events for {session_name}: {events_err}") - result["recentMessages"] = [] - result["totalMessages"] = 0 + except Exception as parse_err: + logger.error(f"Failed to parse events for {session_name}: {parse_err}", exc_info=True) + return _tool_error(parse_err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/bridges/claude/backend_tools.py` around lines 400 - 459, The current broad except around the entire events processing (catching Exception as events_err) hides parsing/runtime errors; restrict error handling to only the fetch step by wrapping api_client.get_session_events(session_name=...) in its own try/except and handling fetch failures there (log warning and set result["recentMessages"]=[] and result["totalMessages"]=0), but do not swallow exceptions raised while iterating/processing events (the loop that uses msg_roles, msg_deltas, MESSAGES_SNAPSHOT, etc.); allow those parsing/runtime exceptions to propagate (or re-raise after logging) so real bugs surface. Use the existing symbols api_client.get_session_events, session_name, events, msg_roles, msg_deltas, last_snapshot, and the current except block (events_err) to locate and refactor the code accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@components/runners/ambient-runner/ambient_runner/bridges/claude/backend_tools.py`:
- Around line 400-459: The current broad except around the entire events
processing (catching Exception as events_err) hides parsing/runtime errors;
restrict error handling to only the fetch step by wrapping
api_client.get_session_events(session_name=...) in its own try/except and
handling fetch failures there (log warning and set result["recentMessages"]=[]
and result["totalMessages"]=0), but do not swallow exceptions raised while
iterating/processing events (the loop that uses msg_roles, msg_deltas,
MESSAGES_SNAPSHOT, etc.); allow those parsing/runtime exceptions to propagate
(or re-raise after logging) so real bugs surface. Use the existing symbols
api_client.get_session_events, session_name, events, msg_roles, msg_deltas,
last_snapshot, and the current except block (events_err) to locate and refactor
the code accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: deb7cfa6-29e7-46da-a46e-de5a3b7a485a
📒 Files selected for processing (2)
components/runners/ambient-runner/ambient_runner/bridges/claude/backend_tools.pycomponents/runners/ambient-runner/tests/test_backend_tools.py
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
acp_get_session_statusreturning emptyrecentMessagesfor all sessions by correctly reassembling AG-UI streaming delta events (deltafield, nottext) and grouping bymessageIdwith role fromTEXT_MESSAGE_STARTMESSAGES_SNAPSHOTfallback for completed/stopped sessions that use snapshot-style event storage instead of streaming deltasdebugtowarningso export failures are observableFixes #1484
Root Cause
The message extraction code checked
event.get("text")onTEXT_MESSAGE_CONTENTevents, but AG-UI streaming protocol usesevent["delta"]for text chunks. Therolefield is also on theTEXT_MESSAGE_STARTevent, not on content events. Result: every event was silently skipped, yielding zero messages.Changes
backend_tools.py: Replace single-event extraction with streaming delta reassembly:rolefromTEXT_MESSAGE_STARTbymessageIddeltachunks fromTEXT_MESSAGE_CONTENTbymessageIdTEXT_MESSAGE_ENDMESSAGES_SNAPSHOT(last one) for completed sessions with no streaming eventstest_backend_tools.py: 7 new tests covering:max_messageslimiting (returns last N)MESSAGES_SNAPSHOTfallback for stopped sessionsTest Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests