can someone help confirm <@U0BEYEVUANS>’s issue wi...
# questions
m
can someone help confirm @mammoth-translator-75263’s issue with VS code?
m
Thanks for checking this one. Is there anything else i need to do for the PR to get merged ?
m
@quick-window-91705 @elegant-branch-54423 ☝️
e
Let's post this in #C06R25BT5B2 to see if we can get some eyes on this
r
This is a test of my Analysis Engine. and according to it. you are correct # Investigation: INV-058 ***Investigation ID***: INV-058 ***Date***: 2026-07-27T231000Z ***Status***: ACTIVE ***Engine***: KDE-ENGINE-002 (v0.1.0) ***Seed***: SEED-001 (v1.0.0) ***Created***: 2026-07-27T231500Z ***Modified***: 2026-07-27T231500Z --- ## Research Question What is the root cause of PR #4181 in the OpenHands/software-agent-sdk repository, and what problems or bad patterns exist in the codebase? ## Scope ***Included***: - Analysis of PR #4181 code changes - Root cause identification - Problem and pattern detection - Code quality assessment ***Excluded***: - Direct testing of the fix - Changes to external repositories ## Background PR #4181 is a bug fix for the
/api/vscode/url
endpoint in the openhands-agent-server. The issue was reported in OpenHands/OpenHands#15434 where the embedded VS Code button was unreachable in self-hosted mode because the advertised URL used a hardcoded port 8001 instead of the configured port. ## Related - ***Linked Issue***: OpenHands/OpenHands#15434 - ***Source PR***: OpenHands/software-agent-sdk#4181 - ***Author***: harish-chandramowli (Claude Code AI agent) - ***Reviewer***: enyst (APPROVED) - ***Merged***: 2026-07-23T211146Z --- ## Evidence ### Code Before (vscode_router.py)
Copy code
python
async def get_vscode_url(
    base_url: str = "<http://localhost:8001|localhost>",  # ← Hardcoded default!
    workspace_dir: str = "workspace"
) -> VSCodeUrlResponse:
### Code After (vscode_router.py)
Copy code
python
async def get_vscode_url(
    base_url: str | None = None,  # ← Now None by default
    workspace_dir: str = "workspace"
) -> VSCodeUrlResponse:
### Dead Code Path (vscode_service.py)
Copy code
python
def get_vscode_url(self, base_url: str | None = None, ...):
    if base_url is None:
        base_url = f"<http://localhost|localhost>:{self.port}"  # ← Never reached before fix
--- ## Problems Identified ### 1. Hardcoded Default Value Decoupled from Configuration [Evidence] The default
"<http://localhost:8001|localhost>"
was completely unrelated to
config.vscode_port
. [Inference] This is a code smell: route parameter defaults should not duplicate logic that exists in service layers. ### 2. Breaking API Change | Scenario | Before (Bug) | After (Fix) | |----------|--------------|-------------| |
GET /api/vscode/url
| Returns port 8001 | Returns configured port | |
GET /api/vscode/url?base_url=...
| Unchanged | Unchanged | [Evidence] Reviewer acknowledged: "REST API compatibility failure is expected" ### 3. Test Coverage Gap [Evidence] Original test
test_get_vscode_url_success
always passed
base_url="<http://localhost|localhost>"
explicitly, never testing the no-argument case. [Inference] This is why the bug was missed during development. ### 4. OpenAPI Schema Impact [Evidence] Default value removed → OpenAPI no longer documents a default. [Inference] May affect typed API clients and client code generation. --- ## Code Quality Assessment | Aspect | Rating | Notes | |--------|--------|-------| | Bug Fix Correctness | ✅ Good | Minimal, targeted fix | | Test Coverage | ✅ Good | New test covers the fixed case | | Documentation | ✅ Good | Improved docstring explains behavior | | Breaking Change Handling | ⚠️ Needs Attention | Release notes required | | Security | ✅ No Issues | Read-only endpoint, no injection risks | --- ## Verdict The PR is a ***valid, well-executed fix*** for a real deployment issue. The root cause was a hardcoded default that bypassed the service layer's proper configuration handling. ### Potential Risks 1. Clients relying on the old default behavior will break 2. OpenAPI schema changes may affect typed API clients ### Recommendations - ✅ Ensure release notes document the breaking change - ✅ Monitor for client-side issues after deployment - ⚠️ Consider adding migration guidance for affected clients