fix(viewer): session-scope chronicle seq — render post-move DM narration after session rotation (BUG2) - #569
Merged
Conversation
…ion after session rotation (BUG2)
In the WorldOS .app the DM's narration stopped rendering in the Chronicle
after a player move ("Composing…" then nothing).
ROOT CAUSE: narration is deduped + ordered by `seq`, a BARE per-session-log
line index with NO session-id scoping (both server and client). When the
engine ROTATES the session log (cold-open start_session + DM-turn-retry
re-mint, 5e71f77), the new session's narration restarts at seq 0,1,2 — the
same values the cold-open already claimed. The post-move narration was then
BOTH suppressed by claimNarrationSeq AND dropped by buildChronicleLog's seq
match.
FIX: namespace `seq` with the session id (composite `${sid}:${seq}`) so it is
globally unique across rotations, while preserving within-session monotonicity
for the order tiebreak.
- server.py /events response now carries the resolved `sid` (new helper
`_active_session_id`); `_session_event_tail_from_dir` + `_session_recent_events`
carry `sid` on recentEvents rows. `seq` stays the bare int (contract intact).
- app.jsx composes `${sid}:${seq}` for claimNarrationSeq/seenSeq and orderSeq.
- screen-table.jsx buildChronicleLog mirrors the composite via a seqKeyOf
helper (liveSeqs + recentEvents dedup); compareChronicle parses the numeric
tail for the within-session tiebreak, falling to creation-order across
sessions (eventAt remains the primary sort).
Adds a cross-session regression test: a NEW session's seq 0,1 after a rotation
RENDERS (not suppressed by collision with a prior session's seq 0,1). The #405
same-session re-ingest collapse (test_same_seq_reingested_is_shown_once) still
passes.
📝 WalkthroughWalkthroughThis PR fixes a narration deduplication bug where sequence indices ( ChangesSession-scoped narration dedup fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@viewer/server.py`:
- Around line 6601-6606: The entries and response sid are read from two separate
snapshots causing a race; ensure both come from the same session snapshot by
reading the active session id once and using that same id when fetching entries
(or modify _read_events to return the session id along with entries and nxt).
Concretely, call _active_session_id(view_cid) first (or update
_read_events(view_cid, since) to return (entries, nxt, sid)) and then use that
single sid value in the JSON response instead of calling _active_session_id
again.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bac45102-b064-4798-9516-ef36afd3e866
📒 Files selected for processing (4)
viewer/openworlds/app.jsxviewer/openworlds/screen-table.jsxviewer/server.pyviewer/tests/test_live_narration_stream.py
Comment on lines
+6601
to
+6606
| entries, nxt = _read_events(view_cid, since) | ||
| # BUG2: include the resolved session id so the client composes a globally-unique | ||
| # `${sid}:${seq}` dedup/order key — a bare per-session line index collides across a | ||
| # session rotation (cold-open + DM-turn-retry re-mint), suppressing the new session's | ||
| # post-move narration (seq 0,1,2 already claimed by the prior session's cold-open). | ||
| self._json({"entries": entries, "next": nxt, "sid": _active_session_id(view_cid)}) |
There was a problem hiding this comment.
Use one session-id source for entries and response sid
Line 6601 and Line 6606 resolve session state via separate snapshot reads, so a rotation between them can return entries from one session and sid from another. That miskeys ${sid}:${seq} on the client and can still drop/duplicate narration around rotation boundaries.
Suggested fix
-def _read_events(campaign_id: str, since: int) -> tuple[list[dict], int]:
+def _read_events(campaign_id: str, since: int) -> tuple[list[dict], int, str]:
@@
- if not sid:
- return [], since
+ if not sid:
+ return [], since, ""
@@
- if not log.exists():
- return [], since
+ if not log.exists():
+ return [], since, sid
@@
- return out, consumed
+ return out, consumed, sid- entries, nxt = _read_events(view_cid, since)
+ entries, nxt, sid = _read_events(view_cid, since)
@@
- self._json({"entries": entries, "next": nxt, "sid": _active_session_id(view_cid)})
+ self._json({"entries": entries, "next": nxt, "sid": sid})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@viewer/server.py` around lines 6601 - 6606, The entries and response sid are
read from two separate snapshots causing a race; ensure both come from the same
session snapshot by reading the active session id once and using that same id
when fetching entries (or modify _read_events to return the session id along
with entries and nxt). Concretely, call _active_session_id(view_cid) first (or
update _read_events(view_cid, since) to return (entries, nxt, sid)) and then use
that single sid value in the JSON response instead of calling _active_session_id
again.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BUG2 (critical)
In the WorldOS
.app, the DM's narration stopped rendering in the Chronicle after a player move — the player saw "Composing…" then nothing.Root cause
Narration is deduped + ordered by
seq, a bare per-session-log line index with NO session-id scoping (both server and client). When the engine rotates the session log (cold-openstart_session+ the DM-turn-retry re-mint,5e71f77), the new session's narration restarts atseq 0,1,2— the same values the cold-open already claimed. The post-move narration was then both suppressed byclaimNarrationSeqand dropped bybuildChronicleLog's seq match.No cross-session test existed to catch this — every prior
seqtest was single-session.Fix
Namespace
seqwith the session id (composite${sid}:${seq}) so it is globally unique across rotations, while preserving within-session monotonicity for the order tiebreak.Server (
viewer/server.py)/eventsresponse now carries the resolvedsid(new helper_active_session_id)._session_event_tail_from_dir+_session_recent_eventscarrysidon recentEvents rows.seqstays a bare int —_read_events's 2-tuple + bare-int contract is unchanged (test_read_events_stamps_absolute_seq_across_pollsandtest_session_event_tail_stamps_stable_absolute_seqstay green).Client (
viewer/openworlds/app.jsx)/eventspoll composes${sid}:${seq}and keysclaimNarrationSeq/seenSeq+ the beat'sorderSeqby the composite (empty sid →:N, still unique within the single session it serves).Client (
viewer/openworlds/screen-table.jsx)buildChronicleLogmirrors the composite via aseqKeyOfhelper (theliveSeqsset + the recentEvents dedup).compareChronicleparses the composite's numeric tail for the within-session tiebreak, falling to creation-order (.at) across sessions.eventAtremains the primary sort.Test
Adds
test_new_session_seq_collision_after_rotation_still_renderstoviewer/tests/test_live_narration_stream.py— the cross-session twin oftest_same_seq_reingested_is_shown_once: a new session'sseq 0,1after a rotation renders (live beats and assembled chronicle), not suppressed by collision with the prior session'sseq 0,1.The #405 same-session re-ingest collapse (
test_same_seq_reingested_is_shown_once) still passes — samesid:seqstill collapses to one row.Verification
python3 -m py_compile viewer/server.py✅.jsxfiles ✅test_live_narration_streamtests pass (33 existing + the new one), plus the two_read_events/ event-tail server tests. CIviewer-testsruns the full suite..appre-run (1 persona) must confirm the DM reply now renders in the Chronicle post-move. The unit tests prove the dedup/order logic; they don't exercise the real.app.Summary by CodeRabbit