Draft
Conversation
Incorporates the multi-peer mapping approach from #31 (spooktheducks) with all three review fixes requested by @ajspig: 1. Concurrency protection: ensureInitialized() now uses a promise-based init lock to prevent races when two concurrent hooks enter init simultaneously. Errors propagate to all waiters (not swallowed). 2. Parallel peer resolution: capture.ts uses Promise.all() instead of sequential for...of + await for resolving human peers, avoiding latency bottleneck in group chats with many unique senders. 3. Naming clarity: session metadata keys renamed from humanPeerId/ humanPeerIds to humanSenderId/humanSenderIds to distinguish raw channel sender IDs from resolved Honcho peer IDs. Includes backward- compatible fallback to legacy key names. Closes #50. Supersedes #31. Co-authored-by: spooktheducks <spooktheducks@users.noreply.github.com> Co-authored-by: Minh Nguyen <menhguin@users.noreply.github.com>
- openclaw.plugin.json: restore disableDefaultNoisePatterns schema entry (config.ts parses it but JSON schema was missing, blocking validation) - helpers.ts: anchor extractSenderId() to first sentinel occurrence only, preventing user-pasted metadata blocks from poisoning attribution - hooks/capture.ts: remove raw sender IDs from debug logs (PII concern) - state.ts: serialize workspace metadata writes in getHumanPeer() and getAgentPeer() with a promise-based lock to prevent concurrent read-modify-write races - workspace_md/AGENTS.md: restored to main (had stale tool names from pre-#49 branch) Co-authored-by: Minh Nguyen <menhguin@users.noreply.github.com>
Per review feedback from @ajspig: the CLI setup simplification was out of scope for the multi-peer PR. Restored commands/cli.ts to match current main, retaining only the -p/--peer flag additions to 'honcho ask' and 'honcho search' subcommands. Co-authored-by: Minh Nguyen <menhguin@users.noreply.github.com>
Generalizes terminology: "participant" covers humans AND any non-agent
bots sharing a group chat, not just humans. Renames touch state.ts,
hooks, tools, CLI commands, and helpers; public helper signatures
change accordingly.
Adtl Behavioral changes:
- state.ts: metadata write lock now propagates errors to the caller
(previously swallowed), with logger.error on failure.
- runtime.ts: buildSessionTranscript and the memory search manager
now resolve the session's participant peer instead of always using
ownerPeer, and transcript speaker labels use isParticipantPeerId
to distinguish User(<id>) from generic Peer(<id>).
- hooks/capture.ts: writes participantSenderId/participantSenderIds.
- helpers.test.ts: new tests covering extractMessages participant
resolution; runtime.test.ts updated for the new label logic.
- README: participant-model wording, and a note that DMs on platforms
that emit sender metadata (Telegram) attribute to the sender peer
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughImplements a Multi-Peer Model replacing the single owner peer with per-sender participant peers. Extracts sender_id from inbound message metadata and creates distinct peers per sender. Updates tools, hooks, and state management to resolve and use session-participant-specific peers instead of a forced owner peer. Adds CLI peer selection support and configuration option for cross-session search scope. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant MessageParser as Message Parser<br/>(extractSenderId)
participant Capture as Capture Hook
participant State as State<br/>(participantPeers)
participant Tool as Tool<br/>(ask/search/context)
participant Peer as Honcho Peer
User->>CLI: Message with sender metadata
CLI->>Capture: Inbound message
Capture->>MessageParser: Extract sender_id from metadata
MessageParser-->>Capture: sender_id
Capture->>State: Resolve peer for sender_id<br/>via getParticipantPeer()
State->>State: Check participantPeers cache
State-->>Capture: Resolved Peer
Capture->>Capture: Build peerConfigs from resolved peers
Capture->>State: Store participantSenderId in<br/>session metadata
User->>CLI: honcho ask/search --peer <id>
CLI->>State: getParticipantPeer(id)
State->>State: Resolve from cache or create new
State-->>CLI: Participant Peer
CLI->>Tool: Execute with participant peer
Tool->>Peer: chat/search/context<br/>(target=participantPeer)
Peer-->>Tool: Results
Tool-->>CLI: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@runtime.test.ts`:
- Around line 140-147: The test doubles for getParticipantPeer and
resolveSessionParticipantPeer diverge from production: change
getParticipantPeer's mock to reference state.participantPeer (not the
closed-over participantPeer constant) so it reflects runtime mutation, have both
mocks throw the same error text matching the new participant model (e.g.,
"participant peer not initialized") when state.participantPeer is
null/undefined, and ensure resolveSessionParticipantPeer continues to return
state.participantPeer to mirror production behavior.
In `@state.ts`:
- Around line 152-156: peer creation is using the raw channelPeerId which can
collide with reserved/system peers; update the call that creates the Honcho peer
(the honcho.peer invocation where peer is assigned and stored in
state.participantPeers) to namespace or prefix the channelPeerId (e.g., prepend
"participant:" or similar) before passing it to honcho.peer and as the map key,
so use a deterministic namespacedId derived from channelPeerId for both the peer
id and for state.participantPeers.set to avoid collisions while preserving the
original channelPeerId in metadata.
- Around line 159-173: The function resolveSessionParticipantPeer currently
swallows all exceptions from honcho.session and session.getMetadata and then
falls back to getParticipantPeer(), which can misattribute data; change
resolveSessionParticipantPeer so it only falls back to the default when metadata
is present but senderId is absent, and do not catch and ignore errors from
honcho.session or session.getMetadata—either let those errors propagate (remove
the catch) or catch and rethrow/log and return null/undefined so callers can
handle failures; update the function signature/return handling and any callers
of resolveSessionParticipantPeer/getParticipantPeer to handle a null/undefined
error case instead of silently using the owner peer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55a15a81-86e1-402b-8cad-ffde40c2da0c
📒 Files selected for processing (4)
README.mdopenclaw.plugin.jsonruntime.test.tsstate.ts
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- openclaw.plugin.json
Comment on lines
+140
to
+147
| getParticipantPeer: vi.fn(async () => { | ||
| if (!participantPeer) throw new Error("Honcho owner peer not initialized"); | ||
| return participantPeer; | ||
| }), | ||
| resolveSessionParticipantPeer: vi.fn(async () => { | ||
| if (!state.participantPeer) throw new Error("Honcho owner peer not initialized"); | ||
| return state.participantPeer; | ||
| }), |
There was a problem hiding this comment.
Test double behavior diverges from production participant resolution.
Line 141 checks the closed-over participantPeer constant (never null), while Line 262 mutates state.participantPeer. This can hide null-state bugs. Also, the thrown/expected "owner peer not initialized" path does not match the new participant model.
💡 Suggested fix
getParticipantPeer: vi.fn(async () => {
- if (!participantPeer) throw new Error("Honcho owner peer not initialized");
- return participantPeer;
+ if (!state.participantPeer) throw new Error("Honcho participant peer not initialized");
+ return state.participantPeer;
}),
resolveSessionParticipantPeer: vi.fn(async () => {
- if (!state.participantPeer) throw new Error("Honcho owner peer not initialized");
+ if (!state.participantPeer) throw new Error("Honcho participant peer not initialized");
return state.participantPeer;
}),
@@
- ).rejects.toThrow(/owner peer not initialized/);
+ ).rejects.toThrow(/participant peer not initialized/);
@@
- ).rejects.toThrow(/owner peer not initialized/);
+ ).rejects.toThrow(/participant peer not initialized/);Also applies to: 260-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@runtime.test.ts` around lines 140 - 147, The test doubles for
getParticipantPeer and resolveSessionParticipantPeer diverge from production:
change getParticipantPeer's mock to reference state.participantPeer (not the
closed-over participantPeer constant) so it reflects runtime mutation, have both
mocks throw the same error text matching the new participant model (e.g.,
"participant peer not initialized") when state.participantPeer is
null/undefined, and ensure resolveSessionParticipantPeer continues to return
state.participantPeer to mirror production behavior.
Comment on lines
+152
to
+156
| // Use the channel peer ID directly as the Honcho peer ID — each participant | ||
| // is its own separate peer. | ||
| peer = await honcho.peer(channelPeerId, { metadata: { channelPeerId } }); | ||
| state.participantPeers.set(channelPeerId, peer); | ||
| return peer; |
There was a problem hiding this comment.
Namespace participant peer IDs to prevent collisions with agent/default peers.
Line 154 uses raw channelPeerId as the Honcho peer ID. A sender ID like owner or agent-main can collide with reserved/system peers and contaminate attribution.
💡 Suggested fix
- // Use the channel peer ID directly as the Honcho peer ID — each participant
- // is its own separate peer.
- peer = await honcho.peer(channelPeerId, { metadata: { channelPeerId } });
- state.participantPeers.set(channelPeerId, peer);
+ const normalizedChannelPeerId = channelPeerId.trim();
+ if (!normalizedChannelPeerId) return await getParticipantPeer();
+
+ const honchoPeerId = `participant-${normalizedChannelPeerId}`;
+ peer = await honcho.peer(honchoPeerId, {
+ metadata: { channelPeerId: normalizedChannelPeerId },
+ });
+ state.participantPeers.set(normalizedChannelPeerId, peer);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@state.ts` around lines 152 - 156, peer creation is using the raw
channelPeerId which can collide with reserved/system peers; update the call that
creates the Honcho peer (the honcho.peer invocation where peer is assigned and
stored in state.participantPeers) to namespace or prefix the channelPeerId
(e.g., prepend "participant:" or similar) before passing it to honcho.peer and
as the map key, so use a deterministic namespacedId derived from channelPeerId
for both the peer id and for state.participantPeers.set to avoid collisions
while preserving the original channelPeerId in metadata.
Comment on lines
+159
to
+173
| async function resolveSessionParticipantPeer(sessionKey: string): Promise<Peer> { | ||
| try { | ||
| const session = await honcho.session(sessionKey); | ||
| const meta = await session.getMetadata(); | ||
| if (meta && typeof meta === "object") { | ||
| const senderId = (meta as Record<string, unknown>).participantSenderId; | ||
| if (typeof senderId === "string" && senderId.length > 0) { | ||
| return await getParticipantPeer(senderId); | ||
| } | ||
| } | ||
| } catch { | ||
| // Fall through to default | ||
| } | ||
| return await getParticipantPeer(); | ||
| } |
There was a problem hiding this comment.
Avoid catch-all fallback that can misattribute participant data.
On Line 169, all errors are swallowed and Line 172 falls back to the default owner peer. If metadata/session fetch fails transiently, data can be attributed to the wrong participant.
💡 Suggested fix
async function resolveSessionParticipantPeer(sessionKey: string): Promise<Peer> {
- try {
- const session = await honcho.session(sessionKey);
- const meta = await session.getMetadata();
- if (meta && typeof meta === "object") {
- const senderId = (meta as Record<string, unknown>).participantSenderId;
- if (typeof senderId === "string" && senderId.length > 0) {
- return await getParticipantPeer(senderId);
- }
- }
- } catch {
- // Fall through to default
- }
+ const session = await honcho.session(sessionKey);
+ const meta = await session.getMetadata();
+ if (meta && typeof meta === "object") {
+ const senderId = (meta as Record<string, unknown>).participantSenderId;
+ if (typeof senderId === "string" && senderId.trim().length > 0) {
+ return await getParticipantPeer(senderId.trim());
+ }
+ }
return await getParticipantPeer();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@state.ts` around lines 159 - 173, The function resolveSessionParticipantPeer
currently swallows all exceptions from honcho.session and session.getMetadata
and then falls back to getParticipantPeer(), which can misattribute data; change
resolveSessionParticipantPeer so it only falls back to the default when metadata
is present but senderId is absent, and do not catch and ignore errors from
honcho.session or session.getMetadata—either let those errors propagate (remove
the catch) or catch and rethrow/log and return null/undefined so callers can
handle failures; update the function signature/return handling and any callers
of resolveSessionParticipantPeer/getParticipantPeer to handle a null/undefined
error case instead of silently using the owner peer.
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
1 participant
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 by CodeRabbit
Release Notes
New Features
--peer <id>option to CLI ask and search commands for targeted peer operationscrossSessionSearchconfiguration option (enabled by default) to control search scope across sessionsDocumentation
Migration Notes
Historical owner-attributed messages in existing sessions are not migrated. After merge, context queries against pre-existing sessions will show attribution mixed across the boundary — e.g.,
owner said X, then real-user Y said Z— because older turns retain their original owner attribution while new turns use per-sender peer IDs. This is acceptable but operators of existing deployments should be aware.