Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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:
✨ 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 |
|
@coderabbitai review Maintainer note: this is a focused companion fix for the multi-peer branch. OpenClaw can now keep Discord/channel metadata out of visible user text and emit it as hidden This PR keeps Honcho multi-peer attribution working in that cleaner setup: capture and prompt-time context lookup read the hidden sender id, do not persist runtime-context rows as messages, and prefer hidden metadata over spoofable user-authored blocks. It also fails closed when a runtime-context row exists without a sender id. Local validation passed: |
|
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit. |
|
Updated this PR to resolve the merge conflict against the refreshed multi-peer base branch. What changed:
Validation passed locally:
GitHub now reports the PR as mergeable/clean again. Maintainer review should be unblocked. |
|
Follow-up pushed to make the runtime-context path fail closed instead of falling back to the default/previous participant when attribution is present but incomplete. What changed:
Validated locally:
|
ajspig
left a comment
There was a problem hiding this comment.
Hey! Thanks for contributing. Runtime-context sender resolution is the right approach for clean multi-peer attribution, and the fail-closed behavior (skip rather than misattribute) is solid. A few things before merging:
API surface cleanup:
- 8 new exported functions in helpers.ts, but only 3 are used outside tests. The strict/non-strict pairs (resolveSenderIdForMessage / resolveSenderIdForMessageStrict, same for CurrentTurn) add surface for a null vs undefined distinction — consider keeping only the strict variants and letting callers do ?? undefined if needed.
pickSenderId is very permissive:
- It tries 7 fallback keys (sender_id, senderId, SenderId, user_id, userId, id, sender). Since runtime-context format is controlled by OpenClaw, can we lock this to the actual field name(s) OpenClaw emits? The wide net risks matching unrelated fields.
Minor:
- maxDistance = 4 — worth a brief comment explaining why 4 (how many rows can sit between a user message and its runtime-context row).
- extractMessages is at 6 params now, with the last being a 4-arg function. An options object would be cleaner, especially as the signature keeps growing.
|
Addressed the review feedback in 680a350:
Validation run locally:
|
|
@ajspig I pushed The core behavior is now: only OpenClaw-generated hidden Review items addressed:
Validation after the change:
Could you re-review this when you get a chance? The branch is clean/mergeable on GitHub from my latest check. |
|
I believe the requested changes from the earlier review have been addressed in 680a350:
Could you re-review when convenient? |
* fix: extract sender.id from object-shaped sender field
OpenClaw's Conversation info metadata block emits sender as an object:
"sender": { "id": "378376356108435457", "name": "bigrob8181", ... }
The previous string-only fallback (parsed.sender_id ?? parsed.sender) returned
undefined when parsed.sender was an object, causing every Discord/HA-Assist
message to fall through to the OWNER_ID default peer instead of the actual
sender. This is the root cause of the "everything goes to owner instead of rob"
attribution bug.
Extraction priority is now:
1. parsed.sender_id (top-level string) — canonical envelope contract
2. parsed.sender (string) — legacy shape, preserved for back-compat
3. parsed.sender.id (object.id string) — current OpenClaw/HA-Assist shape
Tests cover Discord envelope shape, HA Assist envelope shape, priority
ordering, and rejection of non-string / empty sender.id values.
Distinct from upstream plastic-labs#77 (fork PR #46), which removed the string fallback
entirely and did not add object-shape support. That PR is DO NOT MERGE per
the Codex review consensus.
* review: make sender extraction type-guarded per path
Codex review flagged that `parsed.sender_id ?? parsed.sender` uses value-presence
fallback rather than type-aware fallback. A malformed higher-priority field
(e.g. `sender_id: 12345`) would short-circuit the operator and skip the valid
`sender.id` object fallback, returning undefined instead.
Each of the three extraction paths is now type-guarded independently:
1. sender_id (must be non-empty string)
2. sender (must be non-empty string)
3. sender.id (must be non-empty string)
A malformed candidate at any level falls through to the next path.
Two additional tests:
- non-string sender_id falls back to valid sender.id object
- null sender_id falls back to legacy sender string
---------
Co-authored-by: Clawd <clawd@openclaw.ai>
Summary
openclaw.runtime-contextrows when visible user messages no longer carry inline channel metadatabefore_prompt_buildcontext injectionWhy
OpenClaw can keep Discord/channel metadata out of model-visible user text by emitting it as a hidden runtime-context row after the user message. Without this fallback, multi-peer Honcho deployments may attribute clean Discord messages to the default/previous participant and mix memory between people.
This keeps the prompt/history clean while preserving per-speaker Honcho routing.
Tests
corepack pnpm exec vitest run helpers.test.tscorepack pnpm exec vitest runcorepack pnpm buildgit diff --check