fix: getAuthChain including the event itself

[ggazzo]

extracted from #171

Summary by CodeRabbit

• Bug Fixes
  • Improved state resolution to align with spec, preventing self-inclusion in auth chains. This yields more accurate conflict handling, reducing room desyncs and edge-case errors during joins and state changes.
  • Enhances stability when comparing state across events, lowering the chance of incorrect differences and resulting inconsistencies.

[coderabbitai]

Warning

Rate limit exceeded

@ggazzo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 32 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between ac21e3f and 4a41834.

📒 Files selected for processing (1)

• packages/room/src/state_resolution/definitions/definitions.ts (3 hunks)

Walkthrough

The auth chain computation was adjusted to exclude the starting event from its own chain. The difference calculation now explicitly appends the current event ID when building per-state chains. A cached-chain return path was tightened with a non-null assertion. No public signatures changed.

Changes

Cohort / File(s)	Summary
Auth chain semantics and difference calculation packages/room/src/state_resolution/definitions/definitions.ts	- _getAuthChain initializes accumulator as empty set; starting event excluded from its own chain - Wrapper calls _getAuthChain(event, new Set([])) - Cache hit returns eventIdToAuthChainMap.get(eventId)! with non-null assertion - getAuthChainDifference builds per-state chains as [...(await getAuthChain(event, store)), event.eventId] per spec comment

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant Definitions as definitions.ts
  participant Cache as AuthChainCache
  participant Store

  Note over Caller,Definitions: Compute auth chain for an event (starting event excluded)
  Caller->>Definitions: getAuthChain(event, store)
  alt Cached
    Definitions->>Cache: lookup(event.id)
    Cache-->>Definitions: authChain (ancestors only)
    Definitions-->>Caller: authChain
  else Not cached
    rect rgba(200,220,255,0.25)
    Note right of Definitions: _getAuthChain(event, acc = ∅)
    loop Traverse ancestors
      Definitions->>Store: fetch auth events
      Store-->>Definitions: parent events
      Definitions->>Definitions: add parents to acc
    end
    end
    Definitions-->>Caller: acc (ancestors only)
  end

Loading

sequenceDiagram
  autonumber
  participant Caller
  participant Definitions as definitions.ts

  Note over Caller,Definitions: Build per-state chains for difference calculation
  Caller->>Definitions: getAuthChainDifference(stateEvents, store)
  loop For each event in state
    Definitions->>Definitions: chain = await getAuthChain(event, store)
    Note right of Definitions: Excludes starting event by design
    Definitions->>Definitions: perState = [...chain, event.eventId]
  end
  Definitions-->>Caller: Difference result derived from perState sets

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I hopped through chains of auth, so neat,
Skipped the start—just ancestor feet.
Then in the diff, I add it back,
A tidy trail on rabbit track.
Cache burrows warm, assertions bright—
The graph now glows in moonlit byte.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "fix: getAuthChain including the event itself" succinctly and accurately summarizes the primary change: correcting getAuthChain to stop including the starting event and updating consumers to include the event when necessary. It is concise, specific to the implemented fix, and readable for teammates scanning history. As such it aligns with the project's title guidelines.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

---

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

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.42%. Comparing base (4d647f3) to head (4a41834).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #193   +/-   ##
=======================================
  Coverage   80.41%   80.42%           
=======================================
  Files          58       58           
  Lines        4514     4516    +2     
=======================================
+ Hits         3630     3632    +2     
  Misses        884      884           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

packages/room/src/state_resolution/definitions/definitions.ts (2)

104-106: Avoid non-null assertion; prefer a guarded read for clarity.

Behavior is unchanged, but this reads cleaner and satisfies strict linters without !.

-    if (eventIdToAuthChainMap.has(eventId)) {
-      return eventIdToAuthChainMap.get(eventId)!;
-    }
+    const cached = eventIdToAuthChainMap.get(eventId);
+    if (cached) {
+      return cached;
+    }

---

152-156: Including the event ID in per‑state chains is correct; streamline the loop and replace TODO.

Avoid the temporary array and make the intent explicit.

-      // TODO: deb check this I changed to keep the function behaving as the spec
-      for (const authChainEventId of [
-        ...(await getAuthChain(event, store)),
-        event.eventId,
-      ]) {
-        authChainForState.add(authChainEventId);
-      }
+      // Includes the event itself per spec (see NOTE link above).
+      for (const id of await getAuthChain(event, store)) {
+        authChainForState.add(id);
+      }
+      authChainForState.add(event.eventId);

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c41fdf and ac21e3f.

📒 Files selected for processing (1)

• packages/room/src/state_resolution/definitions/definitions.ts (3 hunks)

🔇 Additional comments (1)

packages/room/src/state_resolution/definitions/definitions.ts (1)

132-132: Auth chain should exclude the starting event — verify downstream callers/tests

Matches the spec (ancestors-only). Found call sites to verify/update:

• packages/room/src/state_resolution/definitions/algorithm/v2.ts — calls getAuthChain(event, wrappedStore) and iterates authChain; confirm it doesn't rely on the event being included.
• packages/federation-sdk/src/services/send-join.service.ts — calls getAuthChain(event, stateService._getStore(roomVersion)); confirm expectations for send-join.
• packages/room/src/state_resolution/definitions/definitions.ts — caller explicitly appends event.eventId after getAuthChain (already compensates).

Verify tests and callers that assumed the old behavior and adjust as needed.