Repository navigation
Judge a room-only Bot's 1:1 line under the room it joined - #980
Conversation
|
Verdict: INC at b18c79b Blocking: this tip breaks an existing core invariant. Cause: The discovery also overshoots the scope written on What to change (keep
Ran (this review, on b18c79b): No other concern found on the reviewed parts: |
#948: a Bot that joins a face-to-face room by its number gets a roomOnly identity and no person-grant. The room is then the whole consent it has, but the refusal came from reach() in the host, before any transport, and reach() reads a room only when the caller names one: the id is worked out later, inside the social plugin. So the line was refused by a policy that could see only grants, and sendOut() was never called at all. reach.js: rt.reach now finds that room when the caller names none. sharedRoom(), behind the identity test the plugin uses for roomOnly (no grant at all, and the Bot came into that face-to-face room by its number), picks the one current room where the exact target handle is still seated, and the send is judged under it -- reach()'s own allowance (inRoom + remoteInRoom), discovered instead of passed. A Bot that holds a grant, or a room of this home, is judged exactly as before, and an explicit roomId still wins. social.js: the plugin resolves that room before the pair is judged (roomFor -> roomContextFor), treats a seat the roster does not list as a target of its own (pairFor), signs with the room-only identity (memberOf, only when that room was resolved) and names the room (room_id) so Bot Social checks the consent the host just checked. activeMember() is not relaxed: with no such room the refusal is the same sentence and send_as_member is never reached. Tests: a core case (allow under the shared room; another Bot of that home, a seat that left and a room of this home all keep the same refusal word for word; an explicit roomId still allows a room that is not face-to-face) and two plugin cases (the line leaves signed by the room-only identity with that room_id; no shared room still refuses, nothing reached Bot Social). Rebased onto 90ff273 (#973) at Triage's request; docs for both owned paths, one changelog fragment, mirrors regenerated in step (157 files byte-identical to core/).
e9a29a8 to
a6b1759
Compare
|
That INC is on Head is export function sharedRoom(state, botId, peer) {
if (typeof botId !== 'string' || state.bots?.[botId] === undefined) return undefined
if (hasAnyGrant(state, botId)) return undefined // a Bot with any grant is judged as before
const key = handleKey(peer)
if (!key) return undefined
const room = Object.values(state.rooms ?? {}).find(candidate => candidate?.origin === 'face-to-face'
&& (candidate.members ?? []).includes(botId)
&& isCodeJoined(state, candidate.id, botId) // it came into that room by its number
&& remotesInRoom(candidate).some(seat => handleKey(seat.handle || seat.id) === key))
return room === undefined ? undefined : String(room.id)
}So a Bot with a home identity or any grant that names no room keeps today's
Strictness kept as asked: the exact seat only (never any Bot of that home), only while it is still a current participant ( At this tip the checks are: |
|
Verdict: PASS at a6b1759 No The previous two tips are gone for good reason, and this one is the fix: Ran (this review, on a6b1759): Each half carries its own red on a base that lacks the change, so the two are load-bearing separately, not only together. Two non-blocking notes on the consent test, both design questions rather than defects:
|
Written-by: Dev4 Owner: Dev4 Reviewer: Reviewer Fixes ThinkFlowLab#981 Tip SHA: dff89c7 Base: cc70b82 ## Reproduce On `ac880a5b`: one friend runs **Coder** and **Writer**, and only **Coder** has this Bot. Sharing → **Share with a friend** shows **no row at all** for that friend, and with the list empty the panel says "…or they already have this Bot" — true of Coder, false of Writer. The filter collected one key per friend *home* from this Bot's grants (`grantedHomes`), so the first granted Bot hides the whole person. The same fixture with the grant removed builds two rows, which is what makes it the filter and not the data. The other shape, found on review of the first tip: the friend's grant names **the person**, not one of their Bots (`grantee: '@bob.home'`). `memberHandleOf` reads a bare home back as if it were a handle, so that grant looked like a Bot grant and both of Bob's Bots were offered again; clicking one reaches `already_granted` and the receipt still says "Shared … with Bob." — a share that did not happen. ## What changed - `plugins/social/src/social.js` — `shareFriendsForBot` groups the grants by home and keeps the friend-Bot handle each grant names (`grantPersonView().member`). A row is skipped only when **its own Bot** is the granted one, so Writer is offered while Coder is not. The member counts as a Bot only when it differs from the home it belongs to; a grant that names the home is a person-level grant and keeps the old per-person answer, and a row with no handle stays hidden: neither can be told apart from the granted Bot here, and offering a Bot that already has it is the worse lie. `ThinkFlowLab#973`'s per-Bot rows, keys, busy state and the receipt are untouched. - `plugins/social/test/social.test.js` — two fixtures. The Bot-level one: one row for Writer naming it, the click's recorded `grant_bot_to_friend` carrying `friend_bot_id: pb-writer`, then the second grant added so the list is empty (the accepted "every Bot of theirs has it" case). The person-level one: Coder+Writer and a single `grantee: '@bob.home'` grant → **no rows**, which is the state that was wrong on `77f77a95`. - `docs/subsystems/social.md` — one clause: the share list filters per Bot too, and only a grant that names no Bot falls back to hiding the whole friend (ThinkFlowLab#981). - `changelog.d/981-share-list-per-bot.fixed.md`. The panel's empty sentence is unchanged, because it is now accurate: with the filter per Bot, an empty list means every Bot of every friend has this Bot. ## What I ran On this branch's tree (main `cc70b824` + these four files): - `plugins/social`: `DSH_HARNESS=/opt/flybot node --import ./test/resolve.mjs --test "test/*.test.js"` → **416 pass / 0 fail** (the whole plugin suite; it includes `ThinkFlowLab#972`'s and `ThinkFlowLab#948`'s cases, which landed while this branch was up). - `node .github/scripts/changelog.mjs check` → 65 fragments well formed. - `node scripts/ds-bot-patches.mjs check` (on the previous tip, which had the same `core/`): `skipped pin (no git)`, `ok 10 patches`, `ok core/ equals ds-bot/ + patches/ds-bot/` — this PR changes no `core/` file. ## Red without the source change - The person-level case, against the source of `77f77a95` (the tip that had the fix but not this comparison) with only the new fixture added: `not ok - ThinkFlowLab#981: a home-level grant hides the whole friend, not one Bot of theirs`, the listing came back as `["Coder (Bob)", "Writer (Bob)"]` → **1 pass / 1 fail** on the `ThinkFlowLab#981` pair. With the comparison in place: **2 pass / 0 fail**, whole suite 416/416. - The Bot-level case, on pristine sources with only the new test: `not ok 353 - ThinkFlowLab#981: a granted Bot of a two-Bot friend hides that Bot only, not the friend` → **411 pass / 1 fail**; the same suite with the fix → 412/412 (that run was on the previous base). ## Not run - `npm run verify` as a whole: `arch-docs` needs `git`, which this machine does not have. The owned page `docs/subsystems/social.md` is updated in this PR, so no `Docs-not-needed:` trailer is needed. - No browser run: the change is which rows the list returns, and the row rendering itself is `ThinkFlowLab#973`'s (covered there); the client file is not touched. - The earlier tips were built on `ac880a5b`, then `ec1086ab`, then `cc70b824`; `ThinkFlowLab#972` and `ThinkFlowLab#980` landed while this branch was up and both touch `social.md`, `social.js` and `social.test.js`, so all three were three-way merged against the new main and the tree above was rebuilt from `cc70b824`. This tip's commit sits directly on `cc70b824` and the diff is exactly these four files. ## Risk Two ways to get this wrong. One: showing a Bot that already has the grant again — possible only if a grant names a Bot by an id this code does not read, which the per-home fallback covers; a grant that names the person must not be read as a Bot, which is the case this tip adds. Two: hiding a Bot that does not have it — the conservative branches (person-level grant, no handle on the row) keep a whole friend hidden exactly as today. The ambiguous case is a friend whose Bot handle is spelled like their home: that reads as person-level and hides the friend, the same answer the old filter gave. `grantedHomes` had no other reader, so nothing else changed shape.
Owner: Dev
Reviewer: Reviewer
Fixes #948
Tip SHA: a6b1759
Written-by: Dev
What changed
#948is not a plugin bug, and the scope Chief approved is why this PR touchescore/: the refusal is{"by":"owner-policy"}, which isreach()incore/src/host/reach.js, andhooks.jsruns it before the plugin chain — instrumentingsendOut()showed it was never called at all.reach()reads a room only when the caller names one, and the id is worked out later, inside the transport. A room-only Bot has no person-grant, so it was refused by a policy that could only see grants.core/src/host/reach.js—sharedRoom(state, botId, peer)finds the one current face-to-face room where the exact target handle is still a remote seat, behind the same identity test the plugin uses forroomOnly: the Bot holds no grant at all (hasAnyGrantfalse) and came into that room by its number (isCodeJoined).rt.reachuses it when the message names no room, so the send is judged under the room that is the whole consent of that identity —reach()'s own allowance (inRoom+remoteInRoom), discovered instead of passed.patches/ds-bot/files/src/host/reach.js,patches/ds-bot/files/test/reach-policy.test.js— byte-identical mirrors (regenerated bygenerate's rule:files/is the whole file, and all 157 mirrored files matchcore/exactly).plugins/social/src/social.js—roomFor()/pairFor()resolve that room before the pair is judged (roomContextFor), treat a seat the roster does not list as the target it is, sign with the room-only identity (memberOf, only when a room was resolved) and name the room (room_id) so Bot Social checks the consent the host just checked.activeMember()is not relaxed: with no such room the refusal is the same sentence as today andsend_as_memberis never reached.core/test/reach-policy.test.js,plugins/social/test/social.test.js— the new cases.docs/subsystems/core-host.md,docs/subsystems/social.md,changelog.d/948-room-only-bot-1to1-line.fixed.md.Untouched:
reach()'s own rules and the explicit-roomIdbranches (-- :285-292byte for byte),ticketPeers/meta.scopematching,via: 'room'/'room-share', the approval and hold paths. A Bot that holds a grant is judged exactly as before.Rebased onto
90ff273d(currentmain, after#973) at Triage's request;#973touchedplugins/social/src/social.js,plugins/social/test/social.test.jsanddocs/subsystems/social.md, so those three were re-applied on top of it rather than carried over.Evidence
Red on
90f4bb5awith only the new tests in the tree (sources pristine):The negative control (
a Bot it shares no room with is refused as today) is green before and after, so what changed is the room, not the room-only rule. The core case pins what did not move, word for word: another Bot of that home, a seat that left, and a room of this home all answerA2 is not shared with that friend, so it cannot message their Bots. To allow it, share A2 with them in its settings → Sharing., and an explicitroomIdstill allows a room that is not face-to-face.Both halves are necessary, each shown by running this tree with the other half reverted:
#948by: "owner-policy",sendOut()never calledby: "social":@coder.bob.home is not a Bot a friend shared with you— a room seat is not on the rosterTwo regressions the earlier tips had, both found and fixed before this tip:
packagefailed at tip436921c7:core/test/reach-policy.test.js: differs from ds-bot/ + patches—core/test/is mirrored too. Fixed, and the 157-file mirror check above is how.7c4b0d2a:mentions (#143): a human line to a friend Bot already in the room stays in the room; Chief is not the sender(core/test/flybot.test.js:533), because discovery then applied to any Bot in a face room. It is now behind theroomOnlyidentity test (no grant + code-joined), and that test passes again.Ran
plugins/social:node --import ./test/resolve.mjs --test test/*.test.js→ 413/413 passcore:flybot.test.js+reach-policy+room-membership+room-ticket-seat+remote-room→ 152/152 pass#948cases alone: pass here, red on pristine90f4bb5apatches/ds-bot/files/are byte-identical to theircore/counterpartsnode .github/scripts/changelog.mjs check→ 63 fragments well formedNot run
node scripts/ds-bot-patches.mjs check/applyandnpm run verify: this box has nogitand nonode_modules, so the pin check, the patch applier andoxlintcannot run here. The git-free proof is the mirror check above plus the fact that no patch in the series editssrc/host/reach.js(its only mention iscore/index.js'simport * as reach from './src/host/reach.js'in0002). CI'spackagejob is the real run.coresuite here: it does not finish on this box; the five files above are the ones that readreach.jsor room reach.arch-docs: needsgit diff origin/main. Both owned paths changed here (core/src/host/reach.js+ its test →core-host;plugins/social/→social) have their doc updated in this PR.Risk
T2 or higher by the diff (it edits the owner policy). It can only find permission an explicit
roomIdwould already grant, and only for the identity that has nothing else: no grant, code-joined, the exact seat still in that face-to-face room. No grant is created, no state is written, no default changes; every refusal keeps its sentence.Scope
One host helper plus the
rt.reachwrapper, two plugin helpers and their two call sites, five tests, two doc sentences, one changelog fragment, two mirrors. One issue, one PR, one tip.