Repository navigation
fix(social): never let two local Bots share one Bot Social member identity - #24
Conversation
…ts match the sources
…ntity Bots with non-ASCII names (研究员, 后端, …) were once all slugged to 'bot', and the server hands back the existing member when a handle is taken in the home, so they all ended up holding @bot.danyang.home. Copy link on 研究员 then created an invite for that shared identity, and the joining home saw it under the last name registered (后端). - registerOnce: if the member returned is held by another local Bot, register again with the Bot's id-suffixed name. - registerAll: repair saved shared identities at startup; the first Bot keeps it, the others get their own. - Tests: the mock server now reuses taken handles like the real one.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a256b464b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (owners.has(id)) { | ||
| warn(`${bot.name} shared ${owners.get(id).name}'s Bot Social identity; registering its own`) | ||
| flybot.fields.set(bot.id, 'member', undefined) | ||
| } else owners.set(id, bot) |
There was a problem hiding this comment.
When upgrading a team with the legacy shared-ID state, the shared server member was renamed on every historical add_home_member, so it normally has the last colliding Bot's display name (the added fixture models m0 as 后端). This line selects the first listed Bot to keep that identity without touching the server, while only later Bots re-register; an invite for the keeper is therefore still presented to the friend under the other Bot's name. Re-register or rename the retained handle for its selected owner as part of the repair.
Useful? React with 👍 / 👎.
| if (member && heldByOther(bot.id, member.bot_id)) { | ||
| member = await client.callTool('add_home_member', { home: home.id, name: suffixed, display_name: bot.name }) |
There was a problem hiding this comment.
When a new Bot's normalized name resolves to an existing local handle, the add_home_member call just completed at line 215 has already renamed the incumbent server member to the new Bot's display_name (the revised mock explicitly models this). Retrying with the suffix gives the newcomer a distinct ID but never restores the incumbent, so two differently named Bots with the same slug leave friends seeing the first Bot under the second Bot's name. Avoid the side-effecting primary call when its handle is held locally, or rename the incumbent back.
Useful? React with 👍 / 👎.
| if (!id) continue | ||
| if (owners.has(id)) { | ||
| warn(`${bot.name} shared ${owners.get(id).name}'s Bot Social identity; registering its own`) | ||
| flybot.fields.set(bot.id, 'member', undefined) |
There was a problem hiding this comment.
When a second DSH starts read-only with legacy duplicates, this write executes before register() reaches its read-only guard. fields.set removes the member from that host's in-memory roster and emits a change even though no registration can occur; after lock takeover registerAll() is not run again, so the on-disk duplicate is merely reloaded and remains unfixed until restart. Return before this migration when flybot.readOnly() is true, as polling and registration already do.
Useful? React with 👍 / 👎.
…b#401) (ThinkFlowLab#1155) Owner: Dev2 Reviewer: Reviewer Tip SHA: 0376e73 Fixes ThinkFlowLab#401 Related ThinkFlowLab#1154 ## Reproduce A finished task is a *Needs you* card in the Inbox with one action, **Open**. The host has `task-accept-result` (`core/src/host/tasks.js`) but nothing calls it — `core/src/client/index.js` exposes no `acceptTaskResult` and `inbox.js` draws no result action — so the requester cannot accept from the card. There is no send-back endpoint at all: the only way to reject a result is `task-cancel`, which throws the work away instead of telling the Bot what to change. A result also cannot carry evidence (`task-return` stores `{kind, summary}` only), and the card shows neither the evidence nor which Bot/model produced it. Tests are the three new cases: `core/test/tasks.test.js` ThinkFlowLab#27 (`a returned result carries at most five http(s) links…`) and ThinkFlowLab#28 (`the requester sends a result back with a note…`), `core/test/inbox.test.js` ThinkFlowLab#24 (`a result card is accepted or sent back in place…`). ## What changed - `core/src/host/tasks.js` — `result-ready` may now also go to `working`; `task-return` reads evidence links (`readLinks`: at most five `http(s)` URLs, deduped, in order, the rest dropped; a non-list refused **before** the transition so a malformed write leaves the task where it is) into `result.links`; new `task-send-back` needs a note (1..500, `bot/invalid` without one), requires `result-ready`, moves the same task to `working` with `followUp: {note, at}` and the `by: 'sent back'` actor entry. - `core/src/host/authz.js`, `core/src/host/channel.js` — `task-send-back` in the explicit allow-list and in the channel's `task-*` case list. - `core/src/client/index.js` — `acceptTaskResult(id)` / `sendBackTask(id, note)`. - `core/src/client/inbox.js` — `inboxAnswer` answers a `status: 'result'` card with `'accept'` or `{note}`; a `ResultActions` row (**Send back** → one-line box `What should change` → **Send it back**, plus **Accept**), the box kept in a card-keyed slot so a roster or clock poll does not close it (same shape as ThinkFlowLab#1140's short answer); `task.result.links` drawn as evidence links; `task.followUp.note` drawn as *Sent back: …*; the producing Bot's model added to the card's meta line; four CSS rules. - `core/src/client/i18n.js` — Chinese copy for the five new strings, so `core/test/i18n.test.js`'s "every literal the client translates has a Chinese entry" still holds. - `core/client.js` — rebuilt from `core/src/client/` (`node scripts/build-client.mjs`). - `patches/ds-bot/` — the series regenerated for the touched files (`files/` verbatim copies; `0001`/`0003`/`0005`/`0008` sections rewritten by `diff -u`, see Risk). - `docs/subsystems/core-host.md` (endpoints, `result.links`, `task-send-back`), `docs/subsystems/core-client.md` (the inbox row and a *Done is the acceptance surface* invariant), `docs/subsystems/team.md` (channel allow-list). - `changelog.d/401-accept-or-send-back-a-result.added.md` — user-visible. Not in this PR: writing the verdict back to the issue/PR the task came from (the other half of the owner's phase 1 — a task has no pointer to an issue/PR and the host has no GitHub write path). Tracked as ThinkFlowLab#1154, so **judge this tip as the card + record half**, not as phase 1 complete. ## What I ran On this tip, base `75ca7ab9`, no `git` on this box: - `cd core && node --import ./test/harness-resolve.mjs --test --test-concurrency=1 test/tasks.test.js test/inbox.test.js test/i18n.test.js test/at-menu.test.js test/reach-policy.test.js test/bot-tasks.test.js test/kanban.test.js` → `# tests 88 / pass 88 / fail 0 / skipped 0` - `cd core && node scripts/build-client.mjs && node scripts/build-client.mjs --check` → exit 0 both - `node scripts/ds-bot-patches.mjs check` → `skipped pin (no git): the tree hash cannot be computed here`, `ok 10 patches apply cleanly in order`, `ok core/ equals ds-bot/ + patches/ds-bot/` - `cd core && DSH_HARNESS=… node scripts/lint.mjs` → `Found 51 warnings and 0 errors.` (the same 51 as base `75ca7ab9`; none in the files this PR touches), then `arch-docs: FAIL — cannot diff against origin/main` (no git here) - `node scripts/lint-plugins.mjs` → ok - `node .github/scripts/changelog.mjs check` → `39 fragment(s) in changelog.d/ are well formed` - `diff -rq` against a `75ca7ab9` tarball → exactly the 22 files of this PR, nothing else (no ghost) ## Red without the source change - Host: copy base `75ca7ab9`'s `core/src/host/tasks.js` over this tip's → `not ok 27` and `not ok 28`, `# pass 26 / # fail 2`; restored, `cmp` identical. - Client: copy base `core/src/client/inbox.js` over this tip's → `not ok 24` (`a result card is accepted or sent back in place…`), `# pass 23 / # fail 1`; restored, `cmp` identical, `ds-bot-patches check` still both `ok`. ## Not run - `npm run verify` end to end: its `arch-docs` step needs `git` and `origin/main`, which this box does not have. Both halves were run by hand (above); CI's `tests-ok` is the terminal answer. - The whole suite on this exact tip is in the body above; the eight failures it reports are the same eight a pristine `75ca7ab9` reports on this box (see the comment below). ## Risk - `patches/ds-bot/0008-client-bundle.patch` is rewritten with `diff -u` rather than `git diff` (no git here), so its hunk boundaries differ from git's even though the content is the same: `check` proves `core/client.js` still equals `ds-bot/ + patches`. The next Bot that regenerates this file with git will rewrite that section — the same shape as ThinkFlowLab#1141's regeneration. - A sent-back task keeps its previous `result` on the record (deliberate: the note points at what was already there; the next `task-return` overwrites it). - `task-send-back` is a new channel name: an older host answers `bot/not-found` and the client shows that error on the card; an older client never calls it.
Copy link on 研究员 created an invite for
@bot.danyang.home, and the friend who joined got 后端.Root cause: an older slug turned every non-ASCII Bot name into
bot. The server'sadd_home_memberhands back (and renames) the existing member when a handle is already taken in the home. So 研究员, 后端 and seven other Bots all saved the same member identity. The invite named that shared identity, and the joining home saw it under the last name registered (后端).Fix:
registerOnce: if the returned member is already held by another local Bot, register again with the id-suffixed name.registerAll: repair saved shared identities at startup. The first Bot keeps the identity; the others register their own.Tests: two regression tests, both failing without the fix. ds-bot suite: 633/633 pass.