Skip to content

fix: normalize python websocket task uri without duplicating path#755

Open
aryaMehta26 wants to merge 1 commit intorocketride-org:developfrom
aryaMehta26:fix/python-sdk-websocket-uri-737
Open

fix: normalize python websocket task uri without duplicating path#755
aryaMehta26 wants to merge 1 commit intorocketride-org:developfrom
aryaMehta26:fix/python-sdk-websocket-uri-737

Conversation

@aryaMehta26
Copy link
Copy Markdown

@aryaMehta26 aryaMehta26 commented May 5, 2026

Summary

Type

Testing

  • Tests added or updated
  • Tested locally
  • ./builder test passes

Checklist

  • Commit messages follow conventional commits
  • No secrets or credentials included
  • Wiki updated (if applicable)
  • Breaking changes documented (if applicable)

Linked Issue

Fixes #

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Improved WebSocket URL construction to reliably handle various URL format variations.
    • Enhanced URL normalization to ensure consistent routing to the service endpoint.
    • Fixed potential path composition issues in WebSocket URI generation.

@github-actions github-actions Bot added the module:client-python Python SDK and MCP client label May 5, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 5, 2026

📝 Walkthrough

Walkthrough

The PR refactors WebSocket URI construction in the Python client's ConnectionMixin._get_websocket_uri method to use urllib.parse.urlunparse instead of string concatenation, ensuring the URL path reliably ends with /task/service. Test cases are updated to validate the new normalization behavior.

Changes

WebSocket URI Path Normalization

Layer / File(s) Summary
Core Implementation
packages/client-python/src/rocketride/mixins/connection.py
_get_websocket_uri refactored to derive the path from the normalized URL and conditionally rewrite it to end with /task/service, then reconstruct the URI via urlunparse instead of string concatenation.
Tests
packages/client-python/tests/RocketRideClient_test.py
test_get_websocket_uri_normalization test cases updated to verify consistent normalization: trailing slashes are removed, /task/service is appended when absent, and scheme conversion (http→ws, https→wss) is preserved.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

Possibly related PRs

Suggested labels

module:client-python

Suggested reviewers

  • jmaionchi
  • stepmikhaylov
  • Rod-Christensen

Poem

🐰 A rabbit hops through URL lands so fine,
Where paths and schemes now neatly align!
No more tangled strings, just urlunparse grace,
/task/service finds its proper place. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: fixing WebSocket URI normalization to prevent path duplication in the Python client.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

No description provided.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/client-python/tests/RocketRideClient_test.py (1)

2366-2380: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

The else branch of _get_websocket_uri has no test coverage.

All seven parametrize cases route through either path.endswith('/task/service') or elif path == ''. The else path — appending /task/service to a non-root base (e.g. http://localhost:5565/api) — is never exercised. A regression there would go undetected.

✅ Proposed additional parametrize case
     ('ws://localhost:5565/task/service/', 'ws://localhost:5565/task/service'),
+    ('http://localhost:5565/api', 'ws://localhost:5565/api/task/service'),
 ],
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/client-python/tests/RocketRideClient_test.py` around lines 2366 -
2380, Add a test case that exercises the else branch of
ConnectionMixin._get_websocket_uri by supplying a base URI whose path is
non-empty and does not end with '/task/service' (for example
'http://localhost:5565/api') and assert it returns the websocket URI with
'/task/service' appended to that path (e.g.
'ws://localhost:5565/api/task/service'); ensure the new parametrize entry
mirrors existing cases so the condition that appends '/task/service' to a
non-root path is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@packages/client-python/tests/RocketRideClient_test.py`:
- Around line 2366-2380: Add a test case that exercises the else branch of
ConnectionMixin._get_websocket_uri by supplying a base URI whose path is
non-empty and does not end with '/task/service' (for example
'http://localhost:5565/api') and assert it returns the websocket URI with
'/task/service' appended to that path (e.g.
'ws://localhost:5565/api/task/service'); ensure the new parametrize entry
mirrors existing cases so the condition that appends '/task/service' to a
non-root path is covered.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5c592cfb-bed8-4115-9fa8-88bcf0ea43c4

📥 Commits

Reviewing files that changed from the base of the PR and between 8d71b16 and 8c4cefe.

📒 Files selected for processing (2)
  • packages/client-python/src/rocketride/mixins/connection.py
  • packages/client-python/tests/RocketRideClient_test.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module:client-python Python SDK and MCP client

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant