fix(agent-room): rewrite migration to RESTORE 1:1 DMs, not relabel as chat#235
Merged
fix(agent-room): rewrite migration to RESTORE 1:1 DMs, not relabel as chat#235
Conversation
Contributor
Author
|
Self-review pass via code-reviewer subagent — addressed all findings in
Tests now 6/6 (was 5/5; +1 for the ghost-host edge case). All other findings from #232 already in scope and addressed there. CI status incoming via the Monitor. |
… chat PR #232's migration script converted any agent-room with >2 members to type: 'chat'. Investigating the actual offenders on dev showed every multi-member agent-room was an originally-1:1 DM (1 host agent + 1 human) polluted with rogue agents that auto-joined via ensureAgentInPod — exactly the bug PR #232's runtime guards close going forward. The relabel-to-chat strategy legitimized that pollution; restoration is the correct action. New migration logic: for each agent-room with members.length > 2: humans = members where User.isBot === false case humans.length === 1: keep [createdBy (host agent), the_human]; drop rogue agents stays type='agent-room' case humans.length === 0: agent↔agent DM. Trust insertion order — getOrCreateAgentRoom creates pods with [hostAgent, otherParty]; later auto-joins append. Keep [members[0], members[1]]; drop the rest. stays type='agent-room' case humans.length >= 2: Was never a DM (multi-human surfaces are chat pods, not DMs). type → 'chat'. All members preserved. 5 unit tests cover all three branches + dry-run + idempotency, mocking Pod + User so they run without a real Mongo connection. Also: tighten stale comment on POST /api/agents/runtime/room — was still documenting the rejected "many humans × one agent office" framing. Now matches ADR-001 §3.10 and notes that the endpoint accepts only human JWT (agent↔agent DM creation has no agent-runtime endpoint yet — file a follow-up if needed). The previous migration script was never run on dev, so swapping in the new logic doesn't require any rollback or compensating action. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two important items + several nits/questions from the code-reviewer pass.
1. Defensive guard: if `pod.createdBy` is NOT actually in `pod.members`
(data corruption / bulk-import that bypassed the create hook), the
human↔agent branch would silently produce a 2-member pod where
hostId is a ghost — no AgentInstallation matches, no User session
exists. Now skips the pod with a warning so an operator can triage.
Test pins the behavior.
2. Orphan handling documented: a member ID with no User row returns
undefined from isBotById.get(). Such orphans aren't counted as
humans and aren't counted as bots. The script's behavior is now
spelled out in a comment so a reader doesn't have to reverse-engineer
it from the filter.
3. Insertion-order claim now cites the evidence: dmService.ts:290
creates pods with [hostAgent, otherParty]; the only mutation paths
use Mongoose .push() (append-only); no $set:{members:[...]} reorders.
Verified via repo-wide grep at PR-time.
4. Misleading dedup comment removed (it conflated which branch's keepIds
could collide; the de-dupe is just defensive).
5. Test for the agent↔agent branch now explicitly asserts pod.type
stays 'agent-room' and uses arrayContaining for the unordered
dropIds set (still uses toEqual for the ordered keepIds since order
is part of the contract).
6/6 tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
a3ef495 to
8f0dd06
Compare
3 tasks
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.
Summary
Follow-up to #232. The previous migration script converted any
agent-roompod with >2 members totype: 'chat'. Inspecting the actual offenders onapi-devshowed every multi-member agent-room was an originally-1:1 DM polluted with rogue agents that auto-joined viaensureAgentInPod— exactly the bug PR #232's runtime guards close going forward. Relabel-to-chat legitimized the pollution; restoration is the correct action.Live data that informed this
The three offending pods on dev all had the same shape: 1 host agent + 1 human + N rogue agents.
commonly-bot(1)commonly-botls111openclaw-{fakesam, tarik, ai-citation-strategist, tom}commonly-bot(2)commonly-botxcjsamopenclaw (fakesam)openclaw-fakesamxcjsamopenclaw-{ai-citation-strategist, tom, tarik}None of these were ever multi-party chats. They were 1:1 DMs with intruders. Promoting them to
chatwould have made the bug permanent.New migration logic
Per ADR-001 §3.10, agent-room is exactly 2 members; the pair can be
human + agentORagent + agent, never 3+.Also fixed
backend/routes/agentsRuntime.ts:516— stale comment onPOST /api/agents/runtime/roomwas still documenting the rejected "many humans × one agent office" framing. Now matches §3.10 and notes that the endpoint accepts only human JWT (agent↔agent DM creation has no agent-runtime endpoint yet — file a follow-up if needed).Tests
backend/__tests__/unit/scripts/migrate-agent-room-multimember.test.js— 5 tests covering all three branches + dry-run + idempotency. Pod + User mocks so no real Mongo connection needed.21/21 across the migration test + the inherited podController + ensureAgentInPod suites from #232.
Rollback safety
The previous migration script (#232) was never run on
dev, so swapping in this rewrite doesn't require any rollback or compensating action. The runtime guards in #232 already prevent new pollution.Test plan
kubectl exec ... migrate-agent-room-multimember.ts --dryagainst dev — review the plan against the live data before committing--dry, verifycommonly pod list --instance devshows the three offending pods now have 2 members each and stayedagent-roomtype🤖 Generated with Claude Code