fix: await telemetry.shutdown in stop() and re-throw auth timeout error#1420
fix: await telemetry.shutdown in stop() and re-throw auth timeout error#1420Ricardo-M-L wants to merge 2 commits into
Conversation
In `_convert_responses_items_to_completion_messages`, the scroll direction mapping was completely inverted for all 4 directions: - scroll_x > 0 mapped to "left" instead of "right" - scroll_x < 0 mapped to "right" instead of "left" - scroll_y > 0 mapped to "up" instead of "down" - scroll_y < 0 mapped to "down" instead of "up" The reverse conversion in `_convert_completion_to_responses_items` was correct, making the forward and reverse mappings inconsistent — a round-trip would invert the scroll direction.
cloud.ts stop(): call telemetry.shutdown() to flush PostHog events — matches the pattern in AgentClient.disconnect. auth.ts loginViaBrowser: re-throw the timeout error instead of silently swallowing it, so callers receive a proper error instead of undefined.
|
@Ricardo-M-L is attempting to deploy a commit to the Cua Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThese changes comprise three separate bug fixes: correcting inverted scroll direction mappings in the Python agent loop, ensuring telemetry shutdown completion during cloud computer disconnection, and refactoring error handling in browser-based authentication from finally to catch block logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
libs/python/agent/agent/loops/anthropic.py (1)
484-498:⚠️ Potential issue | 🔴 CriticalThe forward conversion is inverted relative to the correct reverse conversion path and qmp.py convention.
According to qmp.py (line 280), positive
scroll_ymeans "scroll up". However, the forward conversion at lines 484–498 maps:
scroll_y > 0→direction = "down"scroll_y < 0→direction = "up"This inverts the direction. When this flows through the tool_calls reverse conversion (lines 1289–1298), which correctly expects
"up"→ positivescroll_y, the result is backwards:
- Input:
scroll_y > 0(scroll up) → mapped to"down"→ reversed to negativescroll_y(scroll down)The forward conversion should be:
scroll_y > 0→direction = "up"scroll_y < 0→direction = "down"Additionally, there is a pre-existing inconsistency: the content_item reverse conversion (lines 855–872) has the opposite mapping (
"down"→ positivescroll_y), which is also incorrect per qmp.py. Both reverse conversion paths should be aligned to match the qmp.py convention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/python/agent/agent/loops/anthropic.py` around lines 484 - 498, The forward conversion block that sets direction/amount (the code that assigns direction = "right"/"left"/"down"/"up" based on scroll_x/scroll_y and computes amount) is inverted for scroll_y: change it so scroll_y > 0 sets direction = "up" and scroll_y < 0 sets direction = "down" (amount stays absolute). Also update both reverse conversion paths that interpret direction strings—the "tool_calls" reverse conversion and the "content_item" reverse conversion—so they both follow qmp.py's convention: "up" maps to positive scroll_y and "down" maps to negative scroll_y. Ensure the direction string names ("up"/"down") are used consistently between the forward conversion (direction variable) and both reverse conversions.
🤖 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 `@libs/python/agent/agent/loops/anthropic.py`:
- Around line 484-498: The forward conversion block that sets direction/amount
(the code that assigns direction = "right"/"left"/"down"/"up" based on
scroll_x/scroll_y and computes amount) is inverted for scroll_y: change it so
scroll_y > 0 sets direction = "up" and scroll_y < 0 sets direction = "down"
(amount stays absolute). Also update both reverse conversion paths that
interpret direction strings—the "tool_calls" reverse conversion and the
"content_item" reverse conversion—so they both follow qmp.py's convention: "up"
maps to positive scroll_y and "down" maps to negative scroll_y. Ensure the
direction string names ("up"/"down") are used consistently between the forward
conversion (direction variable) and both reverse conversions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19c9adce-41eb-48ae-a0c9-0665be8f6f5a
📒 Files selected for processing (3)
libs/python/agent/agent/loops/anthropic.pylibs/typescript/computer/src/computer/providers/cloud.tslibs/typescript/cua-cli/src/auth.ts
1 participant
Summary
Bug 1 - cloud.ts
stop()missing telemetry shutdown:stop()disconnected the interface but never calledtelemetry.shutdown(). When CloudComputer disconnects, PostHog events are never flushed. Fix: addawait this.telemetry.shutdown(), matchingAgentClient.disconnect().Bug 2 - auth.ts timeout swallowed:
loginViaBrowserusedfinallyinstead ofcatch— timeout fires, error caught but not re-thrown. Callers gotundefinedinstead of an error. Fix: changefinallytocatch, re-throw after stopping server.Test plan
Summary by CodeRabbit
Bug Fixes
Improvements