Pass automation owner IDs to SDK conversations#169
Conversation
|
🚀 Deploy Preview PR Created/Updated A deploy preview has been created/updated for this PR. Deploy PR: https://github.com/OpenHands/deploy/pull/4547 Once the deploy PR's CI passes, the automation service will be deployed to the feature environment. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review
Taste Rating
🟡 Acceptable — Works but could be cleaner
Analysis
[CRITICAL ISSUES]
None — the implementation is sound and the backward-compatibility check is prudent.
[IMPROVEMENT OPPORTUNITIES]
-
[openhands/automation/presets/plugin/sdk_main.py, Lines 138-143] Defensive Pattern: The
_conversation_supports_user_id()helper usesinspect.signature(Conversation.__new__)at runtime. While this works, it's fragile — it depends onConversation.__new__not changing signature for unrelated reasons. A cleaner alternative might be to check the SDK version or feature flag, but given this is a compatibility shim for the transition period, this approach is acceptable. -
[tests/test_preset_router.py, Lines 840-841] Test Fragility: The assertion checks for a specific JSON formatting. If the template later uses different quoting, this test may break. Consider checking for structural content instead.
[STYLE NOTES]
None — code is clean and follows existing patterns.
[TESTING GAPS]
- The test additions in
test_disable_automation.pycorrectly verify the new env vars are passed through. Consider adding a test for the_conversation_supports_user_id()fallback path to ensure backward compatibility is exercised.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
The change is additive and defensive. Env vars are passed through to the execution context, and the SDK integration has a graceful fallback for older versions. No breaking changes to existing functionality.
VERDICT:
✅ Worth merging: Core logic is sound, minor suggestions provided
KEY INSIGHT:
The backward-compatibility pattern (_conversation_supports_user_id()) is appropriate for the transition period but should be revisited once all deployed SDKs support the user_id parameter.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation.
|
👀 OpenHands is reviewing this pull request. Conversation: https://nestable-nonremittably-sha.ngrok-free.dev/conversations/f252821d-6fe5-4fbb-8770-14cf5a5c6295 This comment was generated by an AI agent (OpenHands) on behalf of the user. |
Summary
AUTOMATION_USER_IDandAUTOMATION_ORG_IDinto automation run environmentsAUTOMATION_USER_IDtoConversation(...)when the installed SDK supports theuser_idparameterConversation.__new__support before passinguser_idDependency
RemoteConversation(user_id=...)into the agent-server create payload and adds trace attributes for conversation tags.Testing
python3 -m py_compile openhands/automation/dispatcher.py openhands/automation/presets/prompt/sdk_main.py openhands/automation/presets/plugin/sdk_main.py tests/test_disable_automation.pygit diff --checkIssue
Closes #173
This PR description update was created by an AI agent (OpenHands) on behalf of Graham Neubig.