fix: use actual room versions instead of defaultRoomVersion everywhere - #407
Open
debdutdeb wants to merge 1 commit into
Open
fix: use actual room versions instead of defaultRoomVersion everywhere#407debdutdeb wants to merge 1 commit into
defaultRoomVersion everywhere#407debdutdeb wants to merge 1 commit into
Conversation
Contributor
WalkthroughChangesRoom version federation flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RoomService
participant StateService
participant Federation
RoomService->>StateService: Resolve existing room version
StateService-->>RoomService: Return room version or UnknownRoomError
RoomService->>Federation: Request join with known room version
Federation-->>RoomService: Return join state
RoomService->>RoomService: Process join state using rejoin status
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #407 +/- ##
==========================================
- Coverage 51.81% 51.79% -0.03%
==========================================
Files 114 114
Lines 12704 12712 +8
==========================================
+ Hits 6583 6584 +1
- Misses 6121 6128 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/federation-sdk/src/services/room.service.ts`:
- Around line 765-781: After makeJoinResponse is returned in the rejoin path,
compare its room_version with knownRoomVersion before calling sendJoin; reject
or throw on a mismatch, while preserving the existing flow for new joins where
knownRoomVersion is undefined. Locate the sendJoin invocation and enforce this
validation before it.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe4a6e52-5f75-4d44-b991-fab96b789a5f
📒 Files selected for processing (4)
packages/federation-sdk/src/sdk.tspackages/federation-sdk/src/services/room.service.tspackages/homeserver/src/controllers/internal/external-federation-request.controller.tspackages/room/src/manager/factory.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Code Quality Checks(lint, test, tsc)
🔇 Additional comments (8)
packages/federation-sdk/src/sdk.ts (1)
262-264: LGTM!packages/room/src/manager/factory.ts (1)
41-41: LGTM!packages/federation-sdk/src/services/room.service.ts (4)
443-443: LGTM!Also applies to: 517-517
746-746: LGTM!
811-813: LGTM!
1131-1131: LGTM!Also applies to: 1182-1182
packages/homeserver/src/controllers/internal/external-federation-request.controller.ts (2)
64-64: LGTM!
206-206: LGTM!Also applies to: 222-222
Comment on lines
+765
to
+781
| // Resident server is remote, need to do join flow. | ||
| // If we already have local state for this room (re-join), hint our known | ||
| // version to the resident server instead of asking it to pick from the | ||
| // full supported list. | ||
| let knownRoomVersion: RoomVersion | undefined; | ||
| let isRejoin = false; | ||
| try { | ||
| knownRoomVersion = await stateService.getRoomVersion(roomId); | ||
| isRejoin = true; | ||
| } catch (error) { | ||
| if (!(error instanceof UnknownRoomError)) { | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| // trying to join room from another server | ||
| const makeJoinResponse = await federationService.makeJoin( | ||
| residentServer, | ||
| roomId, | ||
| userId, | ||
| roomVersion, // NOTE: check the comment in the called method | ||
| ); | ||
| const makeJoinResponse = await federationService.makeJoin(residentServer, roomId, userId, knownRoomVersion); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the makeJoin implementation and its tests.
rg -n -C 8 --type ts '\bmakeJoin\s*\(' packages/federation-sdk/src
# Inspect whether the implementation compares a requested known version with
# the returned room_version before returning the response to RoomService.
fd -t f --extension ts . packages/federation-sdk/src | while IFS= read -r file; do
rg -n -C 12 'knownRoomVersion|room_version|makeJoin' "$file" || true
doneRepository: RocketChat/homeserver
Length of output: 46182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== FederationService.makeJoin implementation =="
sed -n '1,55p' packages/federation-sdk/src/services/federation.service.ts
echo
echo "== RoomService.joinRoom relevant section =="
sed -n '760,905p' packages/federation-sdk/src/services/room.service.ts
echo
echo "== sendJoin implementation =="
sed -n '44,70p' packages/federation-sdk/src/services/federation.service.ts
echo
echo "== Schemas =="
sed -n '87,102p' packages/federation-sdk/src/services/federation.service.ts 2>/dev/null || rg -n -C 8 'MakeJoinResponse|MakeJoinEvent' packages/federation-sdk/src | head -80
echo
echo "== Search for makeJoinResponse.room_version validation =="
rg -n -C 5 'makeJoinResponse\.room_version|if \(.*room_version|throw.*room_version|not equal|!==|!=|same' packages/federation-sdk/src/services/room.service.ts packages/federation-sdk/src/services/federation.service.ts packages/federation-sdk/srcRepository: RocketChat/homeserver
Length of output: 50378
Reject mismatched room versions after makeJoin.
When knownRoomVersion is set, compare makeJoinResponse.room_version to the local room version before calling sendJoin. makeJoin only passes ver to the request, so a different remote response can still be parsed with the remote version instead of the stored local room state.
🤖 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/federation-sdk/src/services/room.service.ts` around lines 765 - 781,
After makeJoinResponse is returned in the rejoin path, compare its room_version
with knownRoomVersion before calling sendJoin; reject or throw on a mismatch,
while preserving the existing flow for new joins where knownRoomVersion is
undefined. Locate the sendJoin invocation and enforce this validation before it.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/homeserver/src/controllers/internal/external-federation-request.controller.ts">
<violation number="1" location="packages/homeserver/src/controllers/internal/external-federation-request.controller.ts:64">
P2: The `/internal/event/template` devtools endpoint used to fall back to `PersistentEventFactory.defaultRoomVersion` when the client didn't pass `?version=`. Now `getRoomVersion(roomId)` is the fallback, but `StateService.getRoomVersion` throws `UnknownRoomError` when the room has no local `m.room.create` event. This endpoint generates a template event to fill in and send, frequently for rooms you haven't joined locally or are about to create, so the unhandled throw now turns the endpoint into a 500 instead of returning a template with a default version. Consider catching `UnknownRoomError` (or a `.catch` fallback) so the page keeps working for rooms without local state.</violation>
<violation number="2" location="packages/homeserver/src/controllers/internal/external-federation-request.controller.ts:206">
P2: Same regression as the template endpoint: the `/internal/event/send` route used to default to `PersistentEventFactory.defaultRoomVersion`, but the new fallback calls `federationSDK.getRoomVersion(event.room_id)`, which throws `UnknownRoomError` when the target room has no local `m.room.create` event. The query schema was also changed so `version` is now fully optional. This makes it impossible to send a room's first events (e.g. the initial `m.room.create`/`m.room.member` events for a brand-new room) through this devtool, because the version lookup itself raises before the event is built. Add a `UnknownRoomError` guard or a `.catch` fallback to `defaultRoomVersion` so version resolution failures degrade gracefully.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| async ({ body, query }) => { | ||
| const event = body as Pdu; | ||
| const version = (query.version as RoomVersion | undefined) || PersistentEventFactory.defaultRoomVersion; | ||
| const version = (query?.version as RoomVersion | undefined) || (await federationSDK.getRoomVersion(event.room_id)); |
There was a problem hiding this comment.
P2: Same regression as the template endpoint: the /internal/event/send route used to default to PersistentEventFactory.defaultRoomVersion, but the new fallback calls federationSDK.getRoomVersion(event.room_id), which throws UnknownRoomError when the target room has no local m.room.create event. The query schema was also changed so version is now fully optional. This makes it impossible to send a room's first events (e.g. the initial m.room.create/m.room.member events for a brand-new room) through this devtool, because the version lookup itself raises before the event is built. Add a UnknownRoomError guard or a .catch fallback to defaultRoomVersion so version resolution failures degrade gracefully.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/homeserver/src/controllers/internal/external-federation-request.controller.ts, line 206:
<comment>Same regression as the template endpoint: the `/internal/event/send` route used to default to `PersistentEventFactory.defaultRoomVersion`, but the new fallback calls `federationSDK.getRoomVersion(event.room_id)`, which throws `UnknownRoomError` when the target room has no local `m.room.create` event. The query schema was also changed so `version` is now fully optional. This makes it impossible to send a room's first events (e.g. the initial `m.room.create`/`m.room.member` events for a brand-new room) through this devtool, because the version lookup itself raises before the event is built. Add a `UnknownRoomError` guard or a `.catch` fallback to `defaultRoomVersion` so version resolution failures degrade gracefully.</comment>
<file context>
@@ -203,7 +203,7 @@ export const internalRequestPlugin = (app: Elysia) => {
async ({ body, query }) => {
const event = body as Pdu;
- const version = (query.version as RoomVersion | undefined) || PersistentEventFactory.defaultRoomVersion;
+ const version = (query?.version as RoomVersion | undefined) || (await federationSDK.getRoomVersion(event.room_id));
if (!PersistentEventFactory.isSupportedRoomVersion(version)) {
throw new Error(`Room version ${version} is not supported`);
</file context>
| sender: UserID; | ||
| }; | ||
| const version = (query.version as RoomVersion | undefined) || PersistentEventFactory.defaultRoomVersion; | ||
| const version = (query.version as RoomVersion | undefined) || (await federationSDK.getRoomVersion(roomId)); |
There was a problem hiding this comment.
P2: The /internal/event/template devtools endpoint used to fall back to PersistentEventFactory.defaultRoomVersion when the client didn't pass ?version=. Now getRoomVersion(roomId) is the fallback, but StateService.getRoomVersion throws UnknownRoomError when the room has no local m.room.create event. This endpoint generates a template event to fill in and send, frequently for rooms you haven't joined locally or are about to create, so the unhandled throw now turns the endpoint into a 500 instead of returning a template with a default version. Consider catching UnknownRoomError (or a .catch fallback) so the page keeps working for rooms without local state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/homeserver/src/controllers/internal/external-federation-request.controller.ts, line 64:
<comment>The `/internal/event/template` devtools endpoint used to fall back to `PersistentEventFactory.defaultRoomVersion` when the client didn't pass `?version=`. Now `getRoomVersion(roomId)` is the fallback, but `StateService.getRoomVersion` throws `UnknownRoomError` when the room has no local `m.room.create` event. This endpoint generates a template event to fill in and send, frequently for rooms you haven't joined locally or are about to create, so the unhandled throw now turns the endpoint into a 500 instead of returning a template with a default version. Consider catching `UnknownRoomError` (or a `.catch` fallback) so the page keeps working for rooms without local state.</comment>
<file context>
@@ -61,7 +61,7 @@ export const internalRequestPlugin = (app: Elysia) => {
sender: UserID;
};
- const version = (query.version as RoomVersion | undefined) || PersistentEventFactory.defaultRoomVersion;
+ const version = (query.version as RoomVersion | undefined) || (await federationSDK.getRoomVersion(roomId));
switch (eventType) {
case 'm.room.member': {
</file context>
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
2 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.
defaultRoomVersionshould only be used at creation of a room. Everywhere else, grab version from state or from create event whichever is closer.From claude
getRoomVersionreturns default if fails to get from state, i'd argue we allow it to return undefined instead. but not touching that code here.Claude wrote this patch.
Summary by CodeRabbit
New Features
Bug Fixes
Changes