From dfda61a3cf029a3d2157e2a5a9005cb91ae4ea77 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 16 Oct 2026 20:20:17 +0800 Subject: [PATCH] =?UTF-8?q?fix(social):=20Codex=20review=20on=20#141=20?= =?UTF-8?q?=E2=80=94=20canonical=20grants,=20inbound=20backfill,=20scoped?= =?UTF-8?q?=20unhide?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Persist canonical @handles in core grants (not decorated Chief@…). - Seed inboundBots once from the existing remote roster so upgrade does not drop reciprocal friends before the outgoing-only filter. - Resolve Auto-send on respond from the pending relationship, not the first roster home. - Scope relationship_exists unhide to the invite's Bot/peer home. - Do not mark existing friend remotes as room-only; truncate onboard suffixes within the 32-char homeHandleOf limit. Co-authored-by: Danyang Chen --- docs/subsystems/social.md | 7 ++- plugins/social/src/mcp-client.js | 3 +- plugins/social/src/social.js | 89 +++++++++++++++++++++++++----- plugins/social/test/social.test.js | 80 +++++++++++++++++++++++++-- 4 files changed, 157 insertions(+), 22 deletions(-) diff --git a/docs/subsystems/social.md b/docs/subsystems/social.md index 22b65f5b9..9ca023dca 100644 --- a/docs/subsystems/social.md +++ b/docs/subsystems/social.md @@ -68,9 +68,10 @@ Everything in `plugins/social/` (entry `plugins/social/index.js` and any helpers - **#136:** A taken `displayName` at onboard retries `Name2` … rather than refusing to start. Friends' Bots shows `@your.home` with Copy. - **#137 (deferred):** The owner's chosen Bot avatar/color does not cross homes. Live Bot Social `update_bot_profile` only has `display_name` / `bot_id`; face-room join POST is `{ code, bots: [handle] }`. Peers keep the handle-seeded look until the server grows profile look fields. -- **#138:** Accepting a Helper invite grants Helper only. The joiner picks a talk-through Bot (not shared). Success copy is `{person} shared {Bot} with you.` Manage access lists `SelfB1935 (@selfb1935.home)`. Face-room remotes leave Friends' Bots when the room is left. -- **#139:** **Remove this Bot** asks once (`Remove Helper? Bob can share it again.`). Hide stays local (A's Manage access still shows the grant until A revokes — an incoming un-grant needs Bot Social). Re-joining an existing friendship unhides instead of a raw `relationship_exists`; used invites say “This invite was already used.” -- **#140:** Auto-send defaults off. The owner sees Approve / Reject in the chat, not “Waiting for the owner's approval.” Composer drafts are labelled You. +- **#138:** Accepting a Helper invite grants Helper only. The joiner picks a talk-through Bot (not shared). Success copy is `{person} shared {Bot} with you.` Manage access lists `SelfB1935 (@selfb1935.home)`. Core grants store a canonical `@handle` (never decorated `Chief@…`). On upgrade, remotes already on `list_remote_bots` are seeded into `inboundBots` once so reciprocal friends are not dropped. Face-room remotes leave Friends' Bots when the room is left; existing friend handles are not reclassified as room-only. +- **#139:** **Remove this Bot** asks once (`Remove Helper? Bob can share it again.`). Hide stays local (A's Manage access still shows the grant until A revokes — an incoming un-grant needs Bot Social). Re-joining an existing friendship unhides only that invite's Bot/peer home (not every hidden remote); used invites say “This invite was already used.” +- **#140:** Auto-send defaults off. Accepting a friend request uses that relationship's peer home for Auto-send, not the first roster entry. The owner sees Approve / Reject in the chat, not “Waiting for the owner's approval.” Composer drafts are labelled You. +- **#136 (suffix):** Onboard retries truncate the sanitized handle body so `Name2` / `Name3` still change within the 32-character `homeHandleOf` limit. ### Past rooms, unique titles, poll, and + (#129–#133) diff --git a/plugins/social/src/mcp-client.js b/plugins/social/src/mcp-client.js index 4b23ecfcd..ec1477f3a 100644 --- a/plugins/social/src/mcp-client.js +++ b/plugins/social/src/mcp-client.js @@ -212,7 +212,8 @@ export function normalizeGrant(item) { }) return { ...(id ? { id: String(id) } : {}), - grantee: String(grantee), + // Canonical @handle for core grants.set / normGrantee (not decorated Chief@…). + grantee: view.member || view.home || memberHandleOf(grantee) || String(grantee), ...(view.label ? { name: view.label } : {}), ...(view.home ? { home: view.home } : {}), ...(view.member ? { member: view.member } : {}), diff --git a/plugins/social/src/social.js b/plugins/social/src/social.js index 9e7bc5f64..aa566cc57 100644 --- a/plugins/social/src/social.js +++ b/plugins/social/src/social.js @@ -166,7 +166,13 @@ export const ONBOARD_TRIES = 12 export function nextOnboardDisplayName(base, attempt) { const name = String(base ?? 'FlyBot').trim() || 'FlyBot' - return attempt <= 1 ? name : `${name}${attempt}` + if (attempt <= 1) return name + const suffix = String(attempt) + // homeHandleOf keeps 32 alphanumeric chars; reserve space so the suffix changes the handle. + const sanitized = name.toLowerCase().replace(/[^a-z0-9]+/g, '') + const maxBase = Math.max(1, 32 - suffix.length) + if (sanitized.length <= maxBase) return `${name}${suffix}` + return `${sanitized.slice(0, maxBase)}${suffix}` } async function onboardError(response, displayName) { @@ -220,7 +226,14 @@ export function mapJoinInviteError(error) { const code = String(error?.code ?? '') const blob = `${code} ${msg}` if (/relationship_exists/i.test(blob)) { - return Object.assign(new Error("You're already friends. That Bot is in Friends' Bots again."), { code: 'relationship_exists' }) + return Object.assign(new Error("You're already friends. That Bot is in Friends' Bots again."), { + code: 'relationship_exists', + ...(error?.peer ? { peer: error.peer } : {}), + ...(error?.bot ? { bot: error.bot } : {}), + ...(error?.relationshipId || error?.relationship_id + ? { relationshipId: String(error.relationshipId ?? error.relationship_id) } + : {}), + }) } if (/invite is used|invite_used|already used|max_uses|used_up/i.test(blob)) { return Object.assign(new Error('This invite was already used. Ask your friend for a new one.'), { code: 'invite_used' }) @@ -548,7 +561,13 @@ export function createSocial(service, config = {}, { client: given, api: givenAp const key = norm(handle) const room = String(faceId ?? '') if (!key || key === '@' || !room) return - if (settings.inboundBots?.[key]) return + const peerHome = (() => { + const parts = key.slice(1).split('.') + return parts.length > 2 ? `@${parts.slice(1).join('.')}` : key + })() + if (settings.inboundBots?.[key] || settings.inboundBots?.[peerHome]) return + // Already a normal friend in the roster — do not reclassify as room-only. + if (listedRemoteBots().some(bot => bot.handle === key || bot.home === key || bot.home === peerHome)) return settings.roomBots ??= {} const rec = settings.roomBots[key] ?? { rooms: {} } if (rec.rooms?.[room]) return @@ -582,6 +601,20 @@ export function createSocial(service, config = {}, { client: given, api: givenAp } return keys } + // One-time upgrade: remotes already on the server roster are treated as inbound so + // reciprocal friends are not dropped when inboundBots was empty (Codex P1). + function backfillInboundFromRoster(mappedHomes) { + if (settings.inboundBackfilled) return + settings.inboundBots ??= {} + for (const bot of (mappedHomes ?? []).flatMap(entry => entry.bots ?? [])) { + const key = norm(bot.handle) + if (!key || key === '@') continue + if (settings.roomBots?.[key]) continue + settings.inboundBots[key] = true + } + settings.inboundBackfilled = true + saveSettings() + } function keepGrantedRemote(bot) { const key = norm(bot.handle) if (!key) return false @@ -629,6 +662,7 @@ export function createSocial(service, config = {}, { client: given, api: givenAp if (home) { result = await client.callTool('list_remote_bots', { home: home.id }) const mapped = mapRemoteHomes(result?.homes) + backfillInboundFromRoster(mapped) unfiltered = { homes: mapped.map(entry => ({ ...entry, bots: entry.bots.filter(bot => keepGrantedRemote(bot)) })) } } else { // Per-bot identity (legacy): each active friend is its own "home" of one Bot. @@ -787,9 +821,11 @@ export function createSocial(service, config = {}, { client: given, api: givenAp const roomScoped = (flybot.grants.of?.(bot.id) ?? []).filter(grant => grant.roomId) const people = listed.filter(grant => !grant.roomId).map(grant => { const view = grantPersonView(grant) + // Core normGrantee cannot match decorated non-@ values; persist a canonical @handle. + const canonical = view.member || view.home || memberHandleOf(grant.grantee) || grant.grantee return { - id: grant.id || grant.relationshipId || `${bot.id}:${grant.grantee}`, - grantee: grant.grantee, + id: grant.id || grant.relationshipId || `${bot.id}:${canonical}`, + grantee: canonical, ...(view.label ? { name: view.label } : grant.name ? { name: grant.name } : {}), ...(view.home ? { home: view.home } : {}), ...(view.member ? { member: view.member } : {}), @@ -1211,7 +1247,16 @@ export function createSocial(service, config = {}, { client: given, api: givenAp try { const result = await client.callTool('list_remote_bots', { home: home.id }) const homes = mapRemoteHomes(result?.homes) - const next = revealHiddenIfPresent(settings, presentRemoteHandles(homes)) + // Scope unhide to the invite's Bot/peer home — never every remote on the home. + const focus = norm(mapped.bot?.handle || mapped.peer?.handle) + const present = presentRemoteHandles(homes) + const handles = focus + ? present.filter(h => { + const homeKey = isHomeLevelHandle(focus) ? focus : homeOfPeer(focus) + return h === focus || h === homeKey || homeOfPeer(h) === homeKey + }) + : [] + const next = handles.length ? revealHiddenIfPresent(settings, handles) : settings if (next !== settings) { settings = next; saveSettings() } fetchedAt = 0 await remoteBots({ fresh: true }).catch(() => {}) @@ -1248,17 +1293,33 @@ export function createSocial(service, config = {}, { client: given, api: givenAp })) disposers.push(flybot.api.handle('social/respond', async ({ relationshipId, decision } = {}) => { if (decision !== 'accept' && decision !== 'decline') throw new Error('decision is accept or decline') - const result = await confirmedCall('respond_friend_request', { relationship_id: String(relationshipId ?? ''), decision, ...(decision === 'accept' ? { grant_capabilities: ['chat.public'] } : {}) }) + const rid = String(relationshipId ?? '') + let pendingPeer if (decision === 'accept') { - const before = new Set(listedRemoteBots().map(bot => bot.handle)) - const roster = await remoteBots({ fresh: true }).catch(() => remote) - for (const bot of (roster?.homes ?? []).flatMap(entry => entry.bots)) { - if (!before.has(bot.handle)) rememberInbound(bot.handle) + try { + const overview = await friendsOverview() + pendingPeer = (overview.incoming ?? []).find(item => String(item.relationshipId) === rid) + } catch (error) { + warn('could not load pending request for respond', error) } - const peerHome = roster?.homes?.find(entry => entry.bots?.length)?.home - if (autoAcceptOf(peerHome)) await serverAutoApprove(String(relationshipId ?? ''), true) } - return { relationshipId, status: result?.status ?? (decision === 'accept' ? 'active' : 'declined') } + const result = await confirmedCall('respond_friend_request', { relationship_id: rid, decision, ...(decision === 'accept' ? { grant_capabilities: ['chat.public'] } : {}) }) + if (decision === 'accept') { + const before = new Set(listedRemoteBots().map(bot => bot.handle)) + const roster = await remoteBots({ fresh: true }).catch(() => remote) + const added = (roster?.homes ?? []).flatMap(entry => entry.bots).filter(bot => !before.has(bot.handle)) + for (const bot of added) rememberInbound(bot.handle) + const resultPeer = result?.peer?.handle || result?.friend?.handle || result?.inviter?.handle + const peerHome = pendingPeer?.handle + ? (isHomeLevelHandle(pendingPeer.handle) ? pendingPeer.handle : homeOfPeer(pendingPeer.handle)) + : resultPeer + ? (isHomeLevelHandle(norm(resultPeer)) ? norm(resultPeer) : homeOfPeer(resultPeer)) + : added.length + ? added[0].home + : undefined + if (peerHome && autoAcceptOf(peerHome)) await serverAutoApprove(rid, true) + } + return { relationshipId: rid, status: result?.status ?? (decision === 'accept' ? 'active' : 'declined') } })) // The owner's composer for a remote member: text plus files, through bot.send (approval applies). diff --git a/plugins/social/test/social.test.js b/plugins/social/test/social.test.js index caaefc6dd..f11857f7a 100644 --- a/plugins/social/test/social.test.js +++ b/plugins/social/test/social.test.js @@ -31,7 +31,7 @@ function mockServer({ token = 'owner-token', friends = true } = {}) { create_invite: a => ({ code: 'INV123', url: 'https://s/i/INV123', status: 'active', expires_at: '2026-10-21T00:00:00Z', bot: a.bot_id, auto_accept: a.auto_accept }), join_invite: a => { if (a.code === 'USED') throw Object.assign(new Error('Bot Social join_invite: invite is used'), { code: 'invite_used' }) - if (a.code === 'EXISTS') throw Object.assign(new Error('Bot Social join_invite: relationship_exists'), { code: 'relationship_exists' }) + if (a.code === 'EXISTS') throw Object.assign(new Error('Bot Social join_invite: relationship_exists'), { code: 'relationship_exists', peer: { handle: '@bob.home', display_name: 'Bob' }, bot: { handle: '@coder.bob.home', display_name: 'Coder' } }) return a.code === 'NEEDSOK' ? { relationship_id: 'r9', status: 'pending_requester_owner', awaiting: 'your_owner_approval', approval_id: 'apr9', inviter: { handle: 'eve.home', display_name: 'Eve' }, bot: { handle: '@helper.eve.home', display_name: 'Helper' } } : { relationship_id: 'r8', status: 'active', awaiting: null, inviter: { handle: 'bob.home', display_name: 'Bob' }, bot: { handle: '@coder.bob.home', display_name: 'Coder' } } @@ -3121,6 +3121,11 @@ test('#123: approval waiting notice skips a home handle with no friend Bot and d test('#136: onboard retries a taken display name and then starts', async () => { assert.equal(nextOnboardDisplayName('FlyBot', 1), 'FlyBot') assert.equal(nextOnboardDisplayName('FlyBot', 2), 'FlyBot2') + const long = 'a'.repeat(40) + assert.equal(nextOnboardDisplayName(long, 1), long) + assert.equal(nextOnboardDisplayName(long, 2).length, 32) + assert.equal(homeHandleOf(nextOnboardDisplayName(long, 2)), `@${'a'.repeat(31)}2.home`) + assert.notEqual(homeHandleOf(nextOnboardDisplayName(long, 2)), homeHandleOf(nextOnboardDisplayName(long, 3))) const names = [] const result = await onboardHome({ server: 'https://social.test', @@ -3154,10 +3159,11 @@ test('#136 / #138: grant labels use the person home, not a raw member handle', ( const normalized = normalizeGrant({ grantee: 'Chief@chief.bob.home', display_name: 'Chief@chief.bob.home' }) assert.equal(normalized.home, '@bob.home') assert.equal(normalized.member, '@chief.bob.home') + assert.equal(normalized.grantee, '@chief.bob.home', 'core grants need a canonical @handle, not decorated text') }) test('#138: Friends\' Bots drops the joiner\'s Bot from an outgoing invite', async (t) => { - const { ctx, call, stop } = await team() + const { ctx, call, home, stop } = await team() t.after(stop) const server = mockServer() server.tools.list_bot_grants = () => ({ @@ -3173,7 +3179,10 @@ test('#138: Friends\' Bots drops the joiner\'s Bot from an outgoing invite', asy ], }], }) - const social = createSocial(ctx.flybot, { autoAccept: false, inboxPollMs: 0 }, { + // Migration already done: empty inboundBots so the outgoing-only filter applies to new leaks. + const settingsPath = join(home, 'flybot-social-settings.json') + writeFileSync(settingsPath, JSON.stringify({ inboundBackfilled: true, inboundBots: {} })) + const social = createSocial(ctx.flybot, { autoAccept: false, inboxPollMs: 0, settingsPath }, { client: createMcpClient({ server: 'https://s', token: 'owner-token', fetch: server.fetch }), home: { id: 'h1', handle: '@danyang.home' }, }) @@ -3186,6 +3195,8 @@ test('#138: Friends\' Bots drops the joiner\'s Bot from an outgoing invite', asy const access = (await call('flybot/social/access', { botId: 'a' })).value assert.ok(access.grants.some(g => g.name === 'Bob (@bob.home)' || g.home === '@bob.home')) assert.equal(access.grants.some(g => /Chief@chief/.test(g.name || '') || /Chief@chief/.test(g.grantee || '')), false) + const coreGrant = ctx.flybot.grants.of('a').find(g => !g.roomId) + assert.equal(coreGrant?.grantee, '@chief.bob.home', 'canonical member handle stored in core grants') }) test('#138: joining an invite marks that Bot inbound and does not auto-send', async (t) => { @@ -3211,6 +3222,12 @@ test('#139: join errors are English and relationship_exists unhides', async (t) const { ctx, call, home, stop } = await team() t.after(stop) const server = mockServer() + server.tools.list_remote_bots = () => ({ + homes: [ + { relationship_id: 'r1', home: { bot_id: 'h2', handle: '@bob.home', display_name: 'Bob', owner_display_name: 'Bob' }, bots: [{ bot_id: 'x1', handle: '@coder.bob.home', display_name: 'Coder' }, { bot_id: 'x2', handle: '@writer.bob.home', display_name: 'Writer' }] }, + { relationship_id: 'r9', home: { bot_id: 'h9', handle: '@eve.home', display_name: 'Eve', owner_display_name: 'Eve' }, bots: [{ bot_id: 'x9', handle: '@helper.eve.home', display_name: 'Helper' }] }, + ], + }) const settingsPath = join(home, 'flybot-social-settings.json') const social = createSocial(ctx.flybot, { autoAccept: false, inboxPollMs: 0, settingsPath }, { client: createMcpClient({ server: 'https://s', token: 'owner-token', fetch: server.fetch }), @@ -3221,6 +3238,7 @@ test('#139: join errors are English and relationship_exists unhides', async (t) await social.registerAll() await call('flybot/social/remote-bots', { fresh: true }) await call('flybot/social/hide-remote', { handle: '@coder.bob.home' }) + await call('flybot/social/hide-remote', { handle: '@helper.eve.home' }) const used = await call('flybot/social/join-invite', { code: 'USED' }) assert.equal(used.ok, false) assert.match(used.error.message, /already used/i) @@ -3230,7 +3248,8 @@ test('#139: join errors are English and relationship_exists unhides', async (t) assert.match(exists.error.message, /already friends/i) assert.doesNotMatch(exists.error.message, /relationship_exists/) const again = (await call('flybot/social/remote-bots', { fresh: true })).value - assert.equal(again.homes[0].bots.some(bot => bot.handle === '@coder.bob.home'), true) + assert.equal(again.homes.some(h => h.bots.some(bot => bot.handle === '@coder.bob.home')), true) + assert.equal(again.homes.some(h => h.bots.some(bot => bot.handle === '@helper.eve.home')), false, 'unrelated hidden Eve stays hidden') assert.equal(mapJoinInviteError(Object.assign(new Error('relationship_exists'), { code: 'relationship_exists' })).message.includes('already friends'), true) }) @@ -3276,6 +3295,59 @@ test('#140: new friends default Auto-send off; settings report false', async (t) assert.equal(social.approvalFor('a', '@bob.home'), 'hold') }) +test('#138 upgrade: backfill inbound keeps reciprocal remotes already on the roster', async (t) => { + const { ctx, call, home, stop } = await team() + t.after(stop) + const server = mockServer() + server.tools.list_bot_grants = () => ({ + grants: [{ id: 'g-rev', grantee: '@chief.bob.home', relationship_id: 'r1' }], + }) + server.tools.list_remote_bots = () => ({ + homes: [{ + relationship_id: 'r1', + home: { bot_id: 'h2', handle: '@bob.home', display_name: 'Bob' }, + bots: [ + { bot_id: 'x1', handle: '@chief.bob.home', display_name: 'Chief' }, + { bot_id: 'x2', handle: '@coder.bob.home', display_name: 'Coder' }, + ], + }], + }) + const settingsPath = join(home, 'flybot-social-settings.json') + // Fresh upgrade: no inbound markers yet. + writeFileSync(settingsPath, JSON.stringify({})) + const social = createSocial(ctx.flybot, { autoAccept: false, inboxPollMs: 0, settingsPath }, { + client: createMcpClient({ server: 'https://s', token: 'owner-token', fetch: server.fetch }), + home: { id: 'h1', handle: '@danyang.home' }, + }) + t.after(() => social.dispose()) + await grantTeam(ctx.flybot) + await social.registerAll() + const listed = (await call('flybot/social/remote-bots', { fresh: true })).value + assert.equal(listed.homes[0].bots.some(bot => bot.handle === '@chief.bob.home'), true, 'reciprocal Bot kept on upgrade backfill') + assert.equal(listed.homes[0].bots.some(bot => bot.handle === '@coder.bob.home'), true) + const saved = JSON.parse(readFileSync(settingsPath, 'utf8')) + assert.equal(saved.inboundBackfilled, true) + assert.equal(saved.inboundBots['@chief.bob.home'], true) +}) + +test('#140: respond Auto-send follows the accepted relationship, not the first roster home', async (t) => { + const { ctx, call, stop } = await team() + t.after(stop) + const server = mockServer() + // Bob already a friend with Auto-send on; accepting Carol must not inherit Bob's setting. + const social = createSocial(ctx.flybot, { inboxPollMs: 0, autoAccept: false }, { + client: createMcpClient({ server: 'https://s', token: 'owner-token', fetch: server.fetch }), + home: { id: 'h1', handle: '@danyang.home' }, + }) + t.after(() => social.dispose()) + await social.remoteBots({ fresh: true }) + await call('flybot/social/auto-accept', { home: '@bob.home', on: true }) + server.calls.length = 0 + await call('flybot/social/respond', { relationshipId: 'r5', decision: 'accept' }) + const auto = server.calls.filter(c => c.name === 'set_auto_approve' && c.args?.relationship_id === 'r5') + assert.equal(auto.some(c => c.args.auto_approve_chat === true), false, 'Carol must not get Bob\'s Auto-send') +}) + test('createSocialApi prefers list_bot_grants', async () => { const server = mockServer() server.tools.list_bot_grants = () => ({ grants: [{ id: 'g-bot', grantee: '@carol.home', name: 'Carol' }] })