Open
Conversation
WalkthroughThis PR replaces the single global ownerPeer with a multi-peer model: per-session/channel human peers plus per-agent peers. It adds Changes
Sequence DiagramsequenceDiagram
participant Batch as Message Batch
participant Extract as extractSenderId
participant State as PluginState
participant Honcho as Honcho (Peers)
participant Session as Session/Tool
Batch->>Extract: provide rawContent with metadata block
Extract->>Extract: parse fenced JSON, return sender_id
Extract->>State: resolve/getHumanPeer(sender_id)
State->>State: check humanPeers cache / humanPeerMap
alt cached
State-->>Extract: return existing human peer
else create
State->>Honcho: create new human peer
Honcho-->>State: peer created
State-->>Extract: cache and return new peer
end
Extract->>Session: attach resolved peer to message
Session->>Honcho: use humanPeer and agentPeer for chat/representation/observe
Honcho-->>Session: responses routed per resolved peers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
spooktheducks
force-pushed
the
feat/multi-peer-mappings
branch
from
0360758 to
a3f828e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
config.ts (1)
37-45: Normalize mapping IDs during config parsing.
parseStringRecord()currently keeps leading/trailing whitespace and silently drops malformed entries. Because missing mappings fall back to default or auto-created peer IDs, a stray space or bad value here can split memory across different peers and be hard to diagnose. Trim both sides before the emptiness check, and consider failing fast on invalid entries.♻️ Possible normalization
function parseStringRecord(raw: unknown): Record<string, string> { if (!raw || typeof raw !== "object" || Array.isArray(raw)) return {}; const result: Record<string, string> = {}; - for (const [k, v] of Object.entries(raw as Record<string, unknown>)) { - if (typeof k === "string" && typeof v === "string" && k.length > 0 && v.length > 0) { - result[k] = v; + for (const [rawKey, rawValue] of Object.entries(raw as Record<string, unknown>)) { + if (typeof rawValue !== "string") continue; + const key = rawKey.trim(); + const value = rawValue.trim(); + if (key.length > 0 && value.length > 0) { + result[key] = value; } } return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config.ts` around lines 37 - 45, parseStringRecord currently keeps leading/trailing whitespace and silently drops malformed entries; update it to trim both key and value before validating emptiness (use trimmedK/trimmedV for checks and insertion) and change failure behavior to fail fast on invalid entries (e.g., throw an Error or surface a clear log message) instead of silently ignoring them; update the loop over Object.entries(raw as Record<string, unknown>) and the result assignment to use the trimmed strings and add a validation branch that raises an explicit error when encountering non-string values or empty strings after trimming so misconfigurations are detected early.state.ts (1)
113-142: Potential race condition on concurrent peer creation.If two requests with the same new
channelPeerIdarrive concurrently before either completes, both will miss the cache check (line 125-126), create duplicate Honcho peer requests, and both will attempt to persist the mapping. While the outcome is functionally correct (last write wins, peer is idempotent), this creates unnecessary API calls and metadata writes.Consider adding a pending-creation map to deduplicate in-flight requests:
♻️ Optional: Deduplicate concurrent peer creation
+ const pendingPeers = new Map<string, Promise<Peer>>(); + async function getHumanPeer(channelPeerId?: string): Promise<Peer> { if (!channelPeerId) { // Return default owner peer let peer = state.humanPeers.get(OWNER_ID); if (!peer) { peer = await honcho.peer(OWNER_ID, { metadata: {} }); state.humanPeers.set(OWNER_ID, peer); } return peer; } // Check cache let peer = state.humanPeers.get(channelPeerId); if (peer) return peer; + // Check pending creation + const pending = pendingPeers.get(channelPeerId); + if (pending) return pending; + + const createPeer = async (): Promise<Peer> => { // Resolve honcho peer ID from mapping or use channel peer ID directly let honchoId = state.humanPeerMap[channelPeerId]; if (!honchoId) { honchoId = channelPeerId; // Persist auto-created mapping state.humanPeerMap[channelPeerId] = honchoId; const wsMeta = await honcho.getMetadata(); await honcho.setMetadata({ ...wsMeta, humanPeerMap: state.humanPeerMap }); api.logger.info(`[honcho] Auto-created human peer mapping: "${channelPeerId}" → "${honchoId}"`); } - peer = await honcho.peer(honchoId, { metadata: { channelPeerId } }); - state.humanPeers.set(channelPeerId, peer); - return peer; + const p = await honcho.peer(honchoId, { metadata: { channelPeerId } }); + state.humanPeers.set(channelPeerId, p); + return p; + }; + + const promise = createPeer().finally(() => pendingPeers.delete(channelPeerId)); + pendingPeers.set(channelPeerId, promise); + return promise; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@state.ts` around lines 113 - 142, getHumanPeer currently has a race where concurrent calls for the same new channelPeerId can each miss state.humanPeers and create duplicate honcho.peer and honcho.setMetadata calls; add a pending-creation dedupe: introduce a Map<string, Promise<Peer>> (e.g., pendingHumanPeers) checked immediately after the cache miss, store the in-flight Promise before any await (so subsequent callers return the same Promise), perform the honcho.peer/honcho.setMetadata work inside that Promise, and finally remove the entry from pendingHumanPeers and set state.humanPeers once resolved; keep existing OWNER_ID short-circuit unchanged.hooks/capture.ts (1)
13-33: Minor duplication withhelpers.tscontent extraction.
getRawContentduplicates the content extraction logic inextractMessages(helpers.ts lines 222-236). Consider extracting a shared helper if this pattern appears elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/capture.ts` around lines 13 - 33, getRawContent duplicates the message content extraction already implemented in extractMessages (helpers.ts); refactor by extracting the shared logic into a single exported helper (e.g., extractRawTextFromMessage or normalizeMessageContent) in helpers.ts (or a new utils module), update getRawContent to call that helper, and replace other similar extraction sites to import and reuse the new helper; ensure the helper accepts unknown/msg: unknown and preserves current behavior for string, array-of-blocks, and fallback to "" so callers like getRawContent and extractMessages remain functionally identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commands/cli.ts`:
- Around line 264-265: The CLI ask/search commands call state.getHumanPeer()
with no argument, which falls back to the owner peer and prevents targeting a
specific session human; update the CLI handlers that call agentPeer.chat (the
lines using const humanPeer = await state.getHumanPeer(); const answer = await
agentPeer.chat(question, { target: humanPeer }); and the similar block around
lines 280-281) to accept an optional peer selector flag/argument (e.g., --peer
or --human) and pass that identifier into state.getHumanPeer(peerId) so the
selected peer is resolved and then call agentPeer.chat(question, { target:
humanPeer }) with the resolved peer; ensure the CLI help text and parsing
include the new flag and preserve the fallback behavior when the flag is
omitted.
In `@hooks/capture.ts`:
- Around line 143-151: The current code only writes lastSenderId into session
metadata as humanPeerId, losing information about other resolved participants;
update the metadata write (the updatedMeta object where humanPeerId is set) to
persist all resolved human participant peers instead—e.g., add a humanPeerIds
(or humanPeers) field containing the list/map of resolved peer IDs from the same
data used to build peerConfigMap (or the keys/values of peerConfigMap) so
resolveSessionHumanPeer and related consumers can access every participant
rather than just lastSenderId.
---
Nitpick comments:
In `@config.ts`:
- Around line 37-45: parseStringRecord currently keeps leading/trailing
whitespace and silently drops malformed entries; update it to trim both key and
value before validating emptiness (use trimmedK/trimmedV for checks and
insertion) and change failure behavior to fail fast on invalid entries (e.g.,
throw an Error or surface a clear log message) instead of silently ignoring
them; update the loop over Object.entries(raw as Record<string, unknown>) and
the result assignment to use the trimmed strings and add a validation branch
that raises an explicit error when encountering non-string values or empty
strings after trimming so misconfigurations are detected early.
In `@hooks/capture.ts`:
- Around line 13-33: getRawContent duplicates the message content extraction
already implemented in extractMessages (helpers.ts); refactor by extracting the
shared logic into a single exported helper (e.g., extractRawTextFromMessage or
normalizeMessageContent) in helpers.ts (or a new utils module), update
getRawContent to call that helper, and replace other similar extraction sites to
import and reuse the new helper; ensure the helper accepts unknown/msg: unknown
and preserves current behavior for string, array-of-blocks, and fallback to ""
so callers like getRawContent and extractMessages remain functionally identical.
In `@state.ts`:
- Around line 113-142: getHumanPeer currently has a race where concurrent calls
for the same new channelPeerId can each miss state.humanPeers and create
duplicate honcho.peer and honcho.setMetadata calls; add a pending-creation
dedupe: introduce a Map<string, Promise<Peer>> (e.g., pendingHumanPeers) checked
immediately after the cache miss, store the in-flight Promise before any await
(so subsequent callers return the same Promise), perform the
honcho.peer/honcho.setMetadata work inside that Promise, and finally remove the
entry from pendingHumanPeers and set state.humanPeers once resolved; keep
existing OWNER_ID short-circuit unchanged.
🪄 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: f528c4cc-caf2-45c0-ad4c-f7fb96337522
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
README.mdcommands/cli.tsconfig.tshelpers.tshooks/capture.tshooks/context.tsopenclaw.plugin.jsonstate.tstools/ask.tstools/context.tstools/message-search.tstools/search.tstools/session.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
hooks/capture.ts (2)
110-115: Consider parallel peer resolution for better performance.Sequential
awaitcalls in the loop become a latency bottleneck in group chats with many unique senders.♻️ Proposed parallel resolution
- const resolvedPeers = new Map<string, Awaited<ReturnType<typeof state.getHumanPeer>>>(); - for (const senderId of senderIds) { - resolvedPeers.set(senderId, await state.getHumanPeer(senderId)); - } + const senderIdArray = [...senderIds]; + const peers = await Promise.all(senderIdArray.map((id) => state.getHumanPeer(id))); + const resolvedPeers = new Map(senderIdArray.map((id, i) => [id, peers[i]]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/capture.ts` around lines 110 - 115, The loop resolving peers sequentially is a performance bottleneck; instead, concurrently resolve all peers with Promise.all by mapping senderIds to promises of state.getHumanPeer(senderId), await them together, and populate resolvedPeers from the resulting array; also fetch defaultHumanPeer in parallel (e.g., start its promise before awaiting the batch) so both per-sender and default resolution happen concurrently while keeping the same Map population logic that uses resolvedPeers and the state.getHumanPeer symbol.
13-33: Consider reusing raw content extraction from helpers.ts.This logic duplicates the raw content extraction in
extractMessages(helpers.ts lines 225-235). If the extraction rules change, both locations need updating.Consider exporting a shared helper from
helpers.tsthat bothgetRawContentandextractMessagescan use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/capture.ts` around lines 13 - 33, The getRawContent function duplicates logic already used by extractMessages; extract that shared logic into a single exported helper (e.g., extractRawContent(msg: unknown): string) in helpers.ts, update extractMessages to call that helper, and replace getRawContent with a simple import that delegates to the new helper (or remove getRawContent and import the helper directly) so both places use the same implementation and signature (unknown -> string).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/capture.ts`:
- Around line 143-161: The metadata keys humanPeerId and humanPeerIds in
updatedMeta currently store raw sender IDs (derived from senderIds/lastSenderId)
rather than Honcho peer object IDs; either rename these keys to
humanSenderId/humanSenderIds or update the nearby comment to state explicitly
that these values are sender IDs that must be passed to getHumanPeer() for
resolution (adjust references where existingMeta or callers expect
humanPeerId/humanPeerIds to avoid breaking usage). Ensure all references to
humanPeerId/humanPeerIds in this module and any consumers are updated to the new
names or documented behavior so tooling uses getHumanPeer(senderId) to obtain
peer objects.
---
Nitpick comments:
In `@hooks/capture.ts`:
- Around line 110-115: The loop resolving peers sequentially is a performance
bottleneck; instead, concurrently resolve all peers with Promise.all by mapping
senderIds to promises of state.getHumanPeer(senderId), await them together, and
populate resolvedPeers from the resulting array; also fetch defaultHumanPeer in
parallel (e.g., start its promise before awaiting the batch) so both per-sender
and default resolution happen concurrently while keeping the same Map population
logic that uses resolvedPeers and the state.getHumanPeer symbol.
- Around line 13-33: The getRawContent function duplicates logic already used by
extractMessages; extract that shared logic into a single exported helper (e.g.,
extractRawContent(msg: unknown): string) in helpers.ts, update extractMessages
to call that helper, and replace getRawContent with a simple import that
delegates to the new helper (or remove getRawContent and import the helper
directly) so both places use the same implementation and signature (unknown ->
string).
🪄 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: 7ff26548-7059-464f-a41a-84c0b8747bb7
📒 Files selected for processing (2)
commands/cli.tshooks/capture.ts
✅ Files skipped from review due to trivial changes (1)
- commands/cli.ts
Comment on lines
+143
to
+161
| // Store sender IDs in session metadata for tool resolution. | ||
| // humanPeerId = last active sender (default for tools). | ||
| // humanPeerIds = all known senders in this session (for future multi-target tools). | ||
| const previousPeerIds: string[] = Array.isArray(existingMeta.humanPeerIds) | ||
| ? (existingMeta.humanPeerIds as string[]) | ||
| : []; | ||
| const allPeerIds = [...new Set([...previousPeerIds, ...senderIds])]; | ||
|
|
||
| const updatedMeta: Record<string, unknown> = { | ||
| ...existingMeta, | ||
| ...sessionMeta, | ||
| lastSavedIndex: messages.length, | ||
| }; | ||
| if (lastSenderId) { | ||
| updatedMeta.humanPeerId = lastSenderId; | ||
| } | ||
| if (allPeerIds.length > 0) { | ||
| updatedMeta.humanPeerIds = allPeerIds; | ||
| } |
There was a problem hiding this comment.
Improvement: now persists all sender IDs for multi-participant access.
This addresses the previous review concern about only storing the last sender. The dual storage (humanPeerId for default tool resolution, humanPeerIds for future multi-target tools) is a reasonable design.
However, the naming is misleading: these metadata keys store sender IDs (the raw identifiers extracted from messages), not Honcho peer object IDs. Consider renaming to humanSenderId/humanSenderIds to clarify the distinction, or update the comment to explicitly note these are sender IDs that must be passed to getHumanPeer() for resolution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/capture.ts` around lines 143 - 161, The metadata keys humanPeerId and
humanPeerIds in updatedMeta currently store raw sender IDs (derived from
senderIds/lastSenderId) rather than Honcho peer object IDs; either rename these
keys to humanSenderId/humanSenderIds or update the nearby comment to state
explicitly that these values are sender IDs that must be passed to
getHumanPeer() for resolution (adjust references where existingMeta or callers
expect humanPeerId/humanPeerIds to avoid breaking usage). Ensure all references
to humanPeerId/humanPeerIds in this module and any consumers are updated to the
new names or documented behavior so tooling uses getHumanPeer(senderId) to
obtain peer objects.
ajspig
requested changes
Mar 30, 2026
Collaborator
ajspig
left a comment
ajspig
left a comment
There was a problem hiding this comment.
Thanks @spooktheducks! This is solid work. The sender extraction approach is right (parsing sender_id from the metadata block before cleanMessageContent() strips it), and every tool + hook + CLI command is migrated to the new getHumanPeer/resolveSessionHumanPeer pattern. Backward compat is clean.
Blocking:
- Add concurrency protection to
ensureInitialized()(state.ts). The current bare flag-check (if (state.initialized) return) races when two concurrent hooks enter init simultaneously. This can corrupt workspace metadata. #32 adds aninitLockfor this — please adopt a similar promise-based lock pattern. Make sure errors propagate to waiters (don't just resolve infinally).
Should fix:
-
Parallel peer resolution (capture.ts ~110-115). The sequential
for...of+await getHumanPeer()loop becomes a latency bottleneck with many senders. UsePromise.all()orPromise.allSettled(). -
Naming:
humanPeerIdin session metadata stores sender IDs, not Honcho peer IDs. These raw channel IDs (e.g.U01ZB5DG019) get passed throughgetHumanPeer()for resolution. ConsiderhumanSenderId/humanSenderIdsor a clarifying comment so future readers don't confuse them with resolved Honcho peer IDs.
Note: This PR overlaps significantly with #32 (workspace routing) on state.ts, capture.ts, and context.ts. I'm reviewing both and will decide merge order. Will be heavily dependent on which PR addresses the review first.
4 tasks
menhguin
added a commit
to menhguin/openclaw-honcho
that referenced
this pull request
Apr 6, 2026
Incorporates the multi-peer mapping approach from plastic-labs#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 plastic-labs#50. Supersedes plastic-labs#31. Co-authored-by: spooktheducks <spooktheducks@users.noreply.github.com> Co-authored-by: Minh Nguyen <menhguin@users.noreply.github.com>
menhguin
added a commit
to menhguin/openclaw-honcho
that referenced
this pull request
Apr 6, 2026
Incorporates the multi-peer mapping approach from plastic-labs#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 plastic-labs#50. Supersedes plastic-labs#31. Co-authored-by: spooktheducks <spooktheducks@users.noreply.github.com> Co-authored-by: Minh Nguyen <menhguin@users.noreply.github.com>
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
2 participants
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
New Features
Documentation