feat: state endpoints #164
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/state-ids-for-all-events #164 +/- ##
==================================================================
- Coverage 68.48% 67.95% -0.53%
==================================================================
Files 58 59 +1
Lines 5210 5374 +164
==================================================================
+ Hits 3568 3652 +84
- Misses 1642 1722 +80 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
feat/states_endpoints
branch
from
e60f36d to
73da4fa
Compare
ggazzo
force-pushed
the
feat/states_endpoints
branch
from
3e741d9 to
fe0f861
Compare
ggazzo
force-pushed
the
chore/state-ids-for-all-events
branch
from
d351d1b to
bcbae69
Compare
ggazzo
force-pushed
the
feat/states_endpoints
branch
3 times, most recently
from
74ce87b to
4a0e23e
Compare
| eventId: string, | ||
| ): Promise<{ pdu_ids: string[]; auth_chain_ids: string[] }> { | ||
| try { | ||
| const state = await this.stateService.findStateAtEvent(eventId); |
There was a problem hiding this comment.
This i think should be findStateBeforeEvent
spec says for response - "The fully resolved state for the room, prior to considering any state changes induced by the requested event. Includes the authorization chain for the events."
|
|
||
| if (eventId) { | ||
| // Get state at a specific event | ||
| state = await this.stateService.findStateAtEvent(eventId); |
There was a problem hiding this comment.
Same for this - " prior to considering any state changes induced by the requested event."
ggazzo
force-pushed
the
feat/states_endpoints
branch
from
8b57f53 to
454a10c
Compare
WalkthroughAdds federation state endpoints and DTOs, implements state retrieval and reconstruction logic in services (including new repository method), adds event/state federation APIs, removes multiple signature/verification and utility modules, and adjusts module wiring and package dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor FederationClient
participant HS as Homeserver (Elysia)
participant ES as EventService
participant SS as StateService
participant Repo as StateRepository
participant RR as RoomModule
rect rgba(230,245,255,0.6)
FederationClient->>HS: GET /_matrix/federation/v1/state_ids/:roomId?event_id=E
HS->>ES: getStateIds(roomId, E)
ES->>SS: findStateBeforeEvent(E, includeEvent=false)
SS->>Repo: getLatestStateMappingBeforeEvent(...) / load mappings & events
Repo-->>SS: state mappings / events
SS-->>ES: reconstructed state map
loop per state event
ES->>RR: getAuthChain(stateEvent)
RR-->>ES: auth_chain_ids
end
ES-->>HS: { pdu_ids, auth_chain_ids }
HS-->>FederationClient: 200 JSON
end
sequenceDiagram
autonumber
actor FederationClient
participant HS as Homeserver (Elysia)
participant ES as EventService
participant SS as StateService
participant Repo as StateRepository
participant RR as RoomModule
rect rgba(230,255,230,0.6)
FederationClient->>HS: GET /_matrix/federation/v1/state/:roomId?event_id=E
HS->>ES: getState(roomId, E)
ES->>SS: findStateBeforeEvent(E, includeEvent=false)
SS-->>ES: state map
ES->>RR: getAuthChain(all state events)
RR-->>ES: auth_chain_ids
ES->>Repo: getEvents(auth_chain_ids)
Repo-->>ES: auth events (pdus)
ES-->>HS: { pdus, auth_chain }
HS-->>FederationClient: 200 JSON
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
ggazzo
force-pushed
the
chore/state-ids-for-all-events
branch
from
374df87 to
ba7cbd2
Compare
ggazzo
force-pushed
the
feat/states_endpoints
branch
from
454a10c to
eb40a7b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/profiles.service.ts (1)
136-136: Use findStateBeforeEvent for spec compliance ("prior to considering any state changes induced by the requested event").Both state_ids and state endpoints must return the state prior to the event’s effects. Replace findStateAtEvent with findStateBeforeEvent in both methods.
- const state = await this.stateService.findStateAtEvent(eventId); + const state = await this.stateService.findStateBeforeEvent(eventId);- state = await this.stateService.findStateAtEvent(eventId); + state = await this.stateService.findStateBeforeEvent(eventId);Also applies to: 190-196
🧹 Nitpick comments (5)
packages/federation-sdk/src/services/profiles.service.ts (3)
188-188: Replace console.log with structured logger.- console.log('eventId', eventId); + this.logger.debug({ roomId, eventId }, 'getState');
156-167: Avoid redundant auth-chain walks; memoize per request.Calling getAuthChain for every state event repeats traversals. Introduce a per-request cache keyed by eventId to store previously computed chains and union them.
169-172: Deterministic output order (optional).Sorting pdu_ids and auth_chain_ids aids reproducibility and tests.
- return { - pdu_ids: pduIds, - auth_chain_ids: Array.from(authChainIds), - }; + return { + pdu_ids: pduIds.sort(), + auth_chain_ids: Array.from(authChainIds).sort(), + };packages/federation-sdk/src/repositories/state.repository.ts (1)
43-51: LGTM; add supporting index for performance.The before-event lookup is correct. Add a compound index on { roomId: 1, createdAt: -1 } to support this query and sort.
packages/federation-sdk/src/services/state.service.ts (1)
208-219: Use simple for-of loop (array, not async iterable).stateMappings is an array; for-await is unnecessary and slightly misleading.
- for await (const { delta } of stateMappings) { + for (const { delta } of stateMappings) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/federation-sdk/src/repositories/state.repository.ts(1 hunks)packages/federation-sdk/src/services/profiles.service.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(5 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(3 hunks)packages/homeserver/src/dtos/federation/profiles.dto.ts(1 hunks)packages/homeserver/src/dtos/federation/state-ids.dto.ts(1 hunks)packages/homeserver/src/dtos/federation/state.dto.ts(1 hunks)packages/homeserver/src/dtos/index.ts(1 hunks)packages/room/src/state_resolution/definitions/definitions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/homeserver/src/dtos/federation/state-ids.dto.ts
- packages/homeserver/src/dtos/federation/profiles.dto.ts
- packages/homeserver/src/dtos/federation/state.dto.ts
- packages/room/src/state_resolution/definitions/definitions.ts
- packages/homeserver/src/dtos/index.ts
- packages/homeserver/src/controllers/federation/profiles.controller.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/federation-sdk/src/services/profiles.service.ts (3)
packages/room/src/manager/event-wrapper.ts (2)
roomId(72-74)event(102-112)packages/room/src/manager/v3.ts (1)
eventId(21-33)packages/room/src/state_resolution/definitions/definitions.ts (1)
getAuthChain(91-135)
packages/federation-sdk/src/services/state.service.ts (2)
packages/room/src/manager/factory.ts (1)
PersistentEventFactory(37-968)packages/room/src/manager/event-wrapper.ts (2)
event(102-112)stateKey(90-94)
🔇 Additional comments (2)
packages/federation-sdk/src/services/state.service.ts (2)
149-160: Create-event early return matches spec semantics.Returning empty state for m.room.create is correct.
170-177: event.createdAt is correct here — it's the EventStore DB timestampEventRepository persists a top-level createdAt (packages/federation-sdk/src/repositories/event.repository.ts) and EventStore defines createdAt (packages/core/src/models/event.model.ts). getLatestStateMappingBeforeEvent expects a Date (packages/federation-sdk/src/repositories/state.repository.ts). originServerTs is the PDU's origin_server_ts (event.event.origin_server_ts / originServerTs) and is used elsewhere for PDU-level queries. No change required.
| // Get room version for the store | ||
| const roomVersion = await this.stateService.getRoomVersion(roomId); | ||
| if (!roomVersion) { | ||
| throw new Error('Room version not found'); | ||
| } | ||
|
|
||
| // Get the event store | ||
| const store = this.stateService._getStore(roomVersion); | ||
|
|
There was a problem hiding this comment.
Guard against room/event mismatch and derive room version from the event’s actual room.
Currently roomVersion is derived from the request roomId, which can mismatch the event’s room, leading to incorrect stores and wrong auth chain/state. Validate the event belongs to the room and use its room_id to fetch the version.
@@ async getStateIds(roomId: string, eventId: string)
- const state = await this.stateService.findStateBeforeEvent(eventId);
+ const ev = await this.eventRepository.findById(eventId);
+ if (!ev) {
+ throw new Error(`Event ${eventId} not found`);
+ }
+ if (ev.event.room_id !== roomId) {
+ throw new Error(`event ${eventId} does not belong to room ${roomId}`);
+ }
+ const state = await this.stateService.findStateBeforeEvent(eventId);
- const roomVersion = await this.stateService.getRoomVersion(roomId);
+ const roomVersion = await this.stateService.getRoomVersion(ev.event.room_id);@@ async getState(roomId: string, eventId?: string)
- let state: Map<string, any>;
- console.log('eventId', eventId);
+ let state: Map<string, any>;
+ this.logger.debug({ roomId, eventId }, 'getState');
+ let effectiveRoomId = roomId;
if (eventId) {
- state = await this.stateService.findStateBeforeEvent(eventId);
+ const ev = await this.eventRepository.findById(eventId);
+ if (!ev) {
+ throw new Error(`Event ${eventId} not found`);
+ }
+ if (ev.event.room_id !== roomId) {
+ throw new Error(`event ${eventId} does not belong to room ${roomId}`);
+ }
+ effectiveRoomId = ev.event.room_id;
+ state = await this.stateService.findStateBeforeEvent(eventId);
} else {
// Get current room state
state = await this.stateService.getFullRoomState(roomId);
}
- const roomVersion = await this.stateService.getRoomVersion(roomId);
+ const roomVersion = await this.stateService.getRoomVersion(effectiveRoomId);Also applies to: 201-209
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/profiles.service.ts around lines
141-149, the code derives roomVersion from the request roomId and then gets a
store, which can be wrong if the event's room differs; instead, parse/validate
the incoming event to ensure event.room_id matches the request roomId (or prefer
the event.room_id as the source of truth), throw a clear error on mismatch, and
use the event.room_id to call stateService.getRoomVersion and _getStore so the
auth chain/state use the correct room version; apply the same guard-and-derive
change to the similar block at lines 201-209.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/federation-sdk/src/services/profiles.service.ts (3)
131-149: Validate event belongs to the requested room; derive roomVersion/store from the event’s actual room_id.Prevents wrong state/auth_chain when eventId is from another room; also improves error clarity.
Apply this diff:
@@ async getStateIds( roomId: string, eventId: string, ): Promise<{ pdu_ids: string[]; auth_chain_ids: string[] }> { try { - const state = await this.stateService.findStateBeforeEvent(eventId); + const ev = await this.eventRepository.findById(eventId); + if (!ev) { + throw new Error(`Event ${eventId} not found`); + } + if (ev.event.room_id !== roomId) { + throw new Error(`event ${eventId} does not belong to room ${roomId}`); + } + const effectiveRoomId = ev.event.room_id; + const state = await this.stateService.findStateBeforeEvent(eventId); @@ - const roomVersion = await this.stateService.getRoomVersion(roomId); + const roomVersion = await this.stateService.getRoomVersion(effectiveRoomId); if (!roomVersion) { throw new Error('Room version not found'); } // Get the event store const store = this.stateService._getStore(roomVersion);
186-196: Replace console.log with structured logging; guard event/room mismatch and track effectiveRoomId.Ensures correct state selection and consistent logging.
- let state: Map<string, any>; - console.log('eventId', eventId); + let state: Map<string, any>; + this.logger.debug({ roomId, eventId }, 'getState'); + let effectiveRoomId = roomId; if (eventId) { - // Get state at a specific event - state = await this.stateService.findStateBeforeEvent(eventId); + const ev = await this.eventRepository.findById(eventId); + if (!ev) { + throw new Error(`Event ${eventId} not found`); + } + if (ev.event.room_id !== roomId) { + throw new Error(`event ${eventId} does not belong to room ${roomId}`); + } + effectiveRoomId = ev.event.room_id; + state = await this.stateService.findStateBeforeEvent(eventId); } else { // Get current room state state = await this.stateService.getFullRoomState(roomId); }
201-209: Derive roomVersion using effectiveRoomId; prefer a public store accessor.Room version must come from the event’s room. Also consider replacing _getStore with a public method (e.g., getStore) to avoid relying on internals.
- const roomVersion = await this.stateService.getRoomVersion(roomId); + const roomVersion = await this.stateService.getRoomVersion(effectiveRoomId); if (!roomVersion) { throw new Error('Room version not found'); } // Get the event store const store = this.stateService._getStore(roomVersion);
🧹 Nitpick comments (3)
packages/federation-sdk/src/services/profiles.service.ts (3)
151-167: Iterate values directly and consider shared memoization for auth-chain traversal.
- Minor: use state.values() to avoid creating and discarding tuple entries.
- Perf (optional): getAuthChain is invoked per state event with its own internal cache; across dozens/hundreds of state events this repeats work. Consider a shared traversal or a memoizing wrapper to cut redundant recursion.
- for (const [, event] of state.entries()) { + for (const event of state.values()) { // PersistentEventBase has an eventId getter pduIds.push(event.eventId);
211-231: Small loop tidy-up for readability.Iterate state.values() and keep the existing guards.
- for (const [, event] of state.entries()) { - if (event && typeof event === 'object') { - // PersistentEventBase has an event getter that contains the actual event data - if ('event' in event) { - pdus.push(event.event); + for (const event of state.values()) { + if (event && typeof event === 'object' && 'event' in event) { + // PersistentEventBase has an `event` getter that contains the actual event data + pdus.push(event.event);
131-177: Add minimal tests for state/state_ids happy path, mismatch, and not-found.Covers main correctness paths and helps prevent regressions:
- eventId belongs to roomId → returns state/auth_chain.
- eventId not found → 404/propagated error.
- eventId for different room → 400/clear error.
I can draft unit tests for ProfilesService using a stubbed StateService/EventRepository and sample PersistentEventBase instances. Want me to open a follow-up PR with tests?
Also applies to: 179-256
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/federation-sdk/src/services/profiles.service.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/federation-sdk/src/services/state.service.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/profiles.service.ts (3)
packages/room/src/manager/event-wrapper.ts (2)
roomId(72-74)event(102-112)packages/room/src/manager/v3.ts (1)
eventId(21-33)packages/room/src/state_resolution/definitions/definitions.ts (1)
getAuthChain(91-135)
🔇 Additional comments (1)
packages/federation-sdk/src/services/profiles.service.ts (1)
5-12: LGTM: correct import of getAuthChain and related room types.This aligns the service with the room module’s auth-chain API.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/federation-sdk/src/services/state.service.ts (1)
209-219: Don’t mutate prevStateIds; correct error messages.Reintroduces the pop() mutation and “Event … not found” wording previously flagged.
- const state = await this.stateRepository.getStateById(stateId); - if (!state) { - throw new Error(`Event ${stateId} not found`); - } - const prevStateId = state.prevStateIds.pop(); - if (!prevStateId) { - throw new Error(`State ${stateId} has no previous state`); - } + const state = await this.stateRepository.getStateById(stateId); + if (!state) { + throw new Error(`State mapping ${stateId} not found`); + } + const prevStateIds = state.prevStateIds; + const prevStateId = prevStateIds[prevStateIds.length - 1]; + if (!prevStateId) { + throw new Error(`State mapping ${stateId} has no previous state`); + }packages/federation-sdk/src/services/profiles.service.ts (2)
141-149: Validate room/event match and derive roomVersion from the event’s room.Prevents mismatched stores/auth chains when the request roomId differs from the event’s room.
- // Get room version for the store - const roomVersion = await this.stateService.getRoomVersion(roomId); + // Validate event and derive the effective room id + const ev = await this.eventRepository.findById(eventId); + if (!ev) { + throw new Error(`Event ${eventId} not found`); + } + if (ev.event.room_id !== roomId) { + throw new Error(`event ${eventId} does not belong to room ${roomId}`); + } + // Get room version for the store + const roomVersion = await this.stateService.getRoomVersion(ev.event.room_id);Also update the store construction below to use the same roomVersion (no further changes needed).
196-204: Same fix: validate event->room and use its room version.Mirror the guard in getStateIds to avoid wrong store/auth chain.
- // Get room version for the store - const roomVersion = await this.stateService.getRoomVersion(roomId); + // Validate event and derive the effective room id + const ev = await this.eventRepository.findById(eventId); + if (!ev) { + throw new Error(`Event ${eventId} not found`); + } + if (ev.event.room_id !== roomId) { + throw new Error(`event ${eventId} does not belong to room ${roomId}`); + } + // Get room version for the store + const roomVersion = await this.stateService.getRoomVersion(ev.event.room_id);
🧹 Nitpick comments (6)
packages/room/src/state_resolution/definitions/definitions.ts (2)
105-106: Memoize computed chains to avoid recomputation.Only zero-auth branches are cached. Cache the computed set for all paths.
if (eventIdToAuthChainMap.has(eventId)) { return eventIdToAuthChainMap.get(eventId)!; } @@ - return newAuthChainPart; + eventIdToAuthChainMap.set(eventId, newAuthChainPart); + return newAuthChainPart;Also applies to: 131-131
109-110: Drop console.log; use the project logger or remove.Noisy in libs and shows up in CI formatting diffs.
- console.log('authEvents ->>>', authEvents); + // this module should be silent; consider injecting a logger if needed @@ - console.log('authEvent ->>>', authEvent); + // debug log removedAlso applies to: 120-121
packages/federation-sdk/src/services/state.service.ts (2)
251-252: Remove console.log; use the class logger.These debug logs also trigger the formatting failure noted by CI.
- console.log('pdu ->>>', pdu.isState(), pdu.type); + this.logger.debug({ isState: pdu.isState(), type: pdu.type }, 'pdu'); @@ - console.log('stateId ->>>', stateId); + this.logger.debug({ stateId }, 'state id');Also applies to: 260-263
478-480: Prefer findStateBeforeEvent for “before event” semantics.This method name suggests the “before” view; using findStateAroundEvent can include the event depending on flags. Consider delegating to findStateBeforeEvent.
- const state = await this.findStateAroundEvent(eventId); + const state = await this.findStateBeforeEvent(eventId);packages/federation-sdk/src/services/profiles.service.ts (2)
154-154: Remove console.log in hot path.Use this.logger.debug if you need it.
- console.log('event', event); + // debug: this.logger.debug({ eventId: event.eventId }, 'state event');
188-188: Remove console.log to reduce noise and fix formatting diffs.Switch to the class logger if you need these.
- console.log('eventId', eventId); + this.logger.debug({ roomId, eventId }, 'getState'); @@ - console.log('event ->>>', event); + // debug: this.logger.debug({ eventId: event.eventId }, 'state event');Also applies to: 210-210
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/federation-sdk/src/services/profiles.service.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(3 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(3 hunks)packages/homeserver/src/dtos/federation/state.dto.ts(1 hunks)packages/room/src/state_resolution/definitions/definitions.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/homeserver/src/dtos/federation/state.dto.ts
- packages/homeserver/src/controllers/federation/profiles.controller.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/room/src/state_resolution/definitions/definitions.ts (2)
packages/room/src/manager/v3.ts (1)
eventId(21-33)packages/room/src/manager/event-wrapper.ts (1)
event(102-112)
packages/federation-sdk/src/services/profiles.service.ts (3)
packages/room/src/manager/event-wrapper.ts (2)
roomId(72-74)event(102-112)packages/room/src/manager/v3.ts (1)
eventId(21-33)packages/room/src/state_resolution/definitions/definitions.ts (1)
getAuthChain(91-137)
packages/federation-sdk/src/services/state.service.ts (3)
packages/room/src/types/_common.ts (2)
State(9-9)StateMapKey(7-7)packages/room/src/manager/event-wrapper.ts (2)
event(102-112)stateKey(90-94)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(37-968)
🪛 GitHub Actions: my-workflow
packages/room/src/state_resolution/definitions/definitions.ts
[error] 132-137: File content differs from formatting output
packages/federation-sdk/src/services/profiles.service.ts
[error] 188-210: File content differs from formatting output
packages/federation-sdk/src/services/state.service.ts
[error] 248-252: File content differs from formatting output
🔇 Additional comments (3)
packages/room/src/state_resolution/definitions/definitions.ts (1)
132-137: Fix formatting to satisfy CI.Pipeline flagged “File content differs from formatting output” around here. Run the repo formatter (e.g., pnpm format / biome).
packages/federation-sdk/src/services/state.service.ts (1)
248-252: Formatting drift detected by CI.GitHub Actions reported “File content differs from formatting output” here. Please run the repo formatter.
packages/federation-sdk/src/services/profiles.service.ts (1)
188-210: Formatting drift flagged by CI.Action reported “File content differs from formatting output” in this region. Please run the formatter/linter.
| const stateId = !pdu.isState() | ||
| ? event.stateId | ||
| : await this.findPreviousStateId(event.stateId); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard missing stateId on the event before using it.
Avoid passing undefined to state lookups and helper.
- const stateId = !pdu.isState()
- ? event.stateId
- : await this.findPreviousStateId(event.stateId);
+ if (!event.stateId) {
+ this.logger.error({ eventId }, 'state id not found on event');
+ throw new Error(`State id not found for event ${eventId}`);
+ }
+ const stateId = !pdu.isState()
+ ? event.stateId
+ : await this.findPreviousStateId(event.stateId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const stateId = !pdu.isState() | |
| ? event.stateId | |
| : await this.findPreviousStateId(event.stateId); | |
| if (!event.stateId) { | |
| this.logger.error({ eventId }, 'state id not found on event'); | |
| throw new Error(`State id not found for event ${eventId}`); | |
| } | |
| const stateId = !pdu.isState() | |
| ? event.stateId | |
| : await this.findPreviousStateId(event.stateId); |
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/state.service.ts around lines 253 to
256, the code uses event.stateId without ensuring it exists; add a guard so you
never pass undefined into the state lookup/helper: if pdu.isState() keep the
existing await this.findPreviousStateId(event.stateId) only after verifying
event.stateId is defined, and if !pdu.isState() check event.stateId before
assigning stateId (return/throw or set to null/undefined explicitly depending on
callers) so the lookup is only invoked with a valid stateId and callers handle
the missing-state case.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/federation-sdk/src/services/room.service.ts (2)
895-905: Depth sort: guard undefined/NaN and use consistent access.Use the event.event.depth (source of truth here) and guard with a default to avoid NaN comparisons. Also reference the computed aDepth/bDepth in the equality check.
Apply:
- const sortedEvents = Array.from(eventMap.values()).sort((a, b) => { - const aDepth = a.depth; - const bDepth = b.depth; - // If depths are equal, sort by event_id for deterministic ordering - if (a.depth === b.depth) { - return a.eventId.localeCompare(b.eventId); - } - return aDepth - bDepth; - }); + const sortedEvents = Array.from(eventMap.values()).sort((a, b) => { + const aDepth = a.event.depth ?? 0; + const bDepth = b.event.depth ?? 0; + // If depths are equal, sort by event_id for deterministic ordering + if (aDepth === bDepth) { + return a.eventId.localeCompare(b.eventId); + } + return aDepth - bDepth; + });
912-917: Reduce log payload; avoid stringifying full events on hot path.Dumping entire PDUs at info level is noisy and may expose data. Log eventId + depth at debug; keep full JSON behind a feature flag if needed.
- logger.info( - `Persisting state event ${stateEvent.eventId} (depth: ${stateEvent.event.depth}), ${JSON.stringify( - stateEvent.event, - null, - 2, - )}`, - ); + logger.debug( + `Persisting state event ${stateEvent.eventId} (depth: ${stateEvent.event.depth})`, + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/federation-sdk/src/services/room.service.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/room.service.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
event(102-112)
| const copyEvent = (event: Readonly<PersistentEventBase>) => { | ||
| return PersistentEventFactory.createFromRawEvent( | ||
| structuredClone(event.event), | ||
| event.event, | ||
| roomVersion, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Fix room_version skew in copyEvent (use the server-provided version).
copyEvent rebuilds events using the local constant roomVersion ('10'), which can mismatch the actual room_version returned by makeJoinResponse. This risks incorrect canonicalization/hashes/signature handling when re-wrapping remote events.
Apply this diff to align with the negotiated room version:
- return PersistentEventFactory.createFromRawEvent(
- event.event,
- roomVersion,
- );
+ return PersistentEventFactory.createFromRawEvent(
+ event.event,
+ makeJoinResponse.room_version,
+ );Note: Using event.event is OK, but remember the getter may re-compute hashes; if you see subtle hash diffs, consider passing an immutable/raw clone.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const copyEvent = (event: Readonly<PersistentEventBase>) => { | |
| return PersistentEventFactory.createFromRawEvent( | |
| structuredClone(event.event), | |
| event.event, | |
| roomVersion, | |
| ); | |
| }; | |
| const copyEvent = (event: Readonly<PersistentEventBase>) => { | |
| return PersistentEventFactory.createFromRawEvent( | |
| event.event, | |
| makeJoinResponse.room_version, | |
| ); | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/federation-sdk/src/services/profiles.service.ts (2)
141-149: Guard against room/event mismatch and derive room version from the event’s actual room.Using roomId from the request to resolve roomVersion/store can be wrong if the supplied event belongs to a different room. Validate the event and use its room_id for roomVersion/store resolution in both methods.
@@ async getStateIds(roomId: string, eventId: string) - // Get room version for the store - const roomVersion = await this.stateService.getRoomVersion(roomId); - if (!roomVersion) { - throw new Error('Room version not found'); - } + // Validate event-room match and derive effective room + const ev = await this.eventRepository.findById(eventId); + if (!ev) { + throw new Error(`Event ${eventId} not found`); + } + if (ev.event.room_id !== roomId) { + throw new Error(`Event ${eventId} does not belong to room ${roomId}`); + } + const effectiveRoomId = ev.event.room_id; + + // Get room version for the store + const roomVersion = await this.stateService.getRoomVersion(effectiveRoomId); + if (!roomVersion) { + throw new Error(`Room version not found for room ${effectiveRoomId}`); + } @@ async getState(roomId: string, eventId: string) - // Get room version for the store - const roomVersion = await this.stateService.getRoomVersion(roomId); - if (!roomVersion) { - throw new Error('Room version not found'); - } + // Validate event-room match and derive effective room + const ev = await this.eventRepository.findById(eventId); + if (!ev) { + throw new Error(`Event ${eventId} not found`); + } + if (ev.event.room_id !== roomId) { + throw new Error(`Event ${eventId} does not belong to room ${roomId}`); + } + const effectiveRoomId = ev.event.room_id; + + // Get room version for the store + const roomVersion = await this.stateService.getRoomVersion(effectiveRoomId); + if (!roomVersion) { + throw new Error(`Room version not found for room ${effectiveRoomId}`); + }Also applies to: 196-204
179-186: Make eventId optional to match the Matrix federation API and support “current state” when absent.Spec allows event_id to be omitted; return current state in that case. This also reduces coupling to a specific event for /state.
- async getState( - roomId: string, - eventId: string, - ): Promise<{ + async getState( + roomId: string, + eventId?: string, + ): Promise<{ @@ - let state: Map<string, any>; - console.log('eventId', eventId); - - // Get state at a specific event - state = await this.stateService.findStateBeforeEvent(eventId); + let state: Map<string, any>; + this.logger.debug({ roomId, eventId }, 'getState'); + if (eventId) { + state = await this.stateService.findStateBeforeEvent(eventId); + } else { + state = await this.stateService.getFullRoomState(roomId); + }
🧹 Nitpick comments (4)
packages/federation-sdk/src/services/profiles.service.ts (4)
147-149: Avoid reaching into a private/internal API_getStore; expose and use a public accessor.Using a private-ish method is brittle. Prefer adding
getStoreForRoom(roomId: string)orgetStoreForVersion(version: RoomVersion)in StateService and use that here.- const store = this.stateService._getStore(roomVersion); + const store = this.stateService.getStoreForVersion(roomVersion);If
getStoreForVersiondoesn’t exist yet, I can open a follow-up PR to add it.Also applies to: 202-204
154-156: Replace console.log with structured debug logs; don’t dump full event objects.Use the existing logger and log only identifiers to avoid noisy logs and accidental PII leakage.
- console.log('event', event); + this.logger.debug({ eventId: event.eventId }, 'collecting auth chain'); @@ - console.log('eventId', eventId); + this.logger.debug({ roomId, eventId }, 'getState'); @@ - console.log('event ->>>', event); + this.logger.debug({ eventId: event.eventId }, 'collecting auth chain');Also applies to: 188-190, 208-212
151-167: Potentially expensive N×getAuthChain calls; consider a shared-cache approach.Calling getAuthChain for every state event duplicates traversal work. If feasible, extend getAuthChain to accept an external memo/cache or provide a
getAuthChainForEvents(events, store)helper that reuses traversal results across starting points.I can prototype a
getAuthChainForEventsutility that memoizes per-event chains and unions them efficiently.Also applies to: 205-221
5-12: Clean up unused imports (if any).If
Stateisn’t used here, drop it to keep the surface minimal and avoid linter noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/federation-sdk/src/services/profiles.service.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/profiles.service.ts (3)
packages/room/src/manager/event-wrapper.ts (2)
roomId(72-74)event(102-112)packages/room/src/manager/v3.ts (1)
eventId(21-33)packages/room/src/state_resolution/definitions/definitions.ts (1)
getAuthChain(91-137)
| async getStateIds( | ||
| roomId: string, | ||
| eventId: string, | ||
| ): Promise<{ pdu_ids: string[]; auth_chain_ids: string[] }> { | ||
| try { | ||
| const state = await this.stateService.findStateBeforeEvent(eventId); | ||
|
|
||
| const pduIds: string[] = []; | ||
| const authChainIds = new Set<string>(); | ||
|
|
||
| // Get room version for the store | ||
| const roomVersion = await this.stateService.getRoomVersion(roomId); | ||
| if (!roomVersion) { | ||
| throw new Error('Room version not found'); | ||
| } | ||
|
|
||
| // Get the event store | ||
| const store = this.stateService._getStore(roomVersion); | ||
|
|
||
| // Extract state event IDs and collect auth chain IDs | ||
| for (const [, event] of state.entries()) { | ||
| // PersistentEventBase has an eventId getter | ||
| pduIds.push(event.eventId); | ||
| console.log('event', event); | ||
| // Get the complete auth chain for this event | ||
| try { | ||
| const authChain = await getAuthChain(event, store); | ||
| for (const authEventId of authChain) { | ||
| authChainIds.add(authEventId); | ||
| } | ||
| } catch (error) { | ||
| this.logger.warn( | ||
| `Failed to get auth chain for event ${event.eventId}:`, | ||
| error, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| pdu_ids: pduIds, | ||
| auth_chain_ids: Array.from(authChainIds), | ||
| }; | ||
| } catch (error) { | ||
| this.logger.error(`Failed to get state IDs for room ${roomId}:`, error); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add tests for state_ids and state endpoints (happy-path + guards).
Codecov shows 0% patch coverage on these additions. Add unit/integration tests covering: valid event in room, room/event mismatch (400/404), missing event (404), no event_id (current state), and non-empty auth_chain.
Run to find callsites and align controller/DTO expectations:
Also applies to: 179-244
🏁 Script executed:
#!/bin/bash
# Validate service usage and optional event_id in controllers/DTOs
rg -nC2 "getStateIds\(|getState\(" --type ts
rg -nC2 "GET /_matrix/federation/v1/state_ids|GET /_matrix/federation/v1/state" --type ts
rg -nP "event_id\??:\s*" --type tsLength of output: 10421
Add tests for /state and /state_ids; align optional event_id across controller/DTO/service
Codecov shows 0% patch coverage for these changes.
- Add unit/integration tests (happy-path + guards): valid event in room (200), room/event mismatch (400/404), missing event (404), no event_id (current state), and non-empty auth_chain.
- Findings (callsites / mismatches):
- packages/homeserver/src/controllers/federation/profiles.controller.ts — lines ~142 and ~160 call profilesService.getStateIds(params.roomId, query.event_id) and profilesService.getState(params.roomId, query.event_id!).
- packages/homeserver/src/dtos/federation/state.dto.ts and packages/homeserver/src/dtos/federation/state-ids.dto.ts declare event_id as required (t.String).
- packages/federation-sdk/src/specs/federation-api.ts and packages/federation-sdk/src/utils/event-schemas.ts mark event_id optional.
- packages/federation-sdk/src/services/profiles.service.ts methods currently accept eventId: string (non-optional) and contain a stray console.log('event', event).
- Required actions (recommended):
- Make event_id consistent across the stack. Recommended approach: make homeserver DTOs accept an optional event_id, change controller/service signatures to eventId?: string, and implement handling for undefined (treat as current state). Alternatively enforce presence at controller if you prefer required semantics — but pick one and align DTO + controller + service.
- Add the tests above in homeserver (controller-level) and federation-sdk (service-level) to bring patch coverage up and validate guards.
- Remove the debug console.log in packages/federation-sdk/src/services/profiles.service.ts.
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/profiles.service.ts around lines
131–177, the method signature and behavior treat eventId as required and include
a stray console.log; align event_id optionality across DTOs/controllers/services
and remove the debug log. Change the method signature to accept eventId?:
string, handle undefined by treating it as "current state" when calling
stateService.findStateBeforeEvent/getState (or call the appropriate
current-state helper), remove console.log('event', event), and ensure
callers/controllers and homeserver DTOs mark event_id optional (or vice versa if
you choose required semantics) so the types/signatures match; finally, add
unit/integration tests for /state and /state_ids covering happy path, room/event
mismatch, missing event, no event_id (current state), and non-empty auth_chain
to restore coverage.
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 (4)
packages/room/src/state_resolution/definitions/definitions.ts (4)
104-132: Cache the computed auth chain before returning (missing write).You only cache leaf results (no auth events). For non-leaf events, the result isn’t cached, causing repeated graph walks and extra I/O.
Apply:
for (const authEvent of authEvents) { console.log('authEvent ->>>', authEvent); const nextAuthChainPart = await _getAuthChain( authEvent, newAuthChainPart, ); if (!nextAuthChainPart) { continue; } newAuthChainPart = newAuthChainPart.union(nextAuthChainPart); } - return newAuthChainPart; + eventIdToAuthChainMap.set(eventId, newAuthChainPart); + return newAuthChainPart;
269-271: PriorityQueue API misuse: use dequeue/enqueue, not pop/push.
@datastructures-js/priority-queueexposesenqueue/dequeue. Usingpop/pushwill throw at runtime.Apply:
- const node = zeroIndegreeQueue.pop(); + const node = zeroIndegreeQueue.dequeue(); - zeroIndegreeQueue.push(neighbour); + zeroIndegreeQueue.enqueue(neighbour);Also applies to: 289-291
356-357: Missing await in recursive graph build (race/incomplete graph).The async recursion isn’t awaited, so the graph may be incomplete before ordering.
Apply:
- buildIndegreeGraph(graph, authEvent); + await buildIndegreeGraph(graph, authEvent);
523-539: Comparator truthiness bug: position 0 is treated as falsy.Events at mainline position 0 are skipped from the primary comparison, skewing order.
Apply:
- const e1Position = mainlinePositions.get(e1.eventId); - const e2Position = mainlinePositions.get(e2.eventId); - if (e1Position && e2Position && e1Position < e2Position) return -1; + const e1Position = mainlinePositions.get(e1.eventId); + const e2Position = mainlinePositions.get(e2.eventId); + if (e1Position !== undefined && e2Position !== undefined && e1Position !== e2Position) { + return e1Position - e2Position; // smaller (older) first + }
🧹 Nitpick comments (2)
packages/room/src/state_resolution/definitions/definitions.ts (2)
109-110: Remove or gate console logs (PII/noise).Raw
console.login core resolution leaks event contents and floods logs. Use a logger or guard behind a debug flag.Apply one:
- console.log('authEvents ->>>', authEvents); + // if (process.env.DEBUG_AUTH_CHAIN) console.debug('authEvents', authEvents.map(e => e.eventId)); - console.log('authEvent ->>>', authEvent); + // if (process.env.DEBUG_AUTH_CHAIN) console.debug('authEvent', authEvent.eventId);Also applies to: 120-121
91-98: Follow-up: separate accumulator from “visited” if you need cycle prevention.If you intended the seed to act as a visited set, keep a distinct
visited: Set<EventID>and still return the accumulator without E.Example:
- const _getAuthChain = async (event, acc) => { + const _getAuthChain = async (event, acc, visited = new Set<EventID>()) => { const eventId = event.eventId; + if (visited.has(eventId)) return acc; + visited.add(eventId); ... - const nextAuthChainPart = await _getAuthChain(authEvent, newAuthChainPart); + const nextAuthChainPart = await _getAuthChain(authEvent, newAuthChainPart, visited); ... - return _getAuthChain(event, new Set()); + return _getAuthChain(event, new Set());Also applies to: 134-135
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/room/src/state_resolution/definitions/definitions.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/room/src/state_resolution/definitions/definitions.ts (2)
packages/room/src/manager/v3.ts (1)
eventId(21-33)packages/room/src/manager/event-wrapper.ts (1)
event(102-112)
🔇 Additional comments (1)
packages/room/src/state_resolution/definitions/definitions.ts (1)
117-129: Verify Set.prototype.union/intersection/difference exist (or avoid relying on global augmentation)Native JS Set has no union/intersection/difference. Repository search was inconclusive/no evidence of a global augmentation or helper; ensure a polyfill/augmentation is imported/initialized before use at packages/room/src/state_resolution/definitions/definitions.ts:117–129 and 163–177, or replace the calls with explicit helpers (e.g., new Set([...a, ...b]) or a shared utility function).
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 (2)
packages/room/src/state_resolution/definitions/definitions.ts (2)
98-116: Caching bug: context-polluted memoization returns incorrect chains.
existingAuthChainPartis path-dependent and is stored/returned from the per-event cache, causing unrelated IDs (including the original seed) to contaminate results across branches. Cache per event must represent only “auth events reachable from that event” (context-free), and recursion should build a fresh set per event. Also set the cache for all events, not only leaves.Apply this rewrite for the helper and caller:
export async function getAuthChain( event: PersistentEventBase, store: EventStore, ): Promise<Set<EventID>> { - // TODO: central cache for t6his - const eventIdToAuthChainMap = new Map<EventID, Set<EventID>>(); // do not repeat - - const _getAuthChain = async ( - event: PersistentEventBase, - existingAuthChainPart: Set<EventID>, - ) => { - const eventId = event.eventId; - - if (eventIdToAuthChainMap.has(eventId)) { - return eventIdToAuthChainMap.get(eventId)!; - } - - const authEvents = await event.getAuthorizationEvents(store); - if (authEvents.length === 0) { - eventIdToAuthChainMap.set(eventId, existingAuthChainPart); - return existingAuthChainPart; - } - - const authEventIdsSet = new Set(authEvents.map((e) => e.eventId)); - - let newAuthChainPart = existingAuthChainPart.union(authEventIdsSet); - - for (const authEvent of authEvents) { - const nextAuthChainPart = await _getAuthChain( - authEvent, - newAuthChainPart, - ); - if (!nextAuthChainPart) { - continue; - } - newAuthChainPart = newAuthChainPart.union(nextAuthChainPart); - } - - return newAuthChainPart; - }; - - return _getAuthChain(event, new Set([event.eventId])); + // TODO: central cache for this + const memo = new Map<EventID, Set<EventID>>(); + const inFlight = new Map<EventID, Promise<Set<EventID>>>(); + + const _getAuthChain = async (ev: PersistentEventBase): Promise<Set<EventID>> => { + const id = ev.eventId; + if (memo.has(id)) return memo.get(id)!; + if (inFlight.has(id)) return inFlight.get(id)!; + + const promise = (async () => { + const chain = new Set<EventID>(); + // pre-set to break potential cycles and allow re-entrancy + memo.set(id, chain); + + const authEvents = await ev.getAuthorizationEvents(store); + for (const a of authEvents) { + chain.add(a.eventId); // include direct auth event + const sub = await _getAuthChain(a); // recurse + for (const eId of sub) chain.add(eId); + } + + return chain; + })(); + + inFlight.set(id, promise); + try { + const result = await promise; + return result; + } finally { + inFlight.delete(id); + } + }; + + return _getAuthChain(event); }Also applies to: 104-106, 108-114, 116-127, 129-131
352-356: Async recursion is not awaited; graph may be incomplete.
buildIndegreeGraphisasync, but the recursive call is not awaited, risking partial graph construction and unhandled promise rejections.Apply:
- buildIndegreeGraph(graph, authEvent); + await buildIndegreeGraph(graph, authEvent);
♻️ Duplicate comments (4)
packages/federation-sdk/src/services/profiles.service.ts (3)
135-149: Validate event existence/room match and derive roomVersion from the event’s room.Avoid computing roomVersion from the request roomId; validate the event and use ev.event.room_id so store/auth chain are correct.
Apply:
- try { - const state = await this.stateService.findStateBeforeEvent(eventId); + try { + const ev = await this.eventRepository.findById(eventId); + if (!ev) { + throw new Error(`Event ${eventId} not found`); + } + if (ev.event.room_id !== roomId) { + throw new Error(`event ${eventId} does not belong to room ${roomId}`); + } + + const state = await this.stateService.findStateBeforeEvent(eventId); @@ - // Get room version for the store - const roomVersion = await this.stateService.getRoomVersion(roomId); + // Get room version for the store (from the event's room) + const roomVersion = await this.stateService.getRoomVersion(ev.event.room_id);
179-201: Support optional eventId, add guards, remove console.log, and derive roomVersion from effective room.Aligns with callers and spec (“prior to considering any state changes induced by the requested event”). Logs via logger.
- async getState( - roomId: string, - eventId: string, + async getState( + roomId: string, + eventId?: string, ): Promise<{ @@ - try { - let state: Map<string, any>; - console.log('eventId', eventId); - - // Get state at a specific event - state = await this.stateService.findStateBeforeEvent(eventId); + try { + this.logger.debug({ roomId, eventId }, 'getState'); + let state: State; + let effectiveRoomId = roomId; + + if (eventId) { + const ev = await this.eventRepository.findById(eventId); + if (!ev) { + throw new Error(`Event ${eventId} not found`); + } + if (ev.event.room_id !== roomId) { + throw new Error(`event ${eventId} does not belong to room ${roomId}`); + } + effectiveRoomId = ev.event.room_id; + // State prior to the given event + state = await this.stateService.findStateBeforeEvent(eventId); + } else { + // Current room state + state = await this.stateService.getFullRoomState(roomId); + } @@ - const roomVersion = await this.stateService.getRoomVersion(roomId); + const roomVersion = await this.stateService.getRoomVersion(effectiveRoomId);Follow-up: Please confirm controllers/DTOs accept optional event_id to match this signature, or prefer making it required everywhere.
151-160: Exclude the state event itself from auth_chain and drop console.log.getAuthChain currently includes the seed event; remove it from the chain. Replace console.log with logger.
for (const [, event] of state.entries()) { // PersistentEventBase has an eventId getter pduIds.push(event.eventId); - console.log('event', event); // Get the complete auth chain for this event try { - const authChain = await getAuthChain(event, store); + const authChain = await getAuthChain(event, store); + authChain.delete(event.eventId); // do not include the state event itself for (const authEventId of authChain) { authChainIds.add(authEventId); }packages/room/src/state_resolution/definitions/definitions.ts (1)
132-133: Auth chain must NOT include the event itself; seed must be empty.Seeding
_getAuthChainwithevent.eventIdleaks the event into its own auth chain and breaks auth-diff/state logic. The auth chain of E consists of E’s auth events (and their auth events), not E. [Matrix/Synapse docs define the auth chain as “events’ auth events and their auth events, recursively”.] (matrix-org.github.io)Apply:
- return _getAuthChain(event, new Set([event.eventId])); + return _getAuthChain(event);
🧹 Nitpick comments (6)
packages/federation-sdk/src/services/profiles.service.ts (6)
150-167: Avoid recomputing auth chains per state event (cache locally).Minor perf: memoize per eventId to prevent repeated traversal across overlapping chains.
// before the for-of loop const authChainCache = new Map<string, Set<string>>(); // inside the loop, replace getAuthChain call with: let authChain = authChainCache.get(event.eventId); if (!authChain) { authChain = await getAuthChain(event, store); authChainCache.set(event.eventId, authChain); } authChain.delete(event.eventId); for (const id of authChain) authChainIds.add(id);
169-172: Return deterministic ordering.Stable, sorted arrays reduce diff noise and help callers cache.
- return { - pdu_ids: pduIds, - auth_chain_ids: Array.from(authChainIds), - }; + return { + pdu_ids: pduIds.sort(), + auth_chain_ids: Array.from(authChainIds).sort(), + };
174-175: Improve error context.Include eventId and structure for better logging.
- } catch (error) { - this.logger.error(`Failed to get state IDs for room ${roomId}:`, error); + } catch (error) { + this.logger.error({ roomId, eventId, err: error }, 'Failed to get state IDs');
203-204: Avoid relying on an underscored method.Expose a public accessor (e.g., getStore(roomVersion)) on StateService instead of using _getStore.
226-229: Deterministic auth_chain ordering.Sort IDs before fetching for stable output order.
- const authEvents = await store.getEvents(Array.from(authChainIds)); + const authEvents = await store.getEvents(Array.from(authChainIds).sort());
239-241: Improve error context (state endpoint).Add eventId and structure.
- } catch (error) { - this.logger.error(`Failed to get state for room ${roomId}:`, error); + } catch (error) { + this.logger.error({ roomId, eventId, err: error }, 'Failed to get state');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/federation-sdk/src/services/profiles.service.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(3 hunks)packages/room/src/state_resolution/definitions/definitions.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/federation-sdk/src/services/state.service.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/profiles.service.ts (3)
packages/room/src/manager/event-wrapper.ts (2)
roomId(72-74)event(102-112)packages/room/src/manager/v3.ts (1)
eventId(21-33)packages/room/src/state_resolution/definitions/definitions.ts (1)
getAuthChain(91-133)
🔇 Additional comments (4)
packages/federation-sdk/src/services/profiles.service.ts (1)
5-12: Imports look good.Re-exports from @hs/room (incl. getAuthChain) are used below as expected.
packages/room/src/state_resolution/definitions/definitions.ts (3)
95-97: Nit: fix typo in comment.
[ suggest_nitpick ]
Apply:- // TODO: central cache for t6his + // TODO: central cache for this
116-127: Confirm Node ≥22 or polyfill Set.prototype.union/intersection/differencepackages/room/src/state_resolution/definitions/definitions.ts (lines 116–127) calls Set.prototype.union. Repo check: package.json files do not declare engines.node and no .nvmrc found; search for polyfill references was inconclusive in the sandbox.
Actions:
- Ensure runtime (dev/CI/prod) runs Node.js ≥ 22, or
- Add a polyfill (e.g., set.prototype.union / core-js), or
- Replace Set.prototype.union calls with manual Set merging.
256-269: Confirmed: enqueue()/pop() aliases supported by pinned versionpackages/room/package.json pins @datastructures-js/priority-queue@^6.3.3; v6 README documents .enqueue and .pop (dequeue) — mixing enqueue() and pop() is safe. (app.unpkg.com)
| for (const [, event] of state.entries()) { | ||
| // PersistentEventBase has an event getter that contains the actual event data | ||
| pdus.push(event.event); | ||
| // Get the complete auth chain for this event | ||
| try { | ||
| const authChain = await getAuthChain(event, store); | ||
| for (const authEventId of authChain) { | ||
| authChainIds.add(authEventId); | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Exclude the state event itself from auth_chain here as well.
Consistency with state_ids.
for (const [, event] of state.entries()) {
// PersistentEventBase has an event getter that contains the actual event data
pdus.push(event.event);
// Get the complete auth chain for this event
try {
- const authChain = await getAuthChain(event, store);
+ const authChain = await getAuthChain(event, store);
+ authChain.delete(event.eventId);
for (const authEventId of authChain) {
authChainIds.add(authEventId);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const [, event] of state.entries()) { | |
| // PersistentEventBase has an event getter that contains the actual event data | |
| pdus.push(event.event); | |
| // Get the complete auth chain for this event | |
| try { | |
| const authChain = await getAuthChain(event, store); | |
| for (const authEventId of authChain) { | |
| authChainIds.add(authEventId); | |
| } | |
| } catch (error) { | |
| for (const [, event] of state.entries()) { | |
| // PersistentEventBase has an event getter that contains the actual event data | |
| pdus.push(event.event); | |
| // Get the complete auth chain for this event | |
| try { | |
| const authChain = await getAuthChain(event, store); | |
| authChain.delete(event.eventId); | |
| for (const authEventId of authChain) { | |
| authChainIds.add(authEventId); | |
| } | |
| } catch (error) { |
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/profiles.service.ts around lines 205 to
214, the auth_chain collected for each state event currently includes the state
event itself; to match state_ids consistency, filter out the state's own event
id before adding items to authChainIds. After await getAuthChain(event, store),
remove event.event.event_id from the returned authChain (or use the event id
property present on the event object) and only add the remaining ids to
authChainIds.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
packages/homeserver/src/controllers/federation/event.controller.ts (1)
36-41: Drop the non-null assertion on query.event_id
GetStateQueryDtoalready requiresevent_id; the!is unnecessary and masks potential typing issues.- ({ params, query }) => - eventService.getState(params.roomId, query.event_id!), + ({ params, query }) => + eventService.getState(params.roomId, query.event_id),packages/federation-sdk/src/services/event.service.ts (1)
722-725: Optional: deterministic ordering of resultsIf clients or tests rely on stable ordering, sort
pdu_idsandauth_chain_ids, and consider sortingauth_chainbyevent_idbefore returning.- return { - pdu_ids: pduIds, - auth_chain_ids: Array.from(authChainIds), - }; + return { + pdu_ids: [...pduIds], // or pduIds.sort() + auth_chain_ids: [...authChainIds], // or [...authChainIds].sort() + };Also applies to: 788-791
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/federation-sdk/src/services/event.service.ts(2 hunks)packages/homeserver/src/controllers/federation/event.controller.ts(1 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(2 hunks)packages/homeserver/src/homeserver.module.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/homeserver/src/controllers/federation/profiles.controller.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/homeserver/src/homeserver.module.ts (1)
packages/homeserver/src/controllers/federation/event.controller.ts (1)
eventPlugin(14-54)
packages/homeserver/src/controllers/federation/event.controller.ts (4)
packages/federation-sdk/src/index.ts (1)
EventService(49-49)packages/homeserver/src/dtos/federation/state-ids.dto.ts (3)
GetStateIdsParamsDto(4-6)GetStateIdsQueryDto(8-10)GetStateIdsResponseDto(12-17)packages/homeserver/src/dtos/common/error.dto.ts (1)
ErrorResponseDto(3-6)packages/homeserver/src/dtos/federation/state.dto.ts (3)
GetStateParamsDto(4-6)GetStateQueryDto(8-10)GetStateResponseDto(12-19)
packages/federation-sdk/src/services/event.service.ts (3)
packages/room/src/manager/event-wrapper.ts (2)
roomId(72-74)event(102-112)packages/room/src/manager/v3.ts (1)
eventId(21-33)packages/room/src/state_resolution/definitions/definitions.ts (1)
getAuthChain(91-133)
| async getStateIds( | ||
| roomId: string, | ||
| eventId: string, | ||
| ): Promise<{ pdu_ids: string[]; auth_chain_ids: string[] }> { | ||
| try { | ||
| const state = await this.stateService.findStateBeforeEvent(eventId); | ||
|
|
||
| const pduIds: string[] = []; | ||
| const authChainIds = new Set<string>(); | ||
|
|
||
| // Get room version for the store | ||
| const roomVersion = await this.stateService.getRoomVersion(roomId); | ||
| if (!roomVersion) { | ||
| throw new Error('Room version not found'); | ||
| } | ||
|
|
||
| // Get the event store | ||
| const store = this.stateService._getStore(roomVersion); | ||
|
|
||
| // Extract state event IDs and collect auth chain IDs | ||
| for (const [, event] of state.entries()) { | ||
| // PersistentEventBase has an eventId getter | ||
| pduIds.push(event.eventId); | ||
| console.log('event', event); | ||
| // Get the complete auth chain for this event | ||
| try { | ||
| const authChain = await getAuthChain(event, store); | ||
| for (const authEventId of authChain) { | ||
| authChainIds.add(authEventId); | ||
| } | ||
| } catch (error) { | ||
| this.logger.warn( | ||
| `Failed to get auth chain for event ${event.eventId}:`, | ||
| error, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| pdu_ids: pduIds, | ||
| auth_chain_ids: Array.from(authChainIds), | ||
| }; | ||
| } catch (error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate event-room match; exclude PDUs from auth chain; avoid recomputing chains
- Ensure
eventIdbelongs toroomIdto prevent cross-room state leakage. getAuthChainincludes the seed event ID; spec-wise, PDUs should not be duplicated inauth_chain_ids.- Cache per-event auth chains during the loop to avoid O(N×M) recomputation.
async getStateIds(
roomId: string,
eventId: string,
): Promise<{ pdu_ids: string[]; auth_chain_ids: string[] }> {
try {
+ // Ensure the referenced event belongs to the requested room
+ const seed = await this.eventRepository.findById(eventId);
+ if (!seed || seed.event.room_id !== roomId) {
+ throw new Error('M_EVENT_ROOM_MISMATCH');
+ }
const state = await this.stateService.findStateBeforeEvent(eventId);
const pduIds: string[] = [];
const authChainIds = new Set<string>();
+ const authChainCache = new Map<string, Set<string>>();
// Get room version for the store
const roomVersion = await this.stateService.getRoomVersion(roomId);
if (!roomVersion) {
throw new Error('Room version not found');
}
// Get the event store
const store = this.stateService._getStore(roomVersion);
// Extract state event IDs and collect auth chain IDs
for (const [, event] of state.entries()) {
- // PersistentEventBase has an eventId getter
- pduIds.push(event.eventId);
- console.log('event', event);
+ const eId = event.eventId;
+ pduIds.push(eId);
// Get the complete auth chain for this event
try {
- const authChain = await getAuthChain(event, store);
- for (const authEventId of authChain) {
+ let authChain = authChainCache.get(eId);
+ if (!authChain) {
+ authChain = await getAuthChain(event, store);
+ authChainCache.set(eId, authChain);
+ }
+ // Exclude the state event itself from the auth chain
+ authChain.delete(eId);
+ for (const authEventId of authChain) {
authChainIds.add(authEventId);
}
} catch (error) {
this.logger.warn(
`Failed to get auth chain for event ${event.eventId}:`,
error,
);
}
}
- return {
- pdu_ids: pduIds,
- auth_chain_ids: Array.from(authChainIds),
- };
+ // Optional: sort for deterministic responses
+ return {
+ pdu_ids: pduIds, // or pduIds.sort()
+ auth_chain_ids: Array.from(authChainIds), // or [...authChainIds].sort()
+ };
} catch (error) {
this.logger.error(`Failed to get state IDs for room ${roomId}:`, error);
throw error;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getStateIds( | |
| roomId: string, | |
| eventId: string, | |
| ): Promise<{ pdu_ids: string[]; auth_chain_ids: string[] }> { | |
| try { | |
| const state = await this.stateService.findStateBeforeEvent(eventId); | |
| const pduIds: string[] = []; | |
| const authChainIds = new Set<string>(); | |
| // Get room version for the store | |
| const roomVersion = await this.stateService.getRoomVersion(roomId); | |
| if (!roomVersion) { | |
| throw new Error('Room version not found'); | |
| } | |
| // Get the event store | |
| const store = this.stateService._getStore(roomVersion); | |
| // Extract state event IDs and collect auth chain IDs | |
| for (const [, event] of state.entries()) { | |
| // PersistentEventBase has an eventId getter | |
| pduIds.push(event.eventId); | |
| console.log('event', event); | |
| // Get the complete auth chain for this event | |
| try { | |
| const authChain = await getAuthChain(event, store); | |
| for (const authEventId of authChain) { | |
| authChainIds.add(authEventId); | |
| } | |
| } catch (error) { | |
| this.logger.warn( | |
| `Failed to get auth chain for event ${event.eventId}:`, | |
| error, | |
| ); | |
| } | |
| } | |
| return { | |
| pdu_ids: pduIds, | |
| auth_chain_ids: Array.from(authChainIds), | |
| }; | |
| } catch (error) { | |
| async getStateIds( | |
| roomId: string, | |
| eventId: string, | |
| ): Promise<{ pdu_ids: string[]; auth_chain_ids: string[] }> { | |
| try { | |
| // Ensure the referenced event belongs to the requested room | |
| const seed = await this.eventRepository.findById(eventId); | |
| if (!seed || seed.event.room_id !== roomId) { | |
| throw new Error('M_EVENT_ROOM_MISMATCH'); | |
| } | |
| const state = await this.stateService.findStateBeforeEvent(eventId); | |
| const pduIds: string[] = []; | |
| const authChainIds = new Set<string>(); | |
| const authChainCache = new Map<string, Set<string>>(); | |
| // Get room version for the store | |
| const roomVersion = await this.stateService.getRoomVersion(roomId); | |
| if (!roomVersion) { | |
| throw new Error('Room version not found'); | |
| } | |
| // Get the event store | |
| const store = this.stateService._getStore(roomVersion); | |
| // Extract state event IDs and collect auth chain IDs | |
| for (const [, event] of state.entries()) { | |
| const eId = event.eventId; | |
| pduIds.push(eId); | |
| // Get the complete auth chain for this event | |
| try { | |
| let authChain = authChainCache.get(eId); | |
| if (!authChain) { | |
| authChain = await getAuthChain(event, store); | |
| authChainCache.set(eId, authChain); | |
| } | |
| // Exclude the state event itself from the auth chain | |
| authChain.delete(eId); | |
| for (const authEventId of authChain) { | |
| authChainIds.add(authEventId); | |
| } | |
| } catch (error) { | |
| this.logger.warn( | |
| `Failed to get auth chain for event ${event.eventId}:`, | |
| error, | |
| ); | |
| } | |
| } | |
| // Optional: sort for deterministic responses | |
| return { | |
| pdu_ids: pduIds, // or pduIds.sort() | |
| auth_chain_ids: Array.from(authChainIds), // or [...authChainIds].sort() | |
| }; | |
| } catch (error) { | |
| this.logger.error(`Failed to get state IDs for room ${roomId}:`, error); | |
| throw error; | |
| } | |
| } |
| const store = this.stateService._getStore(roomVersion); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid calling a private API (_getStore)
StateService._getStore looks private by convention. Expose a public getter (e.g., getStoreForRoomVersion(version: string)) and use that instead, to prevent future breakage.
I can open a follow-up PR to add getStoreForRoomVersion to StateService and refactor both call sites.
Also applies to: 756-757
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/event.service.ts around lines 701-702
(and also lines 756-757), the code calls the private API
this.stateService._getStore(roomVersion); replace that call with a new public
method on StateService (suggested name: getStoreForRoomVersion(version: string))
and call this.stateService.getStoreForRoomVersion(roomVersion) instead; add the
new public method to StateService which wraps the existing private logic and
preserves return types and behavior, export it with proper typing and
visibility, update both call sites to use the public getter, and remove any
direct uses of the underscore-prefixed private method to avoid future breakage.
| pduIds.push(event.eventId); | ||
| console.log('event', event); | ||
| // Get the complete auth chain for this event |
There was a problem hiding this comment.
Remove console logging of events and IDs (PII/noise)
console.log('event', event) and console.log('eventId', eventId) may leak PII and generate excessive logs in production. Use the class logger at debug with redaction, or remove.
- console.log('event', event);
+ // this.logger.debug({ msg: 'state event', eventId: event.eventId });
- console.log('eventId', eventId);
+ // this.logger.debug({ msg: 'getState', eventId });Also applies to: 740-742
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/event.service.ts around lines 706-708
(and also 740-742), remove the raw console.log calls that print full event
objects and eventIds (which can leak PII and create noisy production logs);
replace them with the class logger at debug level and ensure sensitive fields
are redacted before logging (or omit logging the full event entirely and only
log non-sensitive identifiers), e.g., call the instance logger.debug with a
minimal, redacted payload or drop the log statements if not needed.
| async getState( | ||
| roomId: string, | ||
| eventId: string, | ||
| ): Promise<{ | ||
| pdus: Record<string, unknown>[]; | ||
| auth_chain: Record<string, unknown>[]; | ||
| }> { | ||
| try { | ||
| let state: Map<string, any>; | ||
| console.log('eventId', eventId); | ||
|
|
||
| // Get state at a specific event | ||
| state = await this.stateService.findStateBeforeEvent(eventId); | ||
|
|
||
| const pdus: Record<string, unknown>[] = []; | ||
| const authChainIds = new Set<string>(); | ||
|
|
||
| // Get room version for the store | ||
| const roomVersion = await this.stateService.getRoomVersion(roomId); | ||
| if (!roomVersion) { | ||
| throw new Error('Room version not found'); | ||
| } | ||
|
|
||
| // Get the event store | ||
| const store = this.stateService._getStore(roomVersion); | ||
| // Extract state event objects and collect auth chain IDs | ||
| for (const [, event] of state.entries()) { | ||
| // PersistentEventBase has an event getter that contains the actual event data | ||
| pdus.push(event.event); | ||
| // Get the complete auth chain for this event | ||
| try { | ||
| const authChain = await getAuthChain(event, store); | ||
| for (const authEventId of authChain) { | ||
| authChainIds.add(authEventId); | ||
| } | ||
| } catch (error) { | ||
| this.logger.warn( | ||
| `Failed to get auth chain for event ${event.eventId}:`, | ||
| error, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Fetch the actual auth event objects | ||
| const authChain: Record<string, unknown>[] = []; | ||
| if (authChainIds.size > 0) { | ||
| try { | ||
| const authEvents = await store.getEvents(Array.from(authChainIds)); | ||
| for (const authEvent of authEvents) { | ||
| authChain.push(authEvent.event); | ||
| } | ||
| } catch (error) { | ||
| this.logger.warn('Failed to fetch auth event objects:', error); | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| pdus: pdus, | ||
| auth_chain: authChain, | ||
| }; | ||
| } catch (error) { | ||
| this.logger.error(`Failed to get state for room ${roomId}:`, error); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same fixes for /state: validate room, drop logs, cache chains, and exclude PDUs from auth_chain
Mirror the safeguards and caching from getStateIds. Also guard against nulls from store.getEvents.
async getState(
roomId: string,
eventId: string,
): Promise<{
pdus: Record<string, unknown>[];
auth_chain: Record<string, unknown>[];
}> {
try {
- let state: Map<string, any>;
- console.log('eventId', eventId);
+ let state: Map<string, any>;
+ // Ensure the referenced event belongs to the requested room
+ const seed = await this.eventRepository.findById(eventId);
+ if (!seed || seed.event.room_id !== roomId) {
+ throw new Error('M_EVENT_ROOM_MISMATCH');
+ }
// Get state at a specific event
state = await this.stateService.findStateBeforeEvent(eventId);
const pdus: Record<string, unknown>[] = [];
const authChainIds = new Set<string>();
+ const authChainCache = new Map<string, Set<string>>();
+ const stateEventIds = new Set<string>();
// Get room version for the store
const roomVersion = await this.stateService.getRoomVersion(roomId);
if (!roomVersion) {
throw new Error('Room version not found');
}
// Get the event store
const store = this.stateService._getStore(roomVersion);
// Extract state event objects and collect auth chain IDs
for (const [, event] of state.entries()) {
- // PersistentEventBase has an event getter that contains the actual event data
- pdus.push(event.event);
+ const eId = event.eventId;
+ stateEventIds.add(eId);
+ pdus.push(event.event);
// Get the complete auth chain for this event
try {
- const authChain = await getAuthChain(event, store);
- for (const authEventId of authChain) {
+ let authChain = authChainCache.get(eId);
+ if (!authChain) {
+ authChain = await getAuthChain(event, store);
+ authChainCache.set(eId, authChain);
+ }
+ // Exclude the state event itself from the auth chain
+ authChain.delete(eId);
+ for (const authEventId of authChain) {
authChainIds.add(authEventId);
}
} catch (error) {
this.logger.warn(
`Failed to get auth chain for event ${event.eventId}:`,
error,
);
}
}
// Fetch the actual auth event objects
const authChain: Record<string, unknown>[] = [];
if (authChainIds.size > 0) {
try {
- const authEvents = await store.getEvents(Array.from(authChainIds));
- for (const authEvent of authEvents) {
- authChain.push(authEvent.event);
+ const authEvents = await store.getEvents(Array.from(authChainIds));
+ for (const authEvent of authEvents) {
+ if (authEvent) {
+ authChain.push(authEvent.event);
+ }
}
} catch (error) {
this.logger.warn('Failed to fetch auth event objects:', error);
}
}
return {
pdus: pdus,
auth_chain: authChain,
};
} catch (error) {
this.logger.error(`Failed to get state for room ${roomId}:`, error);
throw error;
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/event.service.ts around lines 732 to
796, mirror the safeguards from getStateIds: remove the stray console.log,
validate the room exists before proceeding (throw or return if missing), and
reuse a cached auth-chain map/set so you don't repeatedly compute chains for the
same event; when building auth_chain ensure you exclude PDUs already included in
pdus and guard against null/undefined returns from store.getEvents (skip or log
and continue if getEvents returns null/undefined or missing entries). Also
change loose logger.warn/error calls to provide contextual messages and avoid
noisy logs for expected missing data.
| .get( | ||
| '/_matrix/federation/v1/state_ids/:roomId', | ||
| ({ params, query }) => | ||
| eventService.getStateIds(params.roomId, query.event_id), | ||
| { | ||
| params: GetStateIdsParamsDto, | ||
| query: GetStateIdsQueryDto, | ||
| response: { | ||
| 200: GetStateIdsResponseDto, | ||
| 400: ErrorResponseDto, | ||
| }, | ||
| detail: { | ||
| tags: ['Federation'], | ||
| summary: 'Get state IDs', | ||
| description: 'Get state event IDs for a room', | ||
| }, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Map service errors to the declared 400 schema
Handlers currently throw, which Elysia will surface as 500s. Since the route advertises a 400 ErrorResponseDto, catch known errors and set set.status = 400.
- ({ params, query }) =>
- eventService.getStateIds(params.roomId, query.event_id),
+ async ({ params, query, set }) => {
+ try {
+ return await eventService.getStateIds(params.roomId, query.event_id);
+ } catch (e) {
+ set.status = 400;
+ return { error: 'Bad Request', details: e instanceof Error ? e.message : e };
+ }
+ },
- ({ params, query }) =>
- eventService.getState(params.roomId, query.event_id),
+ async ({ params, query, set }) => {
+ try {
+ return await eventService.getState(params.roomId, query.event_id);
+ } catch (e) {
+ set.status = 400;
+ return { error: 'Bad Request', details: e instanceof Error ? e.message : e };
+ }
+ },Also applies to: 36-53
🤖 Prompt for AI Agents
In packages/homeserver/src/controllers/federation/event.controller.ts around
lines 18-35 (and similarly 36-53), the route handlers directly throw service
errors which Elysia surfaces as 500s despite the route declaring a 400
ErrorResponseDto; wrap the service call in a try/catch, detect known/expected
service errors (e.g., validation/not-found types the service emits), set
set.status = 400 inside the catch, and return an object matching
ErrorResponseDto (message/code/etc. as used elsewhere) so the response conforms
to the declared 400 schema instead of resulting in a 500.
| import { swagger } from '@elysiajs/swagger'; | ||
| import type { Emitter } from '@rocket.chat/emitter'; | ||
| import Elysia from 'elysia'; | ||
| import { eventPlugin } from './controllers/federation/event.controller'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm federation auth/middleware order for new routes
Registering eventPlugin is fine. Please ensure all /_matrix/federation/* routes (including the new state endpoints) are wrapped by the same server-to-server authentication/verification middleware used elsewhere, and that plugin order doesn’t bypass it.
Run to spot a shared federation auth middleware:
Also applies to: 113-113
🏁 Script executed:
#!/bin/bash
# Look for federation auth/verify middlewares and where they are .use()'d relative to controllers.
rg -nP -C3 '(auth|signature|matrix).*(verify|auth|middleware)|X-Matrix|Authorization' packages | sed -n '1,200p'Length of output: 20410
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== packages/homeserver/src/homeserver.module.ts (if present) ==="
if [ -f packages/homeserver/src/homeserver.module.ts ]; then
sed -n '1,240p' packages/homeserver/src/homeserver.module.ts || true
else
echo "file not found"
fi
echo
echo "=== locate eventPlugin symbol ==="
rg -n --hidden -S '\beventPlugin\b' packages || true
echo
echo "=== search for federation routes ==="
rg -n --hidden -S '/_matrix/federation' packages -C3 || true
rg -n --hidden -S '\bfederation\b' packages -C3 || true
echo
echo "=== search for middleware / .use() in homeserver package ==="
rg -nP -S '\.use\(|app\.use\(|server\.use\(' packages/homeserver -C3 || true
echo
echo "=== search for Authorization / X-Matrix headers in homeserver package ==="
rg -n --hidden -S 'Authorization|X-Matrix' packages/homeserver -C3 || true
echo
echo "=== search for verify/signature/auth middleware across repo ==="
rg -nP -S 'verify.*signature|verifySignature|signature.*verify|server-to-server|server to server|s2s|federationAuth|federation-auth|federation_middleware|federationMiddleware' packages -C3 || trueLength of output: 110299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
packages/homeserver/src/controllers/federation/event.controller.ts
packages/homeserver/src/controllers/federation/send-join.controller.ts
packages/homeserver/src/controllers/federation/transactions.controller.ts
packages/homeserver/src/controllers/federation/invite.controller.ts
packages/homeserver/src/controllers/federation/profiles.controller.ts
packages/homeserver/src/controllers/key/server.controller.ts
packages/federation-sdk/src/services/signature-verification.service.ts
packages/federation-sdk/src/federation.module.ts
packages/federation-sdk/src/services/federation.service.ts
packages/homeserver/src/homeserver.module.ts
)
for f in "${files[@]}"; do
echo "=== $f ==="
if [ -f "$f" ]; then
sed -n '1,240p' "$f" || true
else
echo "file not found"
fi
echo
done
echo "=== search for verify/signature usage in controllers ==="
rg -n --hidden -S 'verifySignature|verify.*signature|signatureService|Authorization|X-Matrix|signatures' packages/homeserver packages/federation-sdk -C3 || trueLength of output: 66677
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="packages/federation-sdk/src/services/event.service.ts"
echo "=== $f ==="
if [ -f "$f" ]; then
sed -n '1,420p' "$f" || true
else
echo "file not found"
fi
echo
echo "=== search for processIncomingTransaction symbol across repo ==="
rg -n --hidden -S 'processIncomingTransaction' || true
echo
echo "=== search for middleware-like functions (authorization header parsing) ==="
rg -n --hidden -S 'X-Matrix|Authorization:|headers:.*Authorization|req.headers|request.headers' packages -C3 || trueLength of output: 16629
Enforce S2S Authorization header verification for all /_matrix/federation routes
No global middleware was found: homeserver.module.ts registers federation plugins directly (.use(invitePlugin).use(eventPlugin)...) and controllers like transactions.controller.ts call services (processIncomingTransaction) without validating the X-Matrix Authorization header. Event-level signature checks exist (checkSignAndHashes / SignatureVerificationService) but do not replace HTTP Authorization header validation.
- Add a shared federation-auth middleware (or call core/src/utils/authentication.validateAuthorizationHeader) and register it before any federation plugins in packages/homeserver/src/homeserver.module.ts.
- Ensure incoming endpoints that accept federation requests (e.g. packages/homeserver/src/controllers/federation/transactions.controller.ts, packages/homeserver/src/controllers/federation/event.controller.ts) are covered by that middleware or perform explicit header validation.
- Reference: packages/core/src/utils/authentication.ts and packages/federation-sdk/src/services/signature-verification.service.ts for existing helpers.
ggazzo
force-pushed
the
feat/states_endpoints
branch
from
13807c6 to
5b6f94b
Compare
ggazzo
force-pushed
the
chore/state-ids-for-all-events
branch
from
a0a52cb to
27983d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/room/src/state_resolution/definitions/definitions.ts (1)
98-133: Fix correctness: accumulator-based caching corrupts auth-chain results
_getAuthChaincaches a path-dependent accumulator for leaf events, so subsequent visits to the same event can return a superset that depends on traversal order. This can overcount, leak unrelated IDs, and destabilizeauth_chain_difference. Compute the per‑event transitive auth set independently and memoize that, not the running accumulator.Apply:
- // TODO: central cache for t6his - const eventIdToAuthChainMap = new Map<EventID, Set<EventID>>(); // do not repeat - - const _getAuthChain = async ( - event: PersistentEventBase, - existingAuthChainPart: Set<EventID>, - ) => { - const eventId = event.eventId; - - if (eventIdToAuthChainMap.has(eventId)) { - return eventIdToAuthChainMap.get(eventId)!; - } - - const authEvents = await event.getAuthorizationEvents(store); - if (authEvents.length === 0) { - eventIdToAuthChainMap.set(eventId, existingAuthChainPart); - return existingAuthChainPart; - } - - const authEventIdsSet = new Set(authEvents.map((e) => e.eventId)); - - let newAuthChainPart = existingAuthChainPart.union(authEventIdsSet); - - for (const authEvent of authEvents) { - const nextAuthChainPart = await _getAuthChain( - authEvent, - newAuthChainPart, - ); - if (!nextAuthChainPart) { - continue; - } - newAuthChainPart = newAuthChainPart.union(nextAuthChainPart); - } - - return newAuthChainPart; - }; - - return _getAuthChain(event, new Set([])); + // TODO: central cache for this (room-scoped). Memoize per event to avoid re-walks. + const memo = new Map<EventID, Set<EventID>>(); + + const dfs = async (ev: PersistentEventBase): Promise<Set<EventID>> => { + const id = ev.eventId; + const cached = memo.get(id); + if (cached) return cached; + + const result = new Set<EventID>(); + const authEvents = await ev.getAuthorizationEvents(store); + for (const ae of authEvents) { + result.add(ae.eventId); + const sub = await dfs(ae); + for (const s of sub) result.add(s); + } + memo.set(id, result); + return result; + }; + + return dfs(event);packages/federation-sdk/src/services/room.service.ts (3)
975-981: Fix tombstone auth chain type (use m.room.tombstone).Using m.room.message here yields the wrong auth events and can cause federation rejection.
- const authEvents = await this.eventService.getAuthEventIds( - 'm.room.message', + const authEvents = await this.eventService.getAuthEventIds( + 'm.room.tombstone', { roomId, senderId: sender, }, );
948-1034: Tombstone: correct room_version, avoid double-signing, await mutations, and return the persisted event.Current flow signs twice, uses defaultRoomVersion (risking canonicalization/signature issues), doesn’t await mutations, and returns the pre-persist event.
@@ const signingKey = await this.configService.getSigningKey(); @@ if (isTombstoned) { @@ this.validatePowerLevelForTombstone(powerLevelsEvent.event, sender); + // Use the actual room version when wrapping/signing + const roomVersion = await this.stateService.getRoomVersion(roomId); + if (!roomVersion) { + throw new HttpException('Room version not found', HttpStatus.NOT_FOUND); + } @@ - const signedEvent = await signEvent( - tombstoneEvent, - Array.isArray(signingKey) ? signingKey[0] : signingKey, - serverName, - ); - - const event = PersistentEventFactory.createFromRawEvent( - signedEvent, - PersistentEventFactory.defaultRoomVersion, - ); + const event = PersistentEventFactory.createFromRawEvent( + tombstoneEvent, + roomVersion, + ); - this.stateService.addAuthEvents(event); - this.stateService.addPrevEvents(event); - this.stateService.signEvent(event); + await this.stateService.addAuthEvents(event); + await this.stateService.addPrevEvents(event); + await this.stateService.signEvent(event); @@ - return signedEvent; + return event.event as SignedEvent<PduForType<'m.room.tombstone'>>;
461-487: Clean up power-level update: remove unused pre-PDU, use senderId and the room’s version.eventToSign is thrown away; also use the room’s actual version, not default.
- const eventToSign = roomPowerLevelsEvent({ - roomId, - members: [senderId, userId], - auth_events: { - 'm.room.create': createAuthResult._id, - 'm.room.power_levels': powerLevelsAuthResult._id, - 'm.room.member': memberAuthResult._id, - }, - prev_events: lastEventStore._id ? [lastEventStore._id] : [], - depth: lastEventStore.event.depth + 1, - content: { - ...currentPowerLevelsEvent.event.content, - users: { - ...(currentPowerLevelsEvent.event.content.users || {}), - [userId]: powerLevel, - }, - }, - ts: Date.now(), - }) as PduForType<'m.room.power_levels'>; - - const event = PersistentEventFactory.newPowerLevelEvent( - roomId, - eventToSign.sender, - eventToSign.content, - PersistentEventFactory.defaultRoomVersion, - ); + const content = { + ...currentPowerLevelsEvent.event.content, + users: { + ...(currentPowerLevelsEvent.event.content.users || {}), + [userId]: powerLevel, + }, + }; + const roomVersionForPL = await this.stateService.getRoomVersion(roomId); + if (!roomVersionForPL) { + throw new HttpException('Room version not found', HttpStatus.NOT_FOUND); + } + const event = PersistentEventFactory.newPowerLevelEvent( + roomId, + senderId, + content, + roomVersionForPL, + );
♻️ Duplicate comments (7)
packages/federation-sdk/src/services/state.service.ts (3)
150-165: Wrong event fetched when prevStateIds is empty (uses requested eventId instead of lastStateDelta.eventId).Base state should be materialized from lastStateDelta’s event. Use _lastStateEventId.
Apply:
- const event = await this.eventRepository.findById(eventId); + const event = await this.eventRepository.findById(_lastStateEventId); if (!event) { - throw new Error(`Event ${eventId} not found`); + throw new Error(`Event ${_lastStateEventId} not found`); }
209-219: Don’t mutate prevStateIds; fix error message.Avoid pop() to prevent mutating DB-returned arrays and clarify message.
- const state = await this.stateRepository.getStateById(stateId); + const state = await this.stateRepository.getStateById(stateId); if (!state) { - throw new Error(`Event ${stateId} not found`); + throw new Error(`State mapping ${stateId} not found`); } - const prevStateId = state.prevStateIds.pop(); + const prevStateId = + state.prevStateIds[state.prevStateIds.length - 1]; if (!prevStateId) { - throw new Error(`State ${stateId} has no previous state`); + throw new Error(`State mapping ${stateId} has no previous state`); } return prevStateId;
251-255: Guard undefined stateId before querying repository.Avoid passing undefined into getStateById.
- const { stateId } = event; + const { stateId } = event; + if (!stateId) { + this.logger.error({ eventId }, 'state id not found'); + throw new Error('State id not found'); + }packages/homeserver/src/controllers/federation/event.controller.ts (2)
18-35: Map service errors to the declared 400 schema.Wrap handler in try/catch and set set.status = 400 to match ErrorResponseDto.
- ({ params, query }) => - eventService.getStateIds(params.roomId, query.event_id), + async ({ params, query, set }) => { + try { + return await eventService.getStateIds(params.roomId, query.event_id); + } catch (e) { + set.status = 400; + return { + error: 'Bad Request', + details: e instanceof Error ? e.message : e, + }; + } + },
36-53: Same: catch and map to 400; drop unnecessary non-null assertion.GetStateQueryDto requires event_id, so no need for !.
- ({ params, query }) => - eventService.getState(params.roomId, query.event_id!), + async ({ params, query, set }) => { + try { + return await eventService.getState(params.roomId, query.event_id); + } catch (e) { + set.status = 400; + return { + error: 'Bad Request', + details: e instanceof Error ? e.message : e, + }; + } + },packages/room/src/state_resolution/definitions/definitions.ts (1)
152-156: Spec compliance: don’t include the event itself in auth-chain differenceAuth chain excludes the event itself; adding
event.eventIdhere changes the union/intersection and can report spurious differences. Remove explicit inclusion; rely solely ongetAuthChain(event, store). (spec.matrix.org)Apply:
- // TODO: deb check this I changed to keep the function behaving as the spec - for (const authChainEventId of [ - ...(await getAuthChain(event, store)), - event.eventId, - ]) { + // The auth chain of an event excludes the event itself. + for (const authChainEventId of await getAuthChain(event, store)) { authChainForState.add(authChainEventId); }packages/federation-sdk/src/services/room.service.ts (1)
763-772: Fix room_version skew in copyEvent (use negotiated version).copyEvent re-wraps with local constant '10'; use makeJoinResponse.room_version to avoid hash/signature mismatches. (Same concern as earlier review.)
- const roomVersion = '10' as const; @@ - const copyEvent = (event: Readonly<PersistentEventBase>) => { - return PersistentEventFactory.createFromRawEvent( - structuredClone(event.event), - roomVersion, - ); - }; + const copyEvent = (event: Readonly<PersistentEventBase>) => { + return PersistentEventFactory.createFromRawEvent( + structuredClone(event.event), + makeJoinResponse.room_version, + ); + };Also applies to: 817-823
🧹 Nitpick comments (7)
packages/core/package.json (1)
20-22: New @hs/crypto dep: verify workspace wiring and avoid cycles.Looks fine, but please confirm:
- @hs/crypto is properly published in the workspace and built before @hs/core.
- No circular import between @hs/core and @hs/crypto (e.g., through signEvent usage).
packages/federation-sdk/src/services/state.service.ts (4)
285-285: Remove stray console.log in service code.Prefer structured logger to avoid noisy stdout in prod.
- console.log('stateMappings ->', stateMappings); + // this.logger.debug({ stateMappings }, 'state mappings for reconstruction');
401-417: Nit: improve error string.“trying to room state” -> “trying to build room state”.
- throw new Error('Event id mismatch in trying to room state'); + throw new Error('Event id mismatch when trying to build room state');
610-613: Ensure prevStateIds defaults to an array.createStateMapping may receive undefined; coalesce to [] to keep API stable.
- const prevStateIds = lastState?.prevStateIds?.concat( - lastState?._id?.toString(), - ); + const prevStateIds = [ + ...(lastState?.prevStateIds ?? []), + ...(lastState?._id ? [lastState._id.toString()] : []), + ];(Apply in both locations.)
Also applies to: 732-734
119-121: Log message mismatch.Method is “findStateAroundEvent” but log says “before”. Align wording.
- this.logger.debug({ eventId }, 'finding state before event'); + this.logger.debug({ eventId }, 'finding state around event');packages/federation-sdk/package.json (1)
21-22: Validate runtime compatibility for new deps.mongodb@^6.16.0 requires modern Node APIs; if running under Bun, confirm compatibility in CI. Consider adding an engines field or a note to avoid surprise runtime issues.
Would you like a quick script to scan CI config for Node/Bun versions and ensure they meet mongodb’s requirements?
packages/room/package.json (1)
21-21: Hoist Zod to the workspace root and switch package references to "workspace:^".Found "zod" in packages/federation-sdk/package.json:26 and packages/room/package.json:21 (both ^3.22.4). Add Zod to the root package.json (e.g. "zod": "^3.22.4") and replace per-package entries with:
- "zod": "^3.22.4" + "zod": "workspace:^"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
packages/core/package.json(1 hunks)packages/core/src/endpoints.ts(0 hunks)packages/core/src/query.ts(0 hunks)packages/core/src/server-discovery/_multi-error.ts(0 hunks)packages/core/src/server-discovery/_resolver.ts(0 hunks)packages/core/src/utils/makeRequest.ts(0 hunks)packages/core/src/wellknown.ts(0 hunks)packages/federation-sdk/package.json(1 hunks)packages/federation-sdk/src/container.ts(0 hunks)packages/federation-sdk/src/federation.module.ts(0 hunks)packages/federation-sdk/src/index.ts(0 hunks)packages/federation-sdk/src/repositories/state.repository.ts(2 hunks)packages/federation-sdk/src/services/event-state.service.ts(0 hunks)packages/federation-sdk/src/services/event.service.ts(2 hunks)packages/federation-sdk/src/services/federation.service.ts(0 hunks)packages/federation-sdk/src/services/room.service.ts(1 hunks)packages/federation-sdk/src/services/signature-verification.service.spec.ts(0 hunks)packages/federation-sdk/src/services/signature-verification.service.ts(0 hunks)packages/federation-sdk/src/services/state.service.ts(6 hunks)packages/federation-sdk/src/utils/binaryData.ts(0 hunks)packages/federation-sdk/src/utils/keys.ts(0 hunks)packages/federation-sdk/src/utils/signJson.ts(0 hunks)packages/homeserver/src/cache.ts(0 hunks)packages/homeserver/src/controllers/federation/event.controller.ts(1 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(2 hunks)packages/homeserver/src/dtos/federation/profiles.dto.ts(1 hunks)packages/homeserver/src/dtos/federation/state-ids.dto.ts(1 hunks)packages/homeserver/src/dtos/federation/state.dto.ts(1 hunks)packages/homeserver/src/dtos/index.ts(1 hunks)packages/homeserver/src/homeserver.module.ts(2 hunks)packages/room/package.json(1 hunks)packages/room/src/authorizartion-rules/rules.ts(0 hunks)packages/room/src/manager/membership-event.ts(0 hunks)packages/room/src/state_resolution/definitions/definitions.ts(3 hunks)
💤 Files with no reviewable changes (19)
- packages/federation-sdk/src/utils/binaryData.ts
- packages/federation-sdk/src/index.ts
- packages/federation-sdk/src/container.ts
- packages/federation-sdk/src/services/signature-verification.service.ts
- packages/room/src/manager/membership-event.ts
- packages/federation-sdk/src/services/signature-verification.service.spec.ts
- packages/federation-sdk/src/utils/signJson.ts
- packages/core/src/wellknown.ts
- packages/federation-sdk/src/utils/keys.ts
- packages/core/src/server-discovery/_multi-error.ts
- packages/core/src/server-discovery/_resolver.ts
- packages/federation-sdk/src/services/federation.service.ts
- packages/federation-sdk/src/federation.module.ts
- packages/core/src/query.ts
- packages/federation-sdk/src/services/event-state.service.ts
- packages/homeserver/src/cache.ts
- packages/room/src/authorizartion-rules/rules.ts
- packages/core/src/endpoints.ts
- packages/core/src/utils/makeRequest.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/homeserver/src/dtos/index.ts
- packages/homeserver/src/homeserver.module.ts
- packages/homeserver/src/controllers/federation/profiles.controller.ts
- packages/homeserver/src/dtos/federation/state.dto.ts
- packages/homeserver/src/dtos/federation/profiles.dto.ts
- packages/homeserver/src/dtos/federation/state-ids.dto.ts
- packages/federation-sdk/src/services/event.service.ts
- packages/federation-sdk/src/repositories/state.repository.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/homeserver/src/controllers/federation/event.controller.ts (4)
packages/federation-sdk/src/index.ts (1)
EventService(47-47)packages/homeserver/src/dtos/federation/state-ids.dto.ts (3)
GetStateIdsParamsDto(4-6)GetStateIdsQueryDto(8-10)GetStateIdsResponseDto(12-17)packages/homeserver/src/dtos/common/error.dto.ts (1)
ErrorResponseDto(3-6)packages/homeserver/src/dtos/federation/state.dto.ts (3)
GetStateParamsDto(4-6)GetStateQueryDto(8-10)GetStateResponseDto(12-19)
packages/room/src/state_resolution/definitions/definitions.ts (2)
packages/room/src/manager/v3.ts (1)
eventId(21-33)packages/room/src/manager/event-wrapper.ts (1)
event(102-112)
packages/federation-sdk/src/services/room.service.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
event(102-112)
packages/federation-sdk/src/services/state.service.ts (4)
packages/room/src/types/_common.ts (2)
State(9-9)StateMapKey(7-7)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(37-968)packages/federation-sdk/src/repositories/state.repository.ts (1)
StateStore(14-25)packages/room/src/manager/room-state.ts (1)
RoomState(15-154)
🔇 Additional comments (3)
packages/room/package.json (1)
20-21: Keep zod in dependencies — used at runtime.
packages/room/src/types/v3-11.ts imports { z } from 'zod' and defines runtime schemas (z.object, z.enum, z.record, z.union, transforms, etc.), so zod should remain in dependencies.packages/federation-sdk/src/services/room.service.ts (2)
1029-1030: LGTM: generic broadcast for tombstone.Switching to sendEventToAllServersInRoom simplifies the path and aligns with other state broadcasts.
336-341: persistStateEvent signs state events — updateRoomName is fineStateService.persistStateEvent calls this.signEvent internally before persisting (see packages/federation-sdk/src/services/state.service.ts around the signEvent/persistStateEvent implementations), so no explicit signEvent is required in updateRoomName.
ggazzo
force-pushed
the
chore/state-ids-for-all-events
branch
from
27983d5 to
5b6f94b
Compare
Summary by CodeRabbit
New Features
Refactor
Chores