fix(client-python): normalize server URI paths before appending task service#738
Open
wdwd720 wants to merge 5 commits intorocketride-org:developfrom
Open
fix(client-python): normalize server URI paths before appending task service#738wdwd720 wants to merge 5 commits intorocketride-org:developfrom
wdwd720 wants to merge 5 commits intorocketride-org:developfrom
Conversation
wdwd720
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
Contributor
📝 WalkthroughWalkthroughParses and normalizes configured URIs to produce websocket endpoints: trims trailing slashes, ensures Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
|
No description provided. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/client-python/tests/test_connection_uri.py`:
- Around line 6-15: Add explicit http->ws and wss trailing-slash regression
cases to the pytest parametrize list: include entries mapping
'http://localhost:5565' -> 'ws://localhost:5565/task/service' and
'http://localhost:5565/' -> 'ws://localhost:5565/task/service' (to cover
HTTP->WS conversion), and include 'wss://cloud.rocketride.ai' ->
'wss://cloud.rocketride.ai/task/service' and 'wss://cloud.rocketride.ai/' ->
'wss://cloud.rocketride.ai/task/service' (to cover WSS trailing-slash handling)
alongside the existing ('input_uri','expected_uri') tuples in the
test_connection_uri parametrize block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 31f616bf-0012-47aa-af07-f1d072ec806b
📒 Files selected for processing (2)
packages/client-python/src/rocketride/mixins/connection.pypackages/client-python/tests/test_connection_uri.py
Author
|
Thanks, added explicit http→ws and wss trailing-slash regression cases and reran the focused URI normalization test.
|
…ask service Apply the same URI normalization fix from the Python client to the TypeScript client. _getWebsocketUri() now strips trailing slashes and checks whether /task/service is already present before appending it, preventing double-slash and duplicate-path issues. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Rod-Christensen
left a comment
Rod-Christensen
left a comment
There was a problem hiding this comment.
Thank you --- this is great!
I also added the typescript client fix in there as well!
Rod-Christensen
approved these changes
May 1, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/client-typescript/src/client/client.ts`:
- Around line 446-454: Add TypeScript tests to the existing "RocketRideClient
URI normalization" suite to cover the regression cases this PR fixes: add cases
that ensure duplicate slashes are collapsed (e.g., input
"wss://host//task/service" => normalized "wss://host/task/service") and cases
that prevent duplicated path segments (e.g., input
"wss://host/task/service/task/service" => normalized "wss://host/task/service").
Place these new assertions alongside the existing RocketRideClient tests (the
suite referenced in client.ts URI normalization logic that uses SERVICE_PATH and
the try/catch path normalization), verifying both the parsed/wsScheme branch and
the fallback unparseable-URI branch produce the expected single canonical path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f19fabee-6f99-400d-84db-d51237daed3a
📒 Files selected for processing (1)
packages/client-typescript/src/client/client.ts
Comment on lines
+446
to
+454
| let path = url.pathname.replace(/\/+$/, ''); | ||
| if (!path.endsWith(SERVICE_PATH)) { | ||
| path = path ? `${path}${SERVICE_PATH}` : SERVICE_PATH; | ||
| } | ||
| return `${wsScheme}//${url.host}${path}`; | ||
| } catch { | ||
| return `${httpUrl}/task/service`; | ||
| // Fallback for unparseable URIs — same trailing-slash + dedup logic | ||
| const stripped = httpUrl.replace(/\/+$/, ''); | ||
| return stripped.endsWith(SERVICE_PATH) ? stripped : `${stripped}${SERVICE_PATH}`; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the TS test file already covers trailing-slash and pre-normalized-path cases.
rg -n "trailing|task/service" --type=ts packages/client-typescript/tests/Repository: rocketride-org/rocketride-server
Length of output: 608
🏁 Script executed:
# First, read the actual test file context around the URI normalization tests
sed -n '2186,2200p' packages/client-typescript/tests/RocketRideClient.test.tsRepository: rocketride-org/rocketride-server
Length of output: 681
🏁 Script executed:
# Read the code being tested to verify the logic matches the suggested test cases
sed -n '440,460p' packages/client-typescript/src/client/client.tsRepository: rocketride-org/rocketride-server
Length of output: 880
TypeScript tests are missing the exact regression cases this PR fixes.
The existing RocketRideClient URI normalization suite (lines 2186-2198) covers only bare host inputs. The PR's stated fixes — preventing double trailing slashes (e.g., wss://host//task/service) and avoiding duplicate path segments (e.g., wss://host/task/service/task/service) — have no TS-side test coverage.
Suggested additions:
it.each([
['wss://cloud.rocketride.ai', 'wss://cloud.rocketride.ai/task/service'],
['https://cloud.rocketride.ai', 'wss://cloud.rocketride.ai/task/service'],
['ws://localhost:5565', 'ws://localhost:5565/task/service'],
['http://localhost:5565', 'ws://localhost:5565/task/service'],
+ // trailing-slash regression: must not produce double slash
+ ['wss://cloud.rocketride.ai/', 'wss://cloud.rocketride.ai/task/service'],
+ ['https://cloud.rocketride.ai/', 'wss://cloud.rocketride.ai/task/service'],
+ ['http://localhost:5565/', 'ws://localhost:5565/task/service'],
+ // pre-normalized path: must not duplicate /task/service
+ ['wss://cloud.rocketride.ai/task/service', 'wss://cloud.rocketride.ai/task/service'],
+ ['https://cloud.rocketride.ai/task/service', 'wss://cloud.rocketride.ai/task/service'],
])('normalizes %s to %s', (inputUri, expectedUri) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client-typescript/src/client/client.ts` around lines 446 - 454, Add
TypeScript tests to the existing "RocketRideClient URI normalization" suite to
cover the regression cases this PR fixes: add cases that ensure duplicate
slashes are collapsed (e.g., input "wss://host//task/service" => normalized
"wss://host/task/service") and cases that prevent duplicated path segments
(e.g., input "wss://host/task/service/task/service" => normalized
"wss://host/task/service"). Place these new assertions alongside the existing
RocketRideClient tests (the suite referenced in client.ts URI normalization
logic that uses SERVICE_PATH and the try/catch path normalization), verifying
both the parsed/wsScheme branch and the fallback unparseable-URI branch produce
the expected single canonical path.
3 tasks
kwit75
added a commit
that referenced
this pull request
May 1, 2026
…#742) Replace \`ROCKETRIDE_APIKEY: \${{ secrets.ROCKETRIDE_APIKEY }}\` with a literal \`MYAPIKEY\` in the Test step env block. Why this is unblocking the queue -------------------------------- PR #712 set up the env var to fix "No authentication configured" failures in client-python integration tests. Its own inline comment correctly noted that "the secret value itself doesn't matter — it just has to match between server and client in this single CI run." Sourcing it from \`secrets.ROCKETRIDE_APIKEY\` introduced an empty-string failure mode that we hit: 1. The secret was created on 2026-04-27, has not been updated since, and may be set to "" (or rotated to a value the engine no longer accepts). 2. When that happens, the workflow silently expands the expression to \`ROCKETRIDE_APIKEY=""\` for the Test step. 3. The test client reads it via \`os.getenv('ROCKETRIDE_APIKEY', 'MYAPIKEY')\`. \`os.getenv\` returns the empty string when the variable is set-but-empty — NOT the default — so the client authenticates with \`""\`. 4. The server (running in the same step) sees the same empty key and responds AuthenticationException. 5. All 48 client-python integration tests fail uniformly across Ubuntu, Windows, and macOS (which is what's been happening on develop's most recent runs and on PRs #715, #728, #738). Using a literal value eliminates the entire failure mode without changing observable behaviour: the value still isn't a secret (the inline comment was always explicit on this), it never leaves the runner, and it matches the "MYAPIKEY" dev key the engine already recognises elsewhere in the codebase (\`.env.template\`). Together with #734 (the sequential test execution flag, already on develop) this should clear both failure modes that have been blocking PRs since yesterday. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
//task/servicefor trailing-slash server URLs./task/service/task/servicewhen callers pass an already-normalized WebSocket endpoint.Type
fix
Testing
./builder testpassesChecklist
Linked Issue
Fixes #737
Problem
ConnectionMixin._get_websocket_uri()converted the URI scheme and then appended/task/serviceto the full URL string unconditionally. Inputs with a trailing slash produced a double slash beforetask/service, and inputs already ending in/task/serviceduplicated the service path.Fix
Normalize the parsed URL path first, then append
/task/serviceonly when the cleaned path does not already end with it.This preserves existing host-only URI behavior and keeps successful scheme conversion unchanged.
Local Testing
PYTHONDONTWRITEBYTECODE=1 python3 -m pytest packages/client-python/tests/test_connection_uri.py -q✅PYTHONDONTWRITEBYTECODE=1 python3 -m pytest packages/client-python/tests/test_connection_uri.py packages/client-python/tests/test_client_env_loading.py -q✅Notes
This PR is intentionally scoped to Python client URI path normalization and does not change authentication, environment loading, connection retry behavior, or network transport behavior.
Summary by CodeRabbit
Bug Fixes
Tests