fix: normalize WebSocket URI path to prevent /task/service duplicationFix/websocket uri 755#778
Conversation
koushik1359
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
github-actions
Bot
added
module:client-python
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR refactors WebSocket URI construction in the Python client to use ChangesWebSocket URI Normalization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Comment |
|
No description provided. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ai/src/ai/common/store.py (1)
157-157:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPre-existing critical bug: score update replaces
Docwith a plain number.Line 157 writes
tableDocs[tableKey] = doc.score, which overwrites the storedDocobject with afloat. Any subsequent access — including line 189'stableDocs[tableKey].page_content += ...— will raiseAttributeError: 'float' object has no attribute 'page_content'. The intent is clearly to update the score field on the existing entry.🐛 Proposed fix
- if doc.score > tableDocs[tableKey].score: - tableDocs[tableKey] = doc.score + if doc.score > tableDocs[tableKey].score: + tableDocs[tableKey].score = doc.score🤖 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/ai/src/ai/common/store.py` at line 157, The bug is that tableDocs[tableKey] = doc.score replaces the stored Doc with a float; instead, update the score attribute on the stored Doc object (e.g., tableDocs[tableKey].score = doc.score) and only replace the whole entry if no Doc exists yet (i.e., if tableDocs.get(tableKey) is None then set tableDocs[tableKey] = doc). Update the logic around tableDocs, tableKey, and doc so later accesses like tableDocs[tableKey].page_content remain valid.
🤖 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.
Inline comments:
In `@packages/ai/src/ai/common/store.py`:
- Around line 184-189: The code clears chunk.page_content before appending, but
when tableDocs[tableKey] is set to chunk they reference the same object so the
original text is lost; fix by capturing the chunk's original content into a
local variable (e.g., original = chunk.page_content) before clearing or
assigning into tableDocs, then use that captured original when doing
tableDocs[tableKey].page_content += original; ensure references to tableDocs,
chunk, tableKey and page_content are used so the initialization branch preserves
the chunk text instead of appending an already-cleared string.
---
Outside diff comments:
In `@packages/ai/src/ai/common/store.py`:
- Line 157: The bug is that tableDocs[tableKey] = doc.score replaces the stored
Doc with a float; instead, update the score attribute on the stored Doc object
(e.g., tableDocs[tableKey].score = doc.score) and only replace the whole entry
if no Doc exists yet (i.e., if tableDocs.get(tableKey) is None then set
tableDocs[tableKey] = doc). Update the logic around tableDocs, tableKey, and doc
so later accesses like tableDocs[tableKey].page_content remain valid.
🪄 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: 324aba12-d814-4fbf-ad50-ab690b2efb57
📒 Files selected for processing (3)
packages/ai/src/ai/common/store.pypackages/client-python/src/rocketride/mixins/connection.pypackages/client-python/tests/RocketRideClient_test.py
| if tableKey not in tableDocs: | ||
| # Add it to the list | ||
| # TODO: Fix this | ||
| tableDocs[tableKey] = Doc(objectId=objectId, chunk=doc.metadata.chunkId, score=chunk.score) | ||
| chunk.page_content = '' | ||
| tableDocs[tableKey] = chunk | ||
|
|
||
| # Append the text | ||
| tableDocs[tableKey].page_content += chunk.page_content |
There was a problem hiding this comment.
Self-reference causes first chunk's content to be silently dropped.
After lines 185–186, tableDocs[tableKey] and chunk are the same object. Line 189 then evaluates as:
chunk.page_content += chunk.page_content # '' += '' → ''Because chunk.page_content was just cleared on line 185, both sides of += are '' and the original text of this chunk is lost. The correct pattern is to either append before clearing, use a continue to skip the unconditional +=, or restructure so the accumulation handles the initialisation case separately:
🐛 Proposed fix
if tableKey not in tableDocs:
- chunk.page_content = ''
tableDocs[tableKey] = chunk
+ tableDocs[tableKey].page_content = ''
# Append the text
tableDocs[tableKey].page_content += chunk.page_contentMoving the clear onto tableDocs[tableKey] after the assignment is the same object, but by separating the steps the intent is unambiguous. Alternatively, capture the content first:
if tableKey not in tableDocs:
+ original = chunk.page_content
chunk.page_content = ''
tableDocs[tableKey] = chunk
+ tableDocs[tableKey].page_content += original
+ continue
# Append the text
tableDocs[tableKey].page_content += chunk.page_contentNote: Because Phase 1 inserts every
(objectId, tableId)present indocumentsintotableDocsbefore Phase 2 runs, thisif tableKey not in tableDocsbranch is currently unreachable in normal flow. The bug is latent, but the defensive guard should still be correct.
🤖 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/ai/src/ai/common/store.py` around lines 184 - 189, The code clears
chunk.page_content before appending, but when tableDocs[tableKey] is set to
chunk they reference the same object so the original text is lost; fix by
capturing the chunk's original content into a local variable (e.g., original =
chunk.page_content) before clearing or assigning into tableDocs, then use that
captured original when doing tableDocs[tableKey].page_content += original;
ensure references to tableDocs, chunk, tableKey and page_content are used so the
initialization branch preserves the chunk text instead of appending an
already-cleared string.
String concatenation in _get_websocket_uri produced malformed URLs in two cases: - If the URI already contained /task/service, it was appended again - A trailing slash produced a double slash before /task/service Fix: strip trailing slash, check for existing /task/service suffix, then build the final URL with urlunparse instead of string formatting. Added two parametrized test cases covering both failure modes. Resolves rocketride-org#755
koushik1359
force-pushed
the
fix/websocket-uri-755
branch
from
aebed80 to
5f6dec4
Compare
1 participant
Problem
_get_websocket_uribuilt the final URL via string concatenation:This produced malformed URLs in two cases:
/task/service(e.g. after reconnect), the suffix was appended again, yielding
.../task/service/task/service.ws://localhost:5565//task/service.Fix
Strip the trailing slash, guard against an existing
/task/servicesuffix, and construct the final URL with
urllib.parse.urlunparseinstead of string formatting:
Tests
Added two parametrized cases to the existing
test_get_websocket_uri_normalizationsuite covering both failure modes.Resolves #755
Summary by CodeRabbit