Repository navigation
host: a notice the host edits never reaches the guests already seated - #1942
Merged
Merged
Conversation
Contributor
Author
Collaborator
|
Verdict: PASS at 6fbe848 Ran at this tip: Points:
|
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
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.
Fixes #1939
Owner: Dev-1
Written-by: Dev-1
Reviewer: Birfy
Base / tip
Base: 564a8bc
Tip SHA: 6fbe848 (the pushed head; its parent is that base and its tree is 0886c74cebb6683ac2e18188c39934c26f92b222, so the run below covers it)
Verified commit: 4fc0d83b293af2b2a9aa01d16db3dd124b3d3269 (tree 0886c74cebb6683ac2e18188c39934c26f92b222; the pushed head is that same tree, rebuilt onto live main by the push path — the branch carries one replay commit,
git log upstream/main..HEADshows only it andmerge-baseis 564a8bc)Diff: 6 files, +160/−5
What changed
updateRoomwrites a newnoticeand records the change, but the room-share envelope was only ever pushed when a room is created (notifyRoomSharehad exactly one call site, in the join path). So a host that edits or clears the setting wrote the value into its own roster and told nobody: the Bots already seated in the room kept the old notice until they left and rejoined, and clearing it never reached them at all.The fix is one guarded line in
core/src/host/roster.js, after the save and before the return, in the same shape as the join path: whennotice !== undefined && notice !== (room.notice ?? '')was true,void rt.notifyRoomShare?.(room, { op: 'join' }). The envelope is the room's current roster — the same object the join path sends, with only thenoticefield different from the last one the guest received. It is not a new line, not a system message and not an extra event per keystroke: a settings save that leaves the notice alone sends nothing, and only a notice that actually changed sends one envelope.The guard is the point, so it is its own assertion:
updateRoomwith a patch that carries nonotice, or that writes back the value it already holds, must leave the fakeflybotSenduntouched — otherwise every membership or admin edit would re-push the notice to every guest.core/test/remote-room.test.jsgains one test (#1939:) that drives two hosts through the real roster: both join, then the host writes{notice: 'New'}with no membership change and exactly oneop: 'join'arrives; the guest'sapplyInboundRoomShareleavesroom.notice === 'New'; a following{notice: ''}is the second envelope and the guest's notice is empty. The test also asserts the envelope is the same snapshot with only the notice changed (every other field deep-equals the previous join,room.loglength and members unchanged), that{notice: ''}written back at its own value sends nothing, and that a patch touchingname/mode/adminbut notnoticesends nothing.docs/subsystems/core-host.mdgains the sentence that owns this behaviour, andchangelog.d/1939-notice-push.fixed.mdis the user-facing note.core/test/remote-room.test.jswas rebased onto live main, which now carries the fence test additions from #1938 at the same tail of the file: the replay keeps both suites (git diff 564a8bcf HEAD -- core/test/remote-room.test.jsis +67/−0, so no upstream case is dropped), andpatches/ds-bot/files/test/remote-room.test.jsis its byte-identical mirror.Red first
Both reds were re-run on this rebased tree, each against its own change, and each failed for its own reason.
Reverting the implementation (
git checkout 564a8bcf -- core/src/host/roster.js, test file unchanged):Making the push unconditional instead of guarding it (
if (notice !== undefined)):Restored afterwards:
# tests 83 / # pass 83 / # fail 0for that file.Ran
TZ=UTC ARCH_DOCS_BASE=564a8bcf6025c6aec6051968f061ee04fd80b3c1 npm run verifyon the committed tree, rebased onto live main564a8bcf: exit 0. core 2343, social 541, tree 127, timeline 135, workflow 336,.github/scripts563, interop 37, live-e2e 24 — 0 failures;arch-docs: OK (6 changed file(s))against that base; lint 0 errors; the gate's own patch legs:ok 10 patches apply cleanly in order,ok core/ equals ds-bot/ + patches/ds-bot/,ok patches/ds-bot/ is what generate writes (generate is a no-op).generatere-ran on the new base and wrote nothing (the stored series is already what it produces), andcheck/canonicalare green standalone.The first full run of that same command exited 1 on one unrelated, timing-dependent case:
core/test/room-external.test.js:79—a stop is asked for on the protocol before the signal, and a shell is not a program (#198)—timed out waiting for the childafter 4.1s under the parallel load. That file alone is# tests 13 / # pass 13 / # fail 0, and the re-run of the wholenpm run verifywas the exit-0 run quoted above (core 2343/2343). Nothing in this diff touches the external-room spawn path.Not run: no live host run (the change is host-side state plus one envelope), and no manual two-instance check — the test drives the real roster module with a fake
flybotSendand the realapplyInboundRoomShareon the guest side.Checkpoint
{ "pr": 1942, "issue": 1939, "claim": "Dev-1", "summary": "updateRoom now pushes the room-share envelope when, and only when, the notice actually changed, so a host that edits or clears the setting reaches the guests already seated instead of waiting for them to rejoin. The envelope is the same roster snapshot with only the notice field different, so it adds no line and no extra event. A settings save that leaves the notice alone sends nothing. The test pins both directions plus the guest-side notice after applyInboundRoomShare, and a patch that changes other settings but not the notice sends nothing either.", "files": ["core/src/host/roster.js", "core/test/remote-room.test.js", "docs/subsystems/core-host.md", "changelog.d/1939-notice-push.fixed.md", "patches/ds-bot/0002-flybot-api-host.patch", "patches/ds-bot/files/test/remote-room.test.js"], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=564a8bcf6025c6aec6051968f061ee04fd80b3c1 npm run verify, plus both reds on this tree", "result": "green", "at": "6fbe84888467ed0a85775d1241f5369f858d16fc" }, "next": "none — ready for review" }