-
Notifications
You must be signed in to change notification settings - Fork 40
Fix Remote Runtime API Authentication #1090
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@OpenHands review diff from main I got these error when running Can you come up with a few hypothesis what might cause this error? |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
neubig
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a regression test?
Summary: Analysis of 422 Error in Remote Agent ServerI've analyzed the error you encountered when running the example with The ErrorThe remote agent server returned 422 Unprocessable Entity when trying to create a conversation, rejecting these LLM fields as "Extra inputs are not permitted":
These fields appear in both the main agent's LLM and the condenser's LLM. Root Cause Hypotheses (Ordered by Likelihood)1. Version Mismatch Between SDK and Remote Agent Server ⭐ (Most Likely)
2. Serialization Including Default Values
3. Field Exclusion Not Configured
4. Pydantic Model Config Inconsistency
5. PR Changes Side Effects (Least Likely)
Recommended Next StepsTo diagnose:
To fix (short-term):
To fix (long-term):
The error is not directly related to your authentication PR changes, but rather a schema compatibility issue between the SDK and the remote agent-server version. |
🔄 Running Examples with
|
| Example | Status | Duration | Cost |
|---|---|---|---|
| 02_remote_agent_server/04_vscode_with_docker_sandboxed_server.py | ❌ FAIL (exit: 1) | 116s | $0.00 |
❌ Some tests failed
Total: 1 | Passed: 0 | Failed: 1 | Total Cost: $0.00
🔄 Running Examples with
|
| Example | Status | Duration | Cost |
|---|---|---|---|
| 02_remote_agent_server/04_convo_with_api_sandboxed_server.py | ❌ FAIL (exit: 1) | 6s | $0.00 |
❌ Some tests failed
Total: 1 | Passed: 0 | Failed: 1 | Total Cost: $0.00
🔄 Running Examples with
|
| Example | Status | Duration | Cost |
|---|---|---|---|
| 02_remote_agent_server/04_convo_with_api_sandboxed_server.py | ❌ FAIL (exit: 1) | 315s | $0.00 |
❌ Some tests failed
Total: 1 | Passed: 0 | Failed: 1 | Total Cost: $0.00
🔄 Running Examples with
|
| Example | Status | Duration | Cost |
|---|---|---|---|
| 02_remote_agent_server/04_convo_with_api_sandboxed_server.py | ❌ FAIL (exit: 1) | 314s | $0.00 |
❌ Some tests failed
Total: 1 | Passed: 0 | Failed: 1 | Total Cost: $0.00
🔄 Running Examples with
|
| Example | Status | Duration | Cost |
|---|---|---|---|
| 02_remote_agent_server/04_convo_with_api_sandboxed_server.py | ❌ FAIL (exit: 1) | 106s | $0.00 |
❌ Some tests failed
Total: 1 | Passed: 0 | Failed: 1 | Total Cost: $0.00
🔄 Running Examples with
|
| Example | Status | Duration | Cost |
|---|---|---|---|
| 02_remote_agent_server/04_convo_with_api_sandboxed_server.py | ✅ PASS | 96s | $0.04 |
✅ All tests passed!
Total: 1 | Passed: 1 | Failed: 0 | Total Cost: $0.04
This reverts commit 3701e0f.
|
@neubig added and it works! |
| self.host = self._runtime_url.rstrip("/") | ||
| self.api_key = self._session_api_key | ||
| self._client = None # Reset HTTP client with new host and API key | ||
| _ = self.client # Initialize client by accessing the property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a bit weird, maybe error-prone, though I see where it's coming from...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OpenHands review the codebase and understand how we can do this better. Then create an issue about it as we can resolve it in a future PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on it! xingyaoww can track my progress at all-hands.dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
I've reviewed the codebase and created a GitHub issue documenting the architectural concern identified in PR #1090.
What I Did
-
Analyzed the codebase to understand the authentication pattern in
APIRemoteWorkspace(lines 114-116 inopenhands-workspace/openhands/workspace/remote_api/workspace.py) -
Identified the issue: The current pattern for updating HTTP client credentials is fragile and error-prone:
self.api_key = self._session_api_key self._client = None # Reset HTTP client with new host and API key _ = self.client # Initialize client by accessing the property
-
Created GitHub Issue Refactor: Improve HTTP client credential update pattern in Remote Workspace #1101: Refactor: Improve HTTP client credential update pattern in Remote Workspace
Key Problems Documented
The issue describes why this pattern is concerning:
- Non-obvious side effects: Accessing
.clienttriggers lazy initialization - Order dependency: Operations must happen in exact sequence
- Fragility: Changes to parent class could break this silently
- Code smell: Using
_ = self.clientjust for side effects - Mixed concerns: Two authentication mechanisms (Runtime API vs Agent Server) are intertwined
Proposed Solutions
I documented four potential solutions in the issue, with Option 2 (dynamic client recreation) recommended as it:
- Requires minimal code changes
- Makes credential updates transparent
- Is backwards compatible
- Handles both credential AND host changes automatically
Option 3 (separate clients for Runtime API vs Agent Server) would be ideal for a larger refactoring to make separation of concerns explicit.
No Code Changes Made
As requested, I did not modify any code - this analysis is captured in the GitHub issue for future resolution in a separate PR.
openhands-workspace/openhands/workspace/remote_api/workspace.py
Outdated
Show resolved
Hide resolved
Only log header keys (not values) to avoid exposing API keys in logs. This applies to both request kwargs and error response headers. Co-authored-by: openhands <[email protected]>
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
| # If GITHUB_SHA is set (e.g. running in CI of a PR), use that to ensure consistency | ||
| # Otherwise, get the latest commit SHA from main branch (images are built on main) | ||
| server_image_sha = os.getenv("GITHUB_SHA") or get_latest_commit_sha( | ||
| "OpenHands/software-agent-sdk", "main" | ||
| ) | ||
| server_image = f"ghcr.io/openhands/agent-server:{server_image_sha[:7]}-python-amd64" | ||
| logger.info(f"Using server image: {server_image}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is necessarily blocking, but this is a bit complex for an example. Maybe we could move this off into some other utility function in another file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OpenHands please create a new github issue tracking this - we can simplify this after we are able to customize image pull policy on runtime API (after https://github.com/OpenHands/runtime-api/pull/356)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on it! xingyaoww can track my progress at all-hands.dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
I've successfully created a GitHub issue to track the simplification work as requested.
Issue Created: #1104
Title: "Simplify image resolution logic in remote agent server example after runtime-api image pull policy support"
What the issue tracks:
- The complex
get_latest_commit_sha()function and related image resolution logic inexamples/02_remote_agent_server/04_convo_with_api_sandboxed_server.py(lines 49-79) - Plans to simplify this code once runtime-api PR Unify tool initialization — Step 1: global registry + lazy materialization #356 is merged, which will add support for customizable image pull policies
- The simplification will remove the GitHub API querying logic and use simpler, stable image tags instead
- This will make the example more focused on demonstrating core functionality rather than workaround logic
The issue has been labeled with enhancement and technical-debt tags, includes references to the runtime-api PR #356 and the current PR #1090, and contains a task checklist for tracking the work.
Fix remote runtime API authentication and add regression test as part of the "test-examples" workflow
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:55be574-pythonRun
All tags pushed for this build
About Multi-Architecture Support
55be574-python) is a multi-arch manifest supporting both amd64 and arm6455be574-python-amd64) are also available if needed