quick clean up for duplicated dm config and msg helpers#99
Conversation
Introduce helper utilities for DM portal configuration and pre-built remote messages: ConfigureDMPortal, BuildPreConvertedRemoteMessage, and SendSystemMessage. Extend BuildLoginDMChatInfo/BuildDMChatInfo to accept custom senders, bot UserInfo and member extras. Add ClientBase.SendSystemMessage wrapper and replace numerous ad-hoc portal configuration and send implementations across bridges (ai, codex, openclaw, opencode, dummybridge, opencode removal of portal_send). Update OpenClaw gateway client identifiers and refactor message queuing to use BuildPreConvertedRemoteMessage. Update tests to cover new helpers and timing/ID generation. This centralizes DM/remote message logic, reduces duplication, and standardizes system notice delivery.
Apply non-functional formatting changes: align struct field names and types in DMChatInfoParams and LoginDMChatInfoParams, adjust spacing in BuildDMChatInfo (Membership) and provisioning.go (CanBackfill). No behavioral changes; only whitespace/formatting adjustments.
Clean up imports across multiple bridge packages to address unused-import compile errors and tidy code. Removed unused imports (database, event, networkid) from codex, dummybridge, openclaw, and opencode files; added a missing time import in openclaw/events.go. Changes affect: bridges/codex/client.go, bridges/codex/directory_manager.go, bridges/dummybridge/bridge.go, bridges/openclaw/events.go, bridges/openclaw/provisioning.go, and bridges/opencode/opencode_portal.go.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughReplaces ad-hoc DM-portal mutations and bespoke notice/remote-message plumbing with centralized helpers in agentremote: ConfigureDMPortal, BuildLoginDMChatInfo/BuildPreConvertedRemoteMessage, and SendSystemMessage; updates multiple bridges and SDK call sites to use these helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant BridgeClient
participant agentremote as agentremote
participant BridgeBot as Bridge/Bot (if available)
participant Intent as PortalIntent
participant Matrix as MatrixServer
BridgeClient->>agentremote: SendSystemMessage(ctx, login, portal, sender, body)
agentremote->>agentremote: validate inputs, trim body
alt login.Bridge.Bot available and usable
agentremote->>BridgeBot: Bot.SendMessage(portal.MXID, event.MsgNotice(body), sender)
BridgeBot->>Matrix: deliver event to room
else use portal intent
agentremote->>Intent: portal.GetIntentFor(sender)
Intent->>Matrix: SendNotice(portal.MXID, event.MsgNotice(body))
end
agentremote-->>BridgeClient: return success / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🧹 Nitpick comments (3)
bridges/openclaw/provisioning.go (1)
305-314: Minor: Error message is misleading.At line 312, the error message says "failed to save openclaw dm portal" but
ConfigureDMPortalis called withSave: false, so it never saves. The actual save happens inEnsurePortalLifecycleat line 315.🔧 Suggested fix
if err := agentremote.ConfigureDMPortal(ctx, agentremote.ConfigureDMPortalParams{ Portal: portal, Title: meta.OpenClawDMTargetAgentName, Topic: "OpenClaw agent DM", OtherUserID: openClawGhostUserID(agentID), Save: false, }); err != nil { - return nil, fmt.Errorf("failed to save openclaw dm portal: %w", err) + return nil, fmt.Errorf("failed to configure openclaw dm portal: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/provisioning.go` around lines 305 - 314, The error message is misleading because ConfigureDMPortal is called with Save: false; update the error handling around agentremote.ConfigureDMPortal in provisioning.go (the ConfigureDMPortal call) to use a correct description such as "failed to configure openclaw dm portal" (or similar) instead of "failed to save openclaw dm portal", so the message reflects configuration failure (the actual save occurs later in EnsurePortalLifecycle).bridges/codex/backfill.go (1)
237-245: Consider reordering:ConfigureDMPortalis called aftercomposeCodexChatInfo.The chat info is built at line 237 before the portal is configured as a DM at lines 238-245. While this likely works because
composeCodexChatInfouses thetitleparameter rather than reading from the portal, the ordering is counterintuitive.🔧 Suggested reordering for clarity
if meta.Slug == "" { meta.Slug = codexThreadSlug(threadID) } - info := cc.composeCodexChatInfo(portal, title, true) if err := agentremote.ConfigureDMPortal(ctx, agentremote.ConfigureDMPortalParams{ Portal: portal, Title: title, OtherUserID: codexGhostID, Save: false, }); err != nil { return nil, false, err } + info := cc.composeCodexChatInfo(portal, title, true) created, err = bridgesdk.EnsurePortalLifecycle(ctx, bridgesdk.PortalLifecycleOptions{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/codex/backfill.go` around lines 237 - 245, composeCodexChatInfo is called before configuring the portal as a DM which is counterintuitive; move the agentremote.ConfigureDMPortal call (using agentremote.ConfigureDMPortalParams with Portal, Title, OtherUserID: codexGhostID, Save: false) to occur before calling cc.composeCodexChatInfo(portal, title, true) so the portal is configured as a DM prior to building chat info.helpers.go (1)
221-231: Assignment top.MsgIDon line 231 has no effect on the caller.
SendViaPortalParamsis passed by value, so the assignmentp.MsgID = evt.IDonly modifies the local copy. The caller won't see the generated ID reflected in theirSendViaPortalParamsstruct.This appears to be intentional since the generated ID is returned as the second return value, but the assignment is misleading dead code.
🔧 Suggested cleanup
evt := BuildPreConvertedRemoteMessage(PreConvertedRemoteMessageParams{ PortalKey: p.Portal.PortalKey, Sender: p.Sender, MsgID: p.MsgID, IDPrefix: p.IDPrefix, LogKey: p.LogKey, Timestamp: p.Timestamp, StreamOrder: p.StreamOrder, Converted: p.Converted, }) - p.MsgID = evt.ID result := p.Login.QueueRemoteEvent(evt) if !result.Success { if result.Error != nil { - return "", p.MsgID, fmt.Errorf("send failed: %w", result.Error) + return "", evt.ID, fmt.Errorf("send failed: %w", result.Error) } - return "", p.MsgID, fmt.Errorf("send failed") + return "", evt.ID, fmt.Errorf("send failed") } - return result.EventID, p.MsgID, nil + return result.EventID, evt.ID, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helpers.go` around lines 221 - 231, The assignment p.MsgID = evt.ID is dead because SendViaPortalParams is passed by value, so update the code to either remove that assignment (preferred since BuildPreConvertedRemoteMessage already returns the generated ID) or change the caller/signature to accept *SendViaPortalParams if you intend to mutate the caller; locate the assignment near BuildPreConvertedRemoteMessage/PreConvertedRemoteMessageParams and remove p.MsgID = evt.ID (or convert the SendViaPortalParams parameter to a pointer and update all call sites if you choose to make the mutation visible).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bridges/codex/client.go`:
- Around line 1766-1772: The welcome notices are dropped because
sendSystemNotice calls agentremote.SendSystemMessage immediately when a newly
created portal has no MXID; update sendSystemNotice (and keep
createWelcomeCodexChat callers unchanged) to detect an empty portal.MXID and
retry sending: if portal.MXID == "" spawn a short-lived goroutine that retries
agentremote.SendSystemMessage with a small backoff (e.g., 3 attempts, 200–500ms
interval) until portal.MXID is populated or attempts exhausted, and preserve the
existing logging (use cc.log.Warn().Err(err).Msg on final failure); ensure the
retry logic references sendSystemNotice and agentremote.SendSystemMessage so the
message is delivered once the portal has an MXID.
In `@bridges/openclaw/client.go`:
- Around line 738-746: The helper sendSystemNotice currently hardcodes an empty
bridgev2.EventSender which breaks intent resolution and ghost attribution;
change its signature to accept a sender parameter (e.g., sender
bridgev2.EventSender) and use that sender when calling
agentremote.SendSystemMessage in place of bridgev2.EventSender{}, then update
all call sites that compute agent-specific senders (approval notices and others)
to pass the computed sender through; ensure callers that previously relied on
Bridge.Bot behavior still construct the appropriate EventSender before invoking
sendSystemNotice.
In `@sdk/client.go`:
- Around line 78-79: The SendNotice callback currently always passes an empty
bridgev2.EventSender to agentremote.SendSystemMessage, ignoring the configured
ApprovalFlowConfig.Sender; update SendNotice to use the configured sender
(ApprovalFlowConfig.Sender) instead of bridgev2.EventSender{} so notices use the
proper identity and fallback to login only if the configured sender is nil or
unavailable; change the SendNotice invocation of agentremote.SendSystemMessage
to pass the resolved sender value and preserve ctx, login and portal parameters.
In `@sdk/conversation.go`:
- Around line 225-226: SendNotice currently calls agentremote.SendSystemMessage
which bypasses the conversation transport and intentOverride; change SendNotice
to delegate to the conversation's normal message path (use the existing
sendMessageContent helper on Conversation) so messages use c.sender and respect
intentOverride/transport. Specifically, replace the
agentremote.SendSystemMessage call in SendNotice with a call into
Conversation.sendMessageContent (or the internal send helper that handles system
messages) passing ctx, c.sender, the system/text payload and any
intentOverride/transport flags so notices are delivered from the correct
account.
---
Nitpick comments:
In `@bridges/codex/backfill.go`:
- Around line 237-245: composeCodexChatInfo is called before configuring the
portal as a DM which is counterintuitive; move the agentremote.ConfigureDMPortal
call (using agentremote.ConfigureDMPortalParams with Portal, Title, OtherUserID:
codexGhostID, Save: false) to occur before calling
cc.composeCodexChatInfo(portal, title, true) so the portal is configured as a DM
prior to building chat info.
In `@bridges/openclaw/provisioning.go`:
- Around line 305-314: The error message is misleading because ConfigureDMPortal
is called with Save: false; update the error handling around
agentremote.ConfigureDMPortal in provisioning.go (the ConfigureDMPortal call) to
use a correct description such as "failed to configure openclaw dm portal" (or
similar) instead of "failed to save openclaw dm portal", so the message reflects
configuration failure (the actual save occurs later in EnsurePortalLifecycle).
In `@helpers.go`:
- Around line 221-231: The assignment p.MsgID = evt.ID is dead because
SendViaPortalParams is passed by value, so update the code to either remove that
assignment (preferred since BuildPreConvertedRemoteMessage already returns the
generated ID) or change the caller/signature to accept *SendViaPortalParams if
you intend to mutate the caller; locate the assignment near
BuildPreConvertedRemoteMessage/PreConvertedRemoteMessageParams and remove
p.MsgID = evt.ID (or convert the SendViaPortalParams parameter to a pointer and
update all call sites if you choose to make the mutation visible).
🪄 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: 56bce048-afb2-4c2c-a25f-299281f42f92
📒 Files selected for processing (18)
bridges/ai/chat.gobridges/codex/backfill.gobridges/codex/client.gobridges/codex/directory_manager.gobridges/codex/portal_send.gobridges/dummybridge/bridge.gobridges/openclaw/client.gobridges/openclaw/events.gobridges/openclaw/gateway_client.gobridges/openclaw/manager.gobridges/openclaw/manager_test.gobridges/openclaw/provisioning.gobridges/opencode/host.gobridges/opencode/opencode_portal.gobridges/opencode/portal_send.gohelpers.gosdk/client.gosdk/conversation.go
💤 Files with no reviewable changes (1)
- bridges/opencode/portal_send.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Lint
- GitHub Check: build-agentremote-docker (amd64)
- GitHub Check: build-docker
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-16T09:01:24.464Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 71
File: bridges/ai/connector.go:53-63
Timestamp: 2026-03-16T09:01:24.464Z
Learning: In package ai, the AI connector’s configuration type (Config) defines Bridge as a value field of type BridgeConfig (not a pointer). Therefore, accessing oc.Config.Bridge.CommandPrefix in OpenAIConnector.applyRuntimeDefaults (bridges/ai/connector.go) is safe and does not require a nil-check.
Applied to files:
bridges/dummybridge/bridge.go
🔇 Additional comments (14)
helpers.go (5)
384-418: LGTM!The
SendSystemMessagefunction is well-designed with proper input validation, trimming of empty bodies, and a sensible fallback fromBridge.Botto intent-based sending. The empty body guard returningnilis a good defensive pattern.
147-164: LGTM!
ConfigureDMPortalcorrectly sets all DM portal fields (RoomType,OtherUserID,Name/NameSet,Topic/TopicSet) with proper trimming, supports an optionalMutatePortalcallback for custom modifications, and conditionally saves based on theSaveflag. This is a clean consolidation of previously duplicated logic.
177-196: LGTM!
BuildPreConvertedRemoteMessagecorrectly auto-generates a message ID when not provided, resolves event timing, and constructs thePreConvertedMessagewith proper log context. This centralizes logic previously duplicated inSendViaPortalandBuildContinuationMessage.
51-103: LGTM!The expanded
BuildDMChatInfoproperly handles optional sender/user info parameters with sensible defaults. The fallback logic forBotUserInfo(defaulting fromBotDisplayName) andBotMemberEventExtrais clean and maintains backward compatibility.
584-599: LGTM!Good refactor to use
BuildPreConvertedRemoteMessageinstead of duplicating the message ID generation and timing resolution logic.bridges/dummybridge/bridge.go (2)
231-239: LGTM!Clean migration to
ConfigureDMPortal. TheSave: falseflag is appropriate sinceEnsurePortalLifecyclewithSaveBeforeCreate: true(line 246) will persist the portal.
259-270: LGTM!Good use of the new
TopicandBotUserInfoparameters inBuildLoginDMChatInfo. PassingdummySDKAgent().UserInfo()directly provides consistent bot metadata.bridges/codex/portal_send.go (1)
1-17: LGTM!Clean simplification. The file now only contains the essential
senderForPortalandsenderForHumanhelpers. The removedsendViaPortalis replaced by the centralizedagentremote.SendSystemMessage.bridges/codex/directory_manager.go (1)
185-192: LGTM!Proper use of
ConfigureDMPortalwithSave: false. The error is correctly propagated, and the portal will be saved byEnsurePortalLifecyclewithSaveBeforeCreate: true(line 198).bridges/opencode/opencode_portal.go (2)
82-89: LGTM!Proper migration to
ConfigureDMPortal. The error handling and flow withEnsurePortalLifecycle(line 96) is correct.
228-235: LGTM!Consistent use of
ConfigureDMPortalin the managed launcher chat creation path. The pattern matches the other bridge implementations.bridges/openclaw/provisioning.go (1)
333-355: LGTM!Good refactor of
syntheticDMPortalInfoto accept pre-computeduserInfoand use the newBuildLoginDMChatInfoparameters (HumanSender,BotSender,BotUserInfo,BotMemberEventExtra). This eliminates the need to mutate the member map after construction.bridges/openclaw/gateway_client.go (1)
32-33: Verify gateway server compatibility with new client identity values.The client ID and mode constants are sent to the gateway during the connection handshake via
buildConnectParams(). Ensure the gateway server properly recognizes and accepts"gateway-client"as the client ID and"backend"as the mode. This is an external dependency that cannot be verified within this repository.bridges/openclaw/events.go (1)
145-157: LGTM! Clean consolidation of DM portal configuration.The refactor properly passes
Topic,HumanSender,BotSender, andBotUserInfotoBuildLoginDMChatInfoat construction time, eliminating the need for post-mutation. The sender assignments are consistent with the non-DM case below (human usestrue, bot usesfalse), and all parameters align with the helper signature inhelpers.go.
| func (cc *CodexClient) sendSystemNotice(ctx context.Context, portal *bridgev2.Portal, message string) { | ||
| if portal == nil || portal.MXID == "" || cc.UserLogin == nil || cc.UserLogin.Bridge == nil { | ||
| if cc == nil { | ||
| return | ||
| } | ||
| timing := agentremote.ResolveEventTiming(time.Now(), 0) | ||
| cc.sendViaPortal(portal, agentremote.BuildSystemNotice(strings.TrimSpace(message)), "", timing.Timestamp, timing.StreamOrder) | ||
| if err := agentremote.SendSystemMessage(ctx, cc.UserLogin, portal, bridgev2.EventSender{}, strings.TrimSpace(message)); err != nil { | ||
| cc.log.Warn().Err(err).Msg("Failed to send system notice") | ||
| } |
There was a problem hiding this comment.
Welcome notices are now dropped on brand-new rooms.
createWelcomeCodexChat still calls this helper immediately after creating the portal. Before the room has an MXID, SendSystemMessage returns invalid portal, so the onboarding notices are just logged and never delivered.
Suggested fix
func (cc *CodexClient) sendSystemNotice(ctx context.Context, portal *bridgev2.Portal, message string) {
- if cc == nil {
+ if cc == nil || portal == nil || portal.MXID == "" {
return
}
if err := agentremote.SendSystemMessage(ctx, cc.UserLogin, portal, bridgev2.EventSender{}, strings.TrimSpace(message)); err != nil {
cc.log.Warn().Err(err).Msg("Failed to send system notice")
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (cc *CodexClient) sendSystemNotice(ctx context.Context, portal *bridgev2.Portal, message string) { | |
| if portal == nil || portal.MXID == "" || cc.UserLogin == nil || cc.UserLogin.Bridge == nil { | |
| if cc == nil { | |
| return | |
| } | |
| timing := agentremote.ResolveEventTiming(time.Now(), 0) | |
| cc.sendViaPortal(portal, agentremote.BuildSystemNotice(strings.TrimSpace(message)), "", timing.Timestamp, timing.StreamOrder) | |
| if err := agentremote.SendSystemMessage(ctx, cc.UserLogin, portal, bridgev2.EventSender{}, strings.TrimSpace(message)); err != nil { | |
| cc.log.Warn().Err(err).Msg("Failed to send system notice") | |
| } | |
| func (cc *CodexClient) sendSystemNotice(ctx context.Context, portal *bridgev2.Portal, message string) { | |
| if cc == nil || portal == nil || portal.MXID == "" { | |
| return | |
| } | |
| if err := agentremote.SendSystemMessage(ctx, cc.UserLogin, portal, bridgev2.EventSender{}, strings.TrimSpace(message)); err != nil { | |
| cc.log.Warn().Err(err).Msg("Failed to send system notice") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/codex/client.go` around lines 1766 - 1772, The welcome notices are
dropped because sendSystemNotice calls agentremote.SendSystemMessage immediately
when a newly created portal has no MXID; update sendSystemNotice (and keep
createWelcomeCodexChat callers unchanged) to detect an empty portal.MXID and
retry sending: if portal.MXID == "" spawn a short-lived goroutine that retries
agentremote.SendSystemMessage with a small backoff (e.g., 3 attempts, 200–500ms
interval) until portal.MXID is populated or attempts exhausted, and preserve the
existing logging (use cc.log.Warn().Err(err).Msg on final failure); ensure the
retry logic references sendSystemNotice and agentremote.SendSystemMessage so the
message is delivered once the portal has an MXID.
bridges/openclaw/client.go
Outdated
| func (oc *OpenClawClient) sendSystemNotice(ctx context.Context, portal *bridgev2.Portal, msg string) { | ||
| if oc == nil || portal == nil || strings.TrimSpace(msg) == "" { | ||
| return | ||
| } | ||
| converted := &bridgev2.ConvertedMessage{ | ||
| Parts: []*bridgev2.ConvertedMessagePart{{ | ||
| ID: networkid.PartID("0"), | ||
| Type: event.EventMessage, | ||
| Content: &event.MessageEventContent{MsgType: event.MsgNotice, Body: msg, Mentions: &event.Mentions{}}, | ||
| }}, | ||
| } | ||
| oc.UserLogin.QueueRemoteEvent(buildOpenClawRemoteMessage( | ||
| portal.PortalKey, | ||
| newOpenClawMessageID(), | ||
| sender, | ||
| time.Now(), | ||
| 0, | ||
| converted, | ||
| )) | ||
| if err := agentremote.SendSystemMessage(ctx, oc.UserLogin, portal, bridgev2.EventSender{}, msg); err != nil { | ||
| if oc.UserLogin != nil { | ||
| oc.UserLogin.Log.Warn().Err(err).Msg("Failed to send system notice") | ||
| } | ||
| } |
There was a problem hiding this comment.
This helper needs a sender parameter.
Callers still compute agent-specific senders for approval notices, but this wrapper always passes bridgev2.EventSender{}. On installs without Bridge.Bot, those notices can fail intent resolution or show up from the wrong ghost.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/openclaw/client.go` around lines 738 - 746, The helper
sendSystemNotice currently hardcodes an empty bridgev2.EventSender which breaks
intent resolution and ghost attribution; change its signature to accept a sender
parameter (e.g., sender bridgev2.EventSender) and use that sender when calling
agentremote.SendSystemMessage in place of bridgev2.EventSender{}, then update
all call sites that compute agent-specific senders (approval notices and others)
to pass the computed sender through; ensure callers that previously relied on
Bridge.Bot behavior still construct the appropriate EventSender before invoking
sendSystemNotice.
Rename OpenClaw helper syntheticDMPortalInfo to buildOpenClawDMChatInfo and update its call sites in GetChatInfo and createConfiguredAgentDM for clarity. Also change CodexClient.sendSystemNotice to pass the message through unchanged (remove strings.TrimSpace) so system messages are not altered before sending.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
bridges/openclaw/client.go (1)
738-742:⚠️ Potential issue | 🟠 MajorPass the resolved sender through this helper.
Line 742 hardcodes
bridgev2.EventSender{}, so every notice sent throughsendSystemNoticeloses ghost/intent attribution. That can select the wrong sender or fail intent resolution when there is no bot intent to fall back to. Please take asender bridgev2.EventSenderparameter here and forward it toagentremote.SendSystemMessage.Suggested change
-func (oc *OpenClawClient) sendSystemNotice(ctx context.Context, portal *bridgev2.Portal, msg string) { +func (oc *OpenClawClient) sendSystemNotice(ctx context.Context, portal *bridgev2.Portal, sender bridgev2.EventSender, msg string) { if oc == nil || portal == nil || strings.TrimSpace(msg) == "" { return } - if err := agentremote.SendSystemMessage(ctx, oc.UserLogin, portal, bridgev2.EventSender{}, msg); err != nil { + if err := agentremote.SendSystemMessage(ctx, oc.UserLogin, portal, sender, msg); err != nil { if oc.UserLogin != nil { oc.UserLogin.Log.Warn().Err(err).Msg("Failed to send system notice") } } }#!/bin/bash set -euo pipefail # Inspect the helper and its current call sites in the OpenClaw bridge. rg -n -C3 --type go 'func \(oc \*OpenClawClient\) sendSystemNotice|sendSystemNotice\(' bridges/openclaw # Inspect where OpenClaw constructs agent-specific senders today. rg -n -C3 --type go 'senderForAgent\(' bridges/openclaw🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/client.go` around lines 738 - 742, The sendSystemNotice helper currently hardcodes bridgev2.EventSender{} and loses ghost/intent attribution; change func (oc *OpenClawClient) sendSystemNotice to accept a sender bridgev2.EventSender parameter and forward that sender to agentremote.SendSystemMessage (replace the hardcoded bridgev2.EventSender{}). Update all call sites of sendSystemNotice in OpenClawClient to pass the resolved sender (use existing senderForAgent or the local sender variable used where notices are created) so intent/ghost attribution is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@bridges/openclaw/client.go`:
- Around line 738-742: The sendSystemNotice helper currently hardcodes
bridgev2.EventSender{} and loses ghost/intent attribution; change func (oc
*OpenClawClient) sendSystemNotice to accept a sender bridgev2.EventSender
parameter and forward that sender to agentremote.SendSystemMessage (replace the
hardcoded bridgev2.EventSender{}). Update all call sites of sendSystemNotice in
OpenClawClient to pass the resolved sender (use existing senderForAgent or the
local sender variable used where notices are created) so intent/ghost
attribution is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22a6b41a-3299-4d5b-bb1d-8c16876fbaaa
📒 Files selected for processing (3)
bridges/codex/client.gobridges/openclaw/client.gobridges/openclaw/provisioning.go
🚧 Files skipped from review as they are similar to previous changes (1)
- bridges/codex/client.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-agentremote-docker (amd64)
- GitHub Check: build-docker
- GitHub Check: Lint
- GitHub Check: Lint
- GitHub Check: build-docker
🔇 Additional comments (1)
bridges/openclaw/provisioning.go (1)
305-311: The DM portal is correctly persisted; no issue here.The code is safe as written. The
ConfigureDMPortal(..., Save: false)call at line 305 mutates the portal in memory, and the immediately followingEnsurePortalLifecycle(..., SaveBeforeCreate: true)at line 315 saves the portal before any create or update logic executes. TheSaveBeforeCreateflag insdk/portal_lifecycle.go:33-36unconditionally persists the portal, not just on the create path, ensuring that the DM field mutations (RoomType,OtherUserID,Name,Topic) are durably stored.
Propagate and use a portal-specific EventSender for system notices across Codex and OpenClaw bridges and the SDK. Key changes: - bridges/codex: delay composeCodexChatInfo until after portal configuration and add background retry send when portal.MXID is not yet set. - bridges/openclaw: change sendSystemNotice to accept a sender, pass approval-specific sender from manager, and update related calls; tweak provisioning error text. - helpers.go: return the actual event ID (evt.ID) on errors/success instead of using p.MsgID. - sdk/client.go: provide senderForPortal to approval flow and use it when sending notices. - sdk/conversation.go: make Conversation.SendNotice trim input and send via sendMessageContent (uses conversation intent), avoiding direct agentremote call. - sdk/turn_test.go: add test to verify Conversation.SendNotice uses the conversation intent and records sent message details; expand test matrix API stub to record sent messages. These changes ensure system notices are sent with the correct sender identity, handle races when a portal's MXID isn't immediately available, and standardize event ID handling and notice delivery paths.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
sdk/turn_test.go (1)
816-846: Consider expandingSendNoticecoverage for the other new semantics.This test covers trim + transport well; adding assertions for whitespace-only no-op and
msg.Mentions != nilwould better lock in the behavior introduced insdk/conversation.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/turn_test.go` around lines 816 - 846, Expand TestConversationSendNoticeUsesConversationIntent to also verify the new SendNotice semantics: after the existing happy-path assertions, call conv.SendNotice with a whitespace-only string (e.g., " ") and assert that intent.sentMessages length does not increase (whitespace-only is a no-op), and assert on the parsed msg from the previously sent notice that msg.Mentions is not nil to lock in mention-handling behavior. Use the existing test's conv, intent, and intent.sentMessages slice and the msg variable (from intent.sentMessages[0].content.Parsed) to locate where to add the two new assertions.bridges/openclaw/client.go (1)
339-343: Consider removing redundant post-build field overrides.The
buildOpenClawDMChatInfohelper (inprovisioning.go) already setsCanBackfill: trueandTopic: "OpenClaw agent DM". Lines 340-342 overwrite these values:
- Line 342:
CanBackfill = trueis redundant (builder already sets this)- Line 340: For DM portals,
topicForPortalreturns"OpenClaw agent DM"(line 505-506), which matches the builder default- Line 341:
Typeoverride may be intentional ifopenClawRoomType(meta)can differ fromRoomTypeDMIf the builder is meant to control these values centrally, consider removing the redundant assignments or documenting why overrides are necessary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/client.go` around lines 339 - 343, The post-build overrides on the returned info are redundant: remove the explicit assignments to info.Topic and info.CanBackfill in the block that calls buildOpenClawDMChatInfo (leave only the intentional Type override if needed); instead rely on buildOpenClawDMChatInfo to set CanBackfill:true and the default topic, unless you have a specific case where openClawRoomType(meta) must change the Type—if so, keep only the info.Type assignment and delete the lines that set info.Topic = ptr.NonZero(oc.topicForPortal(meta)) and info.CanBackfill = true.bridges/codex/client.go (1)
1773-1790: Retry logic addresses the welcome notice issue, but final attempt could be optimized.The goroutine-based retry approach solves the past concern about dropped notices on new portals. However, after the loop completes its 3 iterations, the final
send()at line 1785-1787 is called without first checking ifportal.MXIDwas populated during the last sleep. If it's still empty,SendSystemMessagewill return "invalid portal" and log a warning unnecessarily.🔧 Suggested refinement
go func() { retryCtx := cc.backgroundContext(ctx) for attempt := 0; attempt < 3; attempt++ { if portal.MXID != "" { if err := send(retryCtx); err != nil { cc.log.Warn().Err(err).Msg("Failed to send system notice") } return } time.Sleep(250 * time.Millisecond) } + if portal.MXID == "" { + cc.log.Debug().Msg("Portal MXID still empty after retries, giving up on system notice") + return + } if err := send(retryCtx); err != nil { cc.log.Warn().Err(err).Msg("Failed to send system notice") } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/codex/client.go` around lines 1773 - 1790, The final unconditional call to send(retryCtx) may run even if portal.MXID is still empty; update the goroutine in the block with portal.MXID to check portal.MXID again after the retry loop and only call send(retryCtx) when portal.MXID != "" to avoid triggering SendSystemMessage("invalid portal"); use the same retryCtx from cc.backgroundContext(ctx) and the existing cc.log.Warn().Err(...) path when send returns an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bridges/codex/client.go`:
- Around line 1773-1790: The final unconditional call to send(retryCtx) may run
even if portal.MXID is still empty; update the goroutine in the block with
portal.MXID to check portal.MXID again after the retry loop and only call
send(retryCtx) when portal.MXID != "" to avoid triggering
SendSystemMessage("invalid portal"); use the same retryCtx from
cc.backgroundContext(ctx) and the existing cc.log.Warn().Err(...) path when send
returns an error.
In `@bridges/openclaw/client.go`:
- Around line 339-343: The post-build overrides on the returned info are
redundant: remove the explicit assignments to info.Topic and info.CanBackfill in
the block that calls buildOpenClawDMChatInfo (leave only the intentional Type
override if needed); instead rely on buildOpenClawDMChatInfo to set
CanBackfill:true and the default topic, unless you have a specific case where
openClawRoomType(meta) must change the Type—if so, keep only the info.Type
assignment and delete the lines that set info.Topic =
ptr.NonZero(oc.topicForPortal(meta)) and info.CanBackfill = true.
In `@sdk/turn_test.go`:
- Around line 816-846: Expand TestConversationSendNoticeUsesConversationIntent
to also verify the new SendNotice semantics: after the existing happy-path
assertions, call conv.SendNotice with a whitespace-only string (e.g., " ") and
assert that intent.sentMessages length does not increase (whitespace-only is a
no-op), and assert on the parsed msg from the previously sent notice that
msg.Mentions is not nil to lock in mention-handling behavior. Use the existing
test's conv, intent, and intent.sentMessages slice and the msg variable (from
intent.sentMessages[0].content.Parsed) to locate where to add the two new
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 241e391a-1e4a-4aa4-b010-cdaed33d0021
📒 Files selected for processing (9)
bridges/codex/backfill.gobridges/codex/client.gobridges/openclaw/client.gobridges/openclaw/manager.gobridges/openclaw/provisioning.gohelpers.gosdk/client.gosdk/conversation.gosdk/turn_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- sdk/client.go
- bridges/codex/backfill.go
- bridges/openclaw/provisioning.go
- helpers.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docker
- GitHub Check: Lint
🔇 Additional comments (9)
sdk/conversation.go (1)
225-234: Good fix: notice delivery now stays on conversation intent and trims safely.This correctly routes through
sendMessageContent(sointentOverride/conversation transport is preserved), while skipping empty notices and keeping typed mentions explicit.sdk/turn_test.go (1)
24-42: Test message capture extension looks solid.Recording
roomID,eventType, andcontentin the stub makes downstream assertions precise and keeps the test intent clear.bridges/openclaw/client.go (1)
738-747: Sender parameter issue has been addressed.The helper now correctly accepts a
senderparameter and passes it toagentremote.SendSystemMessage. This resolves the previously identified issue where passing an emptybridgev2.EventSender{}could fail intent resolution or show notices from the wrong ghost.bridges/openclaw/manager.go (5)
113-115: LGTM!The
SendNoticecallback now correctly passes the agent-specific sender viaapprovalSenderForPortal, ensuring proper ghost attribution for system notices.
377-379: LGTM!The approval expiration notice correctly uses the portal-specific sender for proper ghost attribution.
1806-1808: LGTM!The approval-resolved notice correctly uses the portal-specific sender. The fallback to a system notice when no stream part context is available is appropriate.
1918-1926: LGTM!The switch to
agentremote.BuildPreConvertedRemoteMessagewith the explicitLogKey: "openclaw_msg_id"aligns with the established convention (matchingMessageLogKeyinclient.goline 120 and the approval flow config at line 94).
1958-1966: LGTM!Consistent use of
BuildPreConvertedRemoteMessage. TheStreamOrder: payload.Seq*2 - 1ensures user messages from history are ordered before the corresponding assistant response (payload.Seq * 2), maintaining proper conversation sequencing.bridges/codex/client.go (1)
1541-1549: LGTM! Clean consolidation of ChatInfo construction.The refactor properly delegates Topic handling to
BuildLoginDMChatInfo, which internally usesptr.NonZeroto handle empty topics. This removes unnecessary intermediate variable and post-build assignment.
1 participant
No description provided.