Repository navigation
Pin the rename that leaves the computed name alone, and the settings line's signer - #2025
Merged
Merged
Conversation
Contributor
Author
|
@Birfy — review request for this tip. #2025 at Checks already ran at this tip (14 checks, Scope: test-only. It pins two already-merged guards in |
…inkFlowLab#1971) `followMemberNames` (`core/src/host/roster.js:284`) pushes the same `join` an explicit rename pushes, and three conditions keep it quiet. The `if (next === room.name) continue` guard (`:288`) had no test: the case whose name suggests it renames a Bot that sits in no room, so `updateBot`'s member filter (`:202`) hands the loop an empty list and it never runs. Renaming a member to the name it already holds does reach the loop — the room passes the filter and the computed name is what the room already has — and without the guard that write-back sends one more identical `join`. A join carries a higher `seq`, which reopens a mirror the guest had hidden itself (ThinkFlowLab#1167, ThinkFlowLab#1939). The new case drives the real `updateBot` route on an unnamed room with one seated remote guest: a rename that moves the computed name sends exactly one envelope, and the write-back sends none. `showSettings` (`:515`) reads the signed-in member before the room's implicit owner, and no case could see that order: the fixture's actor and owner are both `Alice`. With a distinct signer — `Bob` saving, `Alice` the owner — moving the `actor` step behind `implicitRoomOwner` names the owner instead of the member. Control 3's comment now says it pins the member filter at `:202`, not the guard. Measured on this tree (base c8c5074): the file is 114 tests / 114 pass. Deleting `:288` turns only the rename case red (1 !== 0, 114 / 113 / 1); moving the `actor` step behind `implicitRoomOwner` turns only the settings case red (`Bob` expected, `Alice` actual, 114 / 113 / 1). Docs-not-needed: test-only pin; no product code and no behaviour change (ThinkFlowLab#1971)
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 #1971
Written-by: Dev-2
Owner: Dev-2
Reviewer: Birfy
Claim-Id: fff42f984c524f0f93cf76308953d695
Branch: dev/2-pin-rename-guard-v2
Base: e807b26
What changed
Three test-only pins in
core/test/remote-room.test.js, plus itspatches/ds-bot/files/mirror.#1971: a member renamed to the name it already holds sends no join— theif (next === room.name) continueguard infollowMemberNames(core/src/host/roster.js:288) had no case. The case whose name suggests it renames a Bot that sits in no room, soupdateBot's member filter (:202) hands the loop an empty list and the guard never runs. The new case drives the realupdateBotroute on an unnamed room with one seated remote guest: a rename that moves the computed name sends exactly onevia: 'room-share'envelope, and saving the same name again sends none.#1971: a settings line names the member who saved, not the room owner—showSettings(:515) reads the signed-in member before the room's implicit owner, and no case could see that order because the fixture's actor and owner are bothAlice. The new case signs inBobagainstAlice's room and saves the notice: the line readsBob updated the group notice.:202, not the:288guard, so the next reader does not count it twice.Line numbers are the live base
e807b261(:287/:201in the issue came from the older2dfc1c3a). No product code, no behaviour change.Line by line
core/test/remote-room.test.js— +63 / −1: the two cases above and the corrected control comment.patches/ds-bot/files/test/remote-room.test.js— +63 / −1: its generated mirror (node scripts/ds-bot-patches.mjs generate).What I ran
TZ=UTC ARCH_DOCS_BASE=e807b2616cf91a3546ebd460cffbb7325d2dc279 npm run verifyat this tip, on a clean tree: exit 0.core2438 tests / 2438 pass / 0 fail / 0 skipped;plugins/social541 (540 + 1 skipped),plugins/tree127 (126 + 1),plugins/timeline135 (134 + 1),plugins/workflow336 / 336,.github563 / 563, interop 37 / 37, live-e2e 24 / 24. The three single skips (social/tree/timeline) are the same case,Cordis 4 not installed (set CORDIS_PATH or DSH_HARNESS); it reproduces on this host, and the totals above are 541/127/135 either way — where Cordis 4 resolves, those three cases run and the skip count is 0.arch-docs: OK (2 changed file(s)); oxlint 73 warnings, 0 errors (the base's own count);10 patches apply cleanly in order;core/ equals ds-bot/ + patches/ds-bot/.core/test/remote-room.test.json the unmutated tree: 115 tests / 115 pass / 0 fail.roster.js:288: only the rename case goes red, three runs out of three —a rename that leaves the computed name alone sends nothing,1 !== 0; the file is 115 / 114 / 1.actorstep behindimplicitRoomOwnerinshowSettings: only the settings case goes red, three runs out of three —name: 'Bob'expected,'Alice'actual; the file is 115 / 114 / 1.What I did not run
dev/bench.sh smoke): this branch touches noindex.jsand nothing undercore/src/host/.tests-okand the merge queue: they run on the pull request, not here.Checkpoint
{ "pr": 2025, "claim": "Dev-2", "issue": 1971, "summary": "Adds the two missing pins in remote-room.test.js: renaming a member to the name it already holds sends no room-share envelope, and a settings line names the signed-in member who saved rather than the room's owner. Also corrects the comment on the control case so it names the member filter it really pins. Test-only; no product code and no behaviour change.", "files": [ "core/test/remote-room.test.js", "patches/ds-bot/files/test/remote-room.test.js" ], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=e807b2616cf91a3546ebd460cffbb7325d2dc279 npm run verify", "result": "green", "at": "97642af391dbd8755b2260423624af25e6962b3f" }, "next": "none — ready for review" }Docs-not-needed: test-only pin; no architecture change and no product code (#1971)