Conversation
Move AI runtime login state out of bridge login metadata into a dedicated DB-backed runtime state (aichats_login_state). Add login_state_db.go with load/save/update/clear helpers and wire usage across AI client code (NextChatIndex, LastHeartbeatEvent, DefaultChatPortalID, LastActiveRoomByAgent, etc.). Refactor scheduler to use an internal runtime context and in-process timers instead of Matrix delayed events (remove pending_delay_* usage), update cron/heartbeat scheduling logic and DB read/write to match the new approach, and stop/cleanup scheduler on disconnect. Remove Matrix-specific coupling and connector event handler registration, and switch message-status sending to the agentremote helper. Includes DB migration/schema updates (pkg/aidb changes) to support the new login state table and scheduler column removals.
Move tool approval rules out of the in-memory login state and into a dedicated DB table (aichats_tool_approval_rules) with lookups/inserts. Remove several login_state fields and related helpers (default_chat_portal_id, tool_approvals_json, last_active_room_by_agent_json, and associated types/functions) and simplify load/save to only manage next_chat_index and last_heartbeat_event. Add DB migration to create the new table and index, and delete tool approval rules on logout. Replace in-memory checks/persistence with DB-backed has/insert functions, update imports, and adjust related call sites (message status and AIRoomInfo now use agentremote). Also remove a now-unused portal reference cleanup call from chat deletion.
Update imports and type/function references to the agentremote SDK package (github.com/beeper/agentremote/sdk) across bridge code. Replace old/ambiguous imports and previous sdk/bridgesdk aliases with agentremote, and update types and calls (e.g. Turn, Writer, ApprovalRequest/ApprovalHandle/ToolApprovalResponse, TurnSnapshot/TurnData builders, SetRoomName/SetRoomTopic, ApplyDefaultCommandPrefix, BuildCompactFinalUIMessage, SnapshotFromTurnData, etc.). Tests and helpers were updated to match the SDK package reorganization and new import alias.
Unify and standardize bridge metadata and display names across connectors and entry points. Updated connector descriptions to reference the AgentRemote SDK, simplified DisplayName values (e.g. "Beeper Cloud" -> "AI", "OpenClaw Bridge" -> "OpenClaw Gateway", etc.), and adjusted bridge entry descriptions to match. Also changed OpenAI login StepIDs from the openai namespace to the ai namespace and updated the corresponding test to expect the new display name. No behavioral changes to network URLs or protocol IDs.
Update branding for the AI bridge: change the SDK config Description and DisplayName to "AI Chats bridge for Beeper" / "Beeper AI". Files updated: bridges/ai/constructors.go (Description, DisplayName), bridges/ai/constructors_test.go (expected DisplayName), and cmd/internal/bridgeentry/bridgeentry.go (Definition.Description). No network URLs or IDs were changed.
Add constants for AI DB table names and replace hardcoded table identifiers across AI database code (sessions, login state, internal messages, system events). Add NetworkCapabilities provisioning hints for AI, Codex, and OpenClaw connectors. Simplify agent ID normalization and system events/session handling (scoped login scope helpers, normalized agent ID filtering). Refactor OpenClaw to scope ghost IDs by login, include login in avatar IDs, persist and load OpenClaw login state (save/load device tokens and session sync state), and adjust session key resolution logic. Simplify DummyBridge by removing synthetic provisioning and unused helpers, and remove Codex host-auth reconciliation and related tests/helpers. Misc: remove unused imports and update tests to match the new APIs/behaviour.
Remove low-level escape hatches and legacy handler plumbing from the SDK. - Drop RawEvent/RawMsg/RawEdit/RawReaction fields from Message, MessageEdit and Reaction types and remove the event import. - Stop populating those raw fields in convertMatrixMessage. - Remove many SDK client handler implementations (edit, redaction, typing, room name/topic, backfilling, delete chat, identifier resolution, contact listing and search) and the related compile-time interface checks, leaving a single NetworkAPI check. - Update connector to use loginAwareClient when setting user login. - Delete the client_resolution_test.go unit tests that covered identifier resolution/contact listing/search. This simplifies the SDK surface by removing escape hatches and legacy handler boilerplate; callers should use higher-level APIs or implement needed handlers via the new configuration surface.
Add persistent storage for AI login configuration and SDK conversation state. Introduce aichats_login_config table and new load/save helpers (loadAIUserLoginConfig, saveAIUserLogin) in bridges/ai/login_config_db.go; update callers to use saveAIUserLogin so AI metadata is stored in DB and compacted on the runtime login row. Add SDK conversation state DB storage with sdk_conversation_state table and DB-backed load/save logic in sdk/conversation_state.go. Update SQL migration (pkg/aidb/001-init.sql) and tests to include the new table. Also tidy misc related changes: add aiLoginConfigTable const, propagate context to login loaders, normalize agent ID usages, simplify OpenCode login instance builders (remove unused instanceID), switch OpenClaw base64 output to base64.RawURLEncoding, and minor imports/formatting adjustments.
Introduce a DB-backed openClawPortalState for OpenClaw: add openClawPortalDBScope helper, ensureOpenClawPortalStateTable, load/save/clear functions, and replace catalog/metadata callers to use the portal state rather than in-meta fields. Refactor OpenClaw PortalMetadata into a minimal struct and move many runtime fields into openClawPortalState; update catalog logic to operate on state. Remove SDKPortalMetadata carriers and their getters/setters from dummybridge and opencode PortalMetadata (and drop the related test). Tighten SDK conversation state checks to handle nil portal.Portal, and update conversation state tests to use an in-memory sqlite Bridge DB and the DB-backed conversation state store; adjust test helpers and expectations accordingly.
Clarify and reformat portal metadata fields and persistable state: updated PortalMetadata comment, aligned field formatting, and adjusted persisted aiPersistedPortalState struct formatting. OpenClaw capability handling refactored: GetCapabilities now builds capabilities from a profile, added openClawCapabilitiesFromProfile, capabilityIDForPortalState and maybeRefreshPortalCapabilities to centralize capability ID generation and conditional refresh. Tests updated to use openClawPortalState (instead of PortalMetadata) and to reflect API changes in helpers that build OpenClaw message/history metadata. Also added IsOpenClawRoom field to openClawPortalState. Mostly mechanical changes to improve clarity and enable capability refresh logic.
Large refactor of the AI bridge: removed Matrix-specific helper files and pin/reaction state APIs, and simplified portal/room handling. Default chat creation and selection logic was streamlined (deterministic key handling removed), listAllChatPortals now queries the AI portal state table, and room name/topic setters were removed in favor of updating portal metadata directly. Reaction removal was consolidated to a new removeReaction flow that uses the bridge DB, and message delete/pin/list-pin handlers were removed. Added session transcript support and adjusted integration host DB access/renames and Codex client to use portal state records.
Cleanup: remove several unused helper functions and perform minor import/formatting tidy-ups. Removed clonePortalState (bridges/ai/portal_state_db.go), hostAuthLoginID, hasManagedCodexLogin, and resolveCodexCommand (bridges/codex/connector.go), and clearOpenClawPortalState (bridges/openclaw/metadata.go). Also adjusted imports and spacing in multiple sdk and bridge files (bridges/ai/session_store.go, bridges/ai/system_events_db.go, sdk/*) to improve code clarity. No functional behavior changes intended.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR migrates the codebase from the top-level agentremote APIs to the sdk package, introduces AI-specific persistent state (login, portal, transcript, internal prompts, tool rules) in aidb, replaces various metadata-based flows with DB-backed ones, refactors schedulers to in-process timers, streamlines portal/room updates, and updates bridges (AI, Codex, OpenClaw, OpenCode, DummyBridge), CLI texts, and tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client as AIClient
participant Portal as Portal
participant AIDB as AI DB (aidb)
participant Matrix as Matrix API
User->>Client: Send message
Client->>Portal: Ensure portal lifecycle/name
Client->>AIDB: Insert transport-safe bridge message (mapping)
Client->>AIDB: Upsert AI transcript message (full metadata)
Client-->>Matrix: SendMessageStatus(room, eventID, Pending)
Client->>AIDB: Update login/portal state as needed
Client-->>Matrix: SendMessageStatus(room, eventID, Success)
Note over Client,AIDB: Later history loads read from aiTranscriptTable
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180+ minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Rename user-facing references to "AgentRemote CLI" across workflows, README, docker docs, help text, and Homebrew cask generation. Introduce defaultCodexClientInfoName/title constants, apply them in the Codex connector and add a unit test to assert defaults. Add a user-agent base constant for OpenClaw Gateway and use it when building client identity and tests. Minor prompt and help string tweaks to clarify wording and usage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
bridges/ai/command_registry.go (1)
197-210:⚠️ Potential issue | 🟠 MajorRemove dead code
buildCommandDescriptionContentand its test.The
BroadcastCommandDescriptionsmethod no longer uses this function—it now directly constructssdk.Commandobjects instead. The only remaining usage is inevents_test.go:51, which tests the unused function directly. Remove both the function definition and the test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/command_registry.go` around lines 197 - 210, Remove the dead helper buildCommandDescriptionContent and its associated unit test that directly invokes it (the test in events_test that targets buildCommandDescriptionContent); update references so BroadcastCommandDescriptions continues to construct sdk.Command objects directly and delete both the function buildCommandDescriptionContent and the test file's test case that calls it. Ensure no other code references buildCommandDescriptionContent remain and run the test suite to confirm removal is safe.sdk/conversation_state.go (2)
75-106:⚠️ Potential issue | 🟠 MajorAvoid caching conversation state under the empty key.
When
portal.Portal == nil,conversationStateKeyreturns"", butgetandsetstill read/writerooms[""]. That lets unrelated partially initialized portals share cached state.Suggested fix
func (s *conversationStateStore) get(portal *bridgev2.Portal) *sdkConversationState { if s == nil || portal == nil { return &sdkConversationState{} } key := conversationStateKey(portal) + if key == "" { + return &sdkConversationState{} + } s.mu.RLock() state := s.rooms[key] s.mu.RUnlock() if state != nil { return state.clone() @@ func (s *conversationStateStore) set(portal *bridgev2.Portal, state *sdkConversationState) { if s == nil || portal == nil { return } key := conversationStateKey(portal) + if key == "" { + return + } s.mu.Lock() s.rooms[key] = state.clone() s.mu.Unlock() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/conversation_state.go` around lines 75 - 106, The code currently allows storing/reading under an empty key when conversationStateKey(portal) returns "", causing unrelated portals to share state; update conversationStateStore.get and conversationStateStore.set to treat an empty key as invalid: after computing key := conversationStateKey(portal) return a new empty sdkConversationState (without accessing s.rooms) if key == "" in get, and return immediately without writing to s.rooms if key == "" in set; reference conversationStateKey, conversationStateStore.get, conversationStateStore.set, s.rooms and ensure the same nil checks remain.
134-176:⚠️ Potential issue | 🔴 CriticalAdd a legacy-state fallback before switching reads to DB-only storage.
loadConversationStatenow consults only the cache andsdk_conversation_statetable. Existing deployments that still have conversation state in portal metadata will silently load defaults until something rewrites the room—a regression unless a migration backfills the data or a fallback read from legacy storage is implemented.The test explicitly validates that
portal.Metadataremains untouched after saving (line 65-66), confirming the migration path doesn't write to the old storage location. No backfill or legacy read fallback is present in the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/conversation_state.go` around lines 134 - 176, loadConversationState currently only checks the cache and DB, so add a legacy fallback: when state is still nil/default after consulting the store and DB (inside loadConversationState), inspect portal.Metadata for the legacy conversation-state entry (the key used previously for storing state in portal metadata), unmarshal that JSON into an sdkConversationState, and use it as the loaded state (without mutating portal.Metadata). After loading from legacy metadata, call state.ensureDefaults() and store.set(portal, state) as currently done so the rest of the code treats it like a normal load; keep loadConversationStateFromDB unchanged.pkg/aidb/db_test.go (1)
53-68:⚠️ Potential issue | 🟡 MinorAssert the rest of the new v1 tables here too.
pkg/aidb/001-init.sqlnow createsaichats_internal_messages,aichats_login_state, andaichats_tool_approval_rulesas part of the fresh schema, but this test doesn't verify them. If upgrade stops creating any of those tables,TestUpgradeV1Freshwill still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/aidb/db_test.go` around lines 53 - 68, TestUpgradeV1Fresh is missing assertions for newly added v1 tables; update the test (TestUpgradeV1Fresh in pkg/aidb/db_test.go) to include checks for the three tables created by pkg/aidb/001-init.sql: aichats_internal_messages, aichats_login_state, and aichats_tool_approval_rules so the fresh-schema path fails if any of those tables are not created during upgrade.sdk/conversation.go (1)
47-52:⚠️ Potential issue | 🔴 CriticalRestore the nil receiver guard in
getIntent.Line 51 now dereferences
cunconditionally. A nil*Conversationwill panic here before returning an error, and that propagates intoSendMedia,SendNotice,sendMessageContent, andSetTyping.🐛 Proposed fix
func (c *Conversation) getIntent(ctx context.Context) (bridgev2.MatrixAPI, error) { + if c == nil { + return nil, fmt.Errorf("conversation unavailable") + } if c != nil && c.intentOverride != nil { return c.intentOverride(ctx) } return resolveMatrixIntent(ctx, c.login, c.portal, c.sender, bridgev2.RemoteEventMessage) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/conversation.go` around lines 47 - 52, The getIntent method on Conversation now dereferences c unconditionally which will panic for a nil *Conversation and cascade into SendMedia, SendNotice, sendMessageContent, and SetTyping; restore the nil receiver guard by checking if c == nil at the top of Conversation.getIntent and return a sensible error (e.g., fmt.Errorf or a package-level err like ErrNoConversation) instead of calling resolveMatrixIntent, and keep the existing intentOverride branch (i.e., if c != nil && c.intentOverride != nil return c.intentOverride(ctx)); ensure callers that expect an error continue to propagate it.bridges/ai/login.go (1)
239-248:⚠️ Potential issue | 🟠 MajorAvoid returning an error after a successful
NewLoginwithout rollback.
ol.User.NewLogin(...)has already created the login beforesaveAIUserLogin(...)runs. If the second write fails, the user sees a failed login while the new login still exists, which can strand state and make retries/relogins produce duplicate or confusing accounts. Please make these writes atomic, or explicitly clean up the just-created login before returning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/login.go` around lines 239 - 248, The code calls ol.User.NewLogin(...) which creates a login and then calls saveAIUserLogin(...); if saveAIUserLogin fails you must roll back the created login to avoid stranded state. Modify the error path after saveAIUserLogin to call the appropriate deletion method (e.g., ol.User.DeleteLogin(ctx, loginID) or equivalent) to remove the created login (and handle/log any deletion error), then return the original wrapped save error; ensure you reference ol.User.NewLogin, saveAIUserLogin, and loginID when implementing the rollback so creation is undone on failure.bridges/ai/tool_approvals_rules.go (1)
59-80:⚠️ Potential issue | 🟠 MajorDon't use
context.Background()for approval-rule lookups.These checks are on the tool/approval path now that they hit the DB. Using
context.Background()makes them uncancellable, so a slow or wedged DB can stall message handling indefinitely. Please thread the caller context through these methods, or wrap the queries in a short timeout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/tool_approvals_rules.go` around lines 59 - 80, The approval checks isMcpAlwaysAllowed and isBuiltinAlwaysAllowed currently call hasToolApprovalRule/hasBuiltinToolApprovalRule with context.Background(), making DB lookups uncancellable; change these functions to accept a context.Context parameter (e.g., ctx) and pass that ctx into oc.hasToolApprovalRule and oc.hasBuiltinToolApprovalRule, or alternatively create a derived context with a short timeout inside each function and use that for the DB call; update all callers of isMcpAlwaysAllowed and isBuiltinAlwaysAllowed to supply the caller context so the DB queries are cancellable.bridges/ai/chat.go (1)
1095-1132:⚠️ Potential issue | 🟠 MajorKeep a legacy-room fallback before creating a new default chat.
This path now only checks
defaultChatPortalKey(...). For existing logins whose default AI room was created under an older key/selection path, bootstrap will create a second default room instead of reusing the current one.Falling back to
listAllChatPortals(...)/chooseDefaultChatPortal(...)before creating a new portal would preserve existing rooms during upgrade.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/chat.go` around lines 1095 - 1132, Before creating a new default portal, add a legacy-room fallback: after the GetExistingPortalByKey check fails (portal == nil) call oc.listAllChatPortals(ctx, oc.UserLogin) and pass the result into oc.chooseDefaultChatPortal(...) to see if an older portal should be reused; if chooseDefaultChatPortal returns a portal, reuse it (materialize if MXID missing via oc.materializePortalRoom) and return nil instead of proceeding to create a new portal with oc.initPortalForChat; keep existing logging behavior and error handling around materializePortalRoom and initPortalForChat, and reference the existing symbols GetExistingPortalByKey, listAllChatPortals, chooseDefaultChatPortal, materializePortalRoom, and initPortalForChat when making the change.bridges/codex/directory_manager.go (1)
133-164:⚠️ Potential issue | 🟠 MajorSave the welcome-room state before creating the Matrix room.
EnsurePortalLifecycle(...)can succeed beforesaveCodexPortalState(...)runs. If the save fails afterward, the room already exists but there is no persistedcodexPortalStateto find or repair it on restart.Suggested ordering
state := &codexPortalState{ Title: "New Codex Chat", Slug: "codex-welcome", AwaitingCwdSetup: true, } portalMeta(portal).IsCodexRoom = true + if err := saveCodexPortalState(ctx, portal, state); err != nil { + return nil, err + } if err := sdk.ConfigureDMPortal(ctx, sdk.ConfigureDMPortalParams{ Portal: portal, Title: state.Title, OtherUserID: codexGhostID, Save: false, }); err != nil { return nil, err } info := cc.composeCodexChatInfo(portal, state, false) created, err := sdk.EnsurePortalLifecycle(ctx, sdk.PortalLifecycleOptions{ Login: cc.UserLogin, Portal: portal, ChatInfo: info, SaveBeforeCreate: true, AIRoomKind: sdk.AIRoomKindAgent, ForceCapabilities: true, }) if err != nil { return nil, err } - if err := saveCodexPortalState(ctx, portal, state); err != nil { - return nil, err - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/codex/directory_manager.go` around lines 133 - 164, The current flow calls sdk.EnsurePortalLifecycle (in bridges/codex/directory_manager.go) before persisting codexPortalState with saveCodexPortalState, which can create the Matrix room without saved state; move the saveCodexPortalState call to occur immediately after initializing the codexPortalState and before calling sdk.EnsurePortalLifecycle, so the state is persisted (and errors returned) prior to creating the room; keep the calls to portalMeta(portal).IsCodexRoom = true and sdk.ConfigureDMPortal/cc.composeCodexChatInfo/sendSystemNotice intact, but ensure saveCodexPortalState is executed and its error handled before invoking EnsurePortalLifecycle to avoid orphaned rooms without persisted state.
🧹 Nitpick comments (13)
cmd/internal/bridgeentry/bridgeentry.go (1)
21-50: Inconsistent description format for AI bridge.The
AIdefinition uses a different description format ("AI Chats bridge for Beeper") compared to all other bridges which follow the pattern "X bridge built with the AgentRemote SDK." If this is intentional, consider adding a comment explaining why. Otherwise, align the format for consistency.AI = Definition{ Name: "ai", - Description: "AI Chats bridge for Beeper", + Description: "AI Chats bridge built with the AgentRemote SDK.", Port: 29345, DBName: "ai.db", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/internal/bridgeentry/bridgeentry.go` around lines 21 - 50, The AI Definition's Description ("AI Chats bridge for Beeper") is inconsistent with the other bridge Definitions (Codex, OpenCode, OpenClaw, DummyBridge) which use the "X bridge built with the AgentRemote SDK." pattern; update the AI variable's Description in the Definition for AI to match that pattern (e.g., "AI bridge built with the AgentRemote SDK.") or, if the different wording is intentional, add a brief inline comment above the AI Definition explaining the special case so reviewers understand the deviation.cmd/agentremote/profile.go (1)
26-42: Comments reference hardcoded "sdk" path but code uses dynamicbinaryName.The comments on lines 26 and 35 hardcode "sdk" in the path description, but the actual code uses the
binaryNamevariable. IfbinaryNamechanges in the future, these comments will be misleading.📝 Suggested fix to make comments more accurate
-// configRoot returns ~/.config/sdk +// configRoot returns ~/.config/<binaryName> func configRoot() (string, error) { home, err := os.UserHomeDir() if err != nil { return "", err } return filepath.Join(home, ".config", binaryName), nil } -// profileRoot returns ~/.config/sdk/profiles/<profile> +// profileRoot returns ~/.config/<binaryName>/profiles/<profile> func profileRoot(profile string) (string, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/agentremote/profile.go` around lines 26 - 42, Update the doc comments for configRoot and profileRoot to reflect that the path uses the dynamic binaryName variable rather than the hardcoded "sdk" string: change the comment above configRoot to indicate it returns ~/.config/<binaryName> and the comment above profileRoot to indicate it returns ~/.config/<binaryName>/profiles/<profile>, referencing the functions configRoot and profileRoot so future readers know the path is derived from binaryName.bridges/opencode/opencode_instance_state.go (1)
100-110: Consider deterministic ordering insessionIDs().Go map iteration order is non-deterministic; sorting
outbefore return can reduce flaky ordering in callers/tests/logs.Suggested refactor
func (inst *openCodeInstance) sessionIDs() []string { if inst == nil { return nil } inst.seenMu.Lock() defer inst.seenMu.Unlock() out := make([]string, 0, len(inst.knownSessions)) for sessionID := range inst.knownSessions { out = append(out, sessionID) } + sort.Strings(out) return out }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/opencode/opencode_instance_state.go` around lines 100 - 110, The sessionIDs() method returns session IDs in non-deterministic map order; lock seenMu, collect keys from inst.knownSessions as done, then sort the slice (e.g., using sort.Strings) before returning to ensure deterministic ordering for callers, tests, and logs; update the function (sessionIDs) to import the sort package and call sort.Strings(out) right before the return.pkg/shared/toolspec/toolspec.go (1)
143-145: Encode theemojirequirement in the schema, not just the description.Line 144 now says
emojiis required whenremove:true, but the schema still accepts a reaction-removal payload without it. If anything downstream relies on schema validation, malformedreactrequests will still get through. Consider adding anif/thenoroneOfrule for thereactaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/shared/toolspec/toolspec.go` around lines 143 - 145, The schema currently documents that "emoji" is required when "remove": true for the "react" action but doesn't enforce it; update the toolspec schema construction in toolspec.go (the code building properties like StringProperty("emoji") and BooleanProperty("remove") for the "react" action) to add a conditional JSON Schema rule that enforces emoji when remove is true — for example add an if/then clause scoped to action === "react" (if: {properties:{action:{const:"react"}, remove:{const:true}}} then: {required:["emoji"]}) or express the same via oneOf alternatives; ensure the new rule is attached to the same schema object used by the validator so malformed react/remove payloads are rejected.bridges/ai/identifiers.go (1)
171-171: Thread caller context throughportalMeta()to propagate cancellation for DB-backed portal state loads.Line 171 uses
context.Background()when callingloadPortalStateIntoMetadata(), preventing request cancellation/timeouts from propagating to the database operation. SinceportalMeta()is an accessor-style helper with 50+ call sites across the codebase, refactoring it to accept a context parameter would require updating all callers, though many already have context available in their call path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/identifiers.go` at line 171, The call to loadPortalStateIntoMetadata is using context.Background() inside portalMeta(), preventing cancellation/timeouts from propagating; change portalMeta to accept a context.Context parameter and propagate that ctx into loadPortalStateIntoMetadata (i.e., call loadPortalStateIntoMetadata(ctx, portal, meta) instead of using Background()), then update portalMeta call sites to pass the available request/context where present (and only use context.Background() as an explicit fallback in rare callers that truly lack a context), ensuring all DB-backed portal state loads honor cancellation.bridges/ai/logout_cleanup.go (2)
60-71: Inconsistent table name usage.Line 45 uses the
aiSessionsTablevariable for the sessions table, but these new deletions use hardcoded table names (aichats_internal_messages,aichats_tool_approval_rules,aichats_login_state). Consider using variables consistently for all AI-related tables to centralize table name definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/logout_cleanup.go` around lines 60 - 71, The three DELETEs use hardcoded table names; replace them with the centralized table-name variables (like the existing aiSessionsTable pattern) so table names are consistent and configurable; update the bestEffortExec calls to use the corresponding variables (e.g., internalMessagesTable, toolApprovalRulesTable, loginStateTable or whatever names are defined in the same package), and if those variables don't exist add them alongside aiSessionsTable where other AI table constants are declared so all DELETEs reference the shared table-name variables rather than literal strings.
72-74: Consider consolidating duplicate type assertion.The
(*AIClient)type assertion is performed twice: once at line 36 forpurgeLoginIntegrationsand again here at line 72 forclearLoginState. Consider calling both methods within a single type-assertion block to reduce redundancy.♻️ Proposed consolidation
if client, ok := login.Client.(*AIClient); ok && client != nil { client.purgeLoginIntegrations(ctx, login, bridgeID, loginID) + defer client.clearLoginState(ctx) } var logger *zerolog.LoggerThen remove the second type assertion block at lines 72-74.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/logout_cleanup.go` around lines 72 - 74, The code performs the same type assertion on login.Client to *AIClient twice; consolidate by doing a single assertion (e.g., if client, ok := login.Client.(*AIClient); ok && client != nil { ... }) and call both client.purgeLoginIntegrations(ctx) and client.clearLoginState(ctx) inside that block, then remove the duplicate assertion and the separate clearLoginState call so both methods run under one nil-checked *AIClient branch.sdk/conversation_test.go (1)
47-49: Prefer test-failure signaling overpanicin helper.Optional: pass
*testing.TintonewTestConversationand uset.Fatalf(...)so failures are attributed cleanly to the calling test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/conversation_test.go` around lines 47 - 49, The helper currently panics on save errors which obscures test attribution; modify the test helper (e.g., newTestConversation) to accept a *testing.T (or change it to return an error) and replace the panic(err) after conv.saveState(...) with t.Fatalf("saveState failed: %v", err) so failures are reported as test failures and attributed to the calling test; update all callers of newTestConversation to pass the *testing.T (or handle the returned error) accordingly.pkg/integrations/memory/sessions.go (1)
63-67: Unnecessary state save when hash matches.When the computed hash matches
state.contentHashand!force, the code saves the session state (line 64-66) even though nothing has changed. This appears redundant sincestatehasn't been modified at this point.💡 Suggested simplification
if !force && hash == state.contentHash { - if err := m.saveSessionState(ctx, key, state); err != nil { - m.log.Warn().Err(err).Str("session", key).Msg("memory session state save failed") - } continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/integrations/memory/sessions.go` around lines 63 - 67, The code redundantly calls m.saveSessionState(ctx, key, state) when !force and hash == state.contentHash even though state wasn't modified; remove the save call from that branch (the if-block in sessions.go that checks !force && hash == state.contentHash) so it simply continues, and ensure any necessary saves remain where state is actually mutated (keep m.saveSessionState only in places like saveSessionState invocations after state changes).bridges/ai/login_config_db.go (1)
69-84: Table creation uses safe constant, but consider schema migrations.The DDL uses
CREATE TABLE IF NOT EXISTSwhich is safe for initial creation. SinceaiLoginConfigTableappears to be a constant, there's no SQL injection risk. However, for production systems, consider using formal schema migrations for better version control and rollback capabilities.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/login_config_db.go` around lines 69 - 84, The current ensureAILoginConfigTable function contains inline DDL (CREATE TABLE IF NOT EXISTS) for aiLoginConfigTable; replace this ad-hoc creation with a proper schema migration: remove the inline CREATE TABLE from ensureAILoginConfigTable and instead register a migration (e.g., a new migration entry or function like migrateCreateAILoginConfigTable) that performs the DDL via your project's migration framework or migration runner, ensure the migration is idempotent and versioned, and update any bootstrap code that called ensureAILoginConfigTable to run/verify migrations (refer to symbols ensureAILoginConfigTable and aiLoginConfigTable to locate the code to change).bridges/openclaw/metadata.go (2)
127-143: Consider caching table creation status.
ensureOpenClawPortalStateTableexecutesCREATE TABLE IF NOT EXISTSon every load/save operation. While SQLite/PostgreSQL handle this idempotently, it adds unnecessary overhead.Consider using a
sync.Onceor a package-level flag to track whether the table has been created during this process lifetime.♻️ Optional: Cache table creation
+var openClawPortalStateTableCreated sync.Once + func ensureOpenClawPortalStateTable(ctx context.Context, portal *bridgev2.Portal, login *bridgev2.UserLogin) error { scope := openClawPortalDBScopeFor(portal, login) if scope == nil { return nil } - _, err := scope.db.Exec(ctx, ` + var err error + openClawPortalStateTableCreated.Do(func() { + _, err = scope.db.Exec(ctx, ` CREATE TABLE IF NOT EXISTS openclaw_portal_state ( ... ) - `) + `) + }) return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/metadata.go` around lines 127 - 143, The ensureOpenClawPortalStateTable function runs CREATE TABLE IF NOT EXISTS on every call; add a cheap in-process cache to avoid repeated DDL: introduce a package-level sync.Once or a map[<dbIdentifier>]bool guarded by a mutex to record that the openclaw_portal_state table has been created, check that cache at the top of ensureOpenClawPortalStateTable (using the same scope derived via openClawPortalDBScopeFor/ scope.db to derive a stable key), and only call scope.db.Exec when the cache says the table hasn’t been created yet, updating the cache after a successful Exec (or using Once.Do to run the Exec exactly once).
115-116: Portal key uses null byte separator.The composite key
string(portal.PortalKey.ID) + "\x00" + string(portal.PortalKey.Receiver)works but is unconventional. Null bytes in strings can cause issues with some tools/loggers. Consider using a printable delimiter like|or storing ID and Receiver as separate columns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/metadata.go` around lines 115 - 116, The code builds a composite portalKey using a null byte separator which can cause tooling/logging issues; replace the null ("\x00") separator in the expression that assigns portalKey (currently string(portal.PortalKey.ID) + "\x00" + string(portal.PortalKey.Receiver)) with a printable delimiter (e.g., "|" or another safe character) or, better, persist ID and Receiver as separate fields instead of a single concatenated string; update any consumers of portalKey to parse the new delimiter or to use the separate fields accordingly (look for usages of portalKey and portal.PortalKey.ID / portal.PortalKey.Receiver).bridges/ai/internal_prompt_db.go (1)
102-134: Consider filtering in SQL instead of post-fetch.Lines 125-127 and 132-134 filter rows after fetching. Moving these conditions to the WHERE clause would reduce data transfer and processing:
♻️ Proposed optimization
rows, err := scope.db.Query(ctx, ` SELECT event_id, canonical_turn_data, exclude_from_history, created_at_ms FROM `+aiInternalMessagesTable+` - WHERE bridge_id=$1 AND login_id=$2 AND room_id=$3 + WHERE bridge_id=$1 AND login_id=$2 AND room_id=$3 AND exclude_from_history=0 + AND ($5 = 0 OR created_at_ms >= $5) ORDER BY created_at_ms DESC, event_id DESC LIMIT $4 - `, scope.bridgeID, scope.loginID, portal.MXID.String(), limit) + `, scope.bridgeID, scope.loginID, portal.MXID.String(), limit, resetAt)Note: The
opts.excludeMessageIDfilter may still need post-fetch handling unless added as another WHERE clause.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/internal_prompt_db.go` around lines 102 - 134, Move the post-fetch filters into the SQL query to reduce rows returned: update the scope.db.Query call that selects from aiInternalMessagesTable to include "exclude_from_history = false" and, when resetAt > 0, add a "created_at_ms >= $N" predicate (and bind resetAt) so rows with excludeFromHistory and older createdAtMs are filtered out in SQL; keep the opts.excludeMessageID check in Go unless you also want to add it as another WHERE clause (in which case bind opts.excludeMessageID and compare event_id != $M). Target the Query call and the variables excludeFromHistory, createdAtMs, resetAt, and opts.excludeMessageID when applying these changes.
🤖 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/ai/agentstore.go`:
- Line 563: CreateRoom and ModifyRoom call b.client.savePortalQuiet which
swallows errors, causing those APIs to report success even when portal
persistence fails; change the call to use a persistence function that returns an
error (or modify savePortalQuiet to return an error instead of only logging) and
handle that error in CreateRoom and ModifyRoom: call
savePortal/savePortalWithError (or update savePortalQuiet to return error),
check the returned error, and if non-nil return an appropriate failure to the
caller instead of a success response.
In `@bridges/ai/command_registry.go`:
- Around line 176-194: The log call at the end of the command broadcasting block
uses len(handlers) which is incorrect after filtering; change the debug log to
report len(cmds) instead so it reflects the actual number broadcast. Locate the
block that builds cmds from handlers (variables: handlers, cmds) and the call to
sdk.BroadcastCommandDescriptions(ctx, portal, bot, cmds), then update the
log.Debug().Int("count", ...) argument to use len(cmds) and keep the same "room"
portal.MXID and message.
In `@bridges/ai/delete_chat.go`:
- Around line 73-82: The delete cleanup must also clear login-level portal
metadata so stale default-chat/last-active-room IDs don't point at the deleted
portal; add a bestEffortExec call (same pattern as the existing bestEffortExec
calls that delete from aiSessionsTable and aichats_system_events) to UPDATE the
login record(s) for the same bridgeID/loginID and set the default-chat and
last-active-room fields to NULL (or their zero values) before/after
deleteInternalPromptsForRoom; locate the cleanup block referencing
bestEffortExec, aiSessionsTable, deleteInternalPromptsForRoom, and
id.RoomID(sessionKey) and add the SQL UPDATE against the login table clearing
the relevant columns.
In `@bridges/ai/handleai.go`:
- Around line 315-317: The code currently sets currentMeta.AutoGreetingSent (and
similarly WelcomeSent) and calls oc.savePortalQuiet but ignores save failures
before calling oc.dispatchInternalMessage, which can cause sent messages without
persisted flags; change the flow so oc.savePortalQuiet returns an error (or use
an existing error-returning save variant) and check its result: after setting
currentMeta.AutoGreetingSent (and for WelcomeSent in the other block) call
oc.savePortalQuiet (or a new savePortal that returns error) and if it returns an
error immediately return/bail instead of proceeding to
oc.dispatchInternalMessage; ensure you update both the AutoGreetingSent branch
(where currentMeta.AutoGreetingSent is set) and the WelcomeSent branch (around
the 383-387 area) to persist first and only dispatch the one-shot message on
successful save.
- Around line 452-459: The code updates portal.Name and portal.NameSet then
calls savePortalQuiet which only persists to DB; to sync the generated title to
the Matrix room you must call the Matrix-sync path after saving — either invoke
materializePortalRoom(bgCtx, portal, ...) (as used elsewhere when returning
PortalInfo) or call oc.sdk.EnsurePortalLifecycle(bgCtx, portal) (as in
scheduler_rooms.go) so the room state is updated; add the appropriate call
immediately after oc.savePortalQuiet(bgCtx, portal, "room title") so the DB
change is also propagated to the Matrix room.
In `@bridges/ai/integration_host.go`:
- Around line 238-272: SessionTranscript currently calls CountMessagesInPortal
then fetches that many messages with GetLastNInPortal which can load thousands
into memory; cap the number fetched (e.g., limit := min(count, 500) or a
configurable constant) and call GetLastNInPortal with that limit instead of
count to prevent unbounded memory use. Update the logic around
CountMessagesInPortal and the call to GetLastNInPortal in
runtimeIntegrationHost.SessionTranscript (and ensure
h.client.backgroundContext(ctx) is still passed) so only the capped number of
recent messages are retrieved.
In `@bridges/ai/login_config_db.go`:
- Around line 139-143: The swap-and-save pattern around
compactAIUserLoginMetadata and login.Save can leave the system inconsistent
because the separate custom DB upsert (the AI login config table) may succeed
while login.Save fails; to fix, make both updates atomic: if both tables share
the same DB, execute the custom upsert and the login.Save inside a single
transaction (use the DB transaction API surrounding the upsert and the call to
login.Save), otherwise implement a compensating rollback for the custom upsert
(e.g., delete or restore the previous row) when login.Save returns an error;
locate and change code around compactAIUserLoginMetadata, login.Save(ctx) and
the custom upsert logic so both succeed or the earlier change is reverted.
In `@bridges/ai/portal_state_db.go`:
- Around line 165-170: The current error handling block incorrectly hides
database errors by checking strings.TrimSpace(raw) in the same OR with err ==
sql.ErrNoRows; move and split checks so real Scan() errors are returned: first
check if err != nil and return err, then handle the no-row case (err ==
sql.ErrNoRows) and separately check if strings.TrimSpace(raw) == "" to return
nil,nil; reference the variables err and raw and the sql.ErrNoRows comparison in
the error-handling block around the Scan()/query result processing.
In `@bridges/ai/prompt_builder.go`:
- Around line 152-171: The current code merges internalRows into candidates and
then applies hr.limit to the combined slice, allowing internal prompts to evict
recent chat messages and corrupt image-injection indexing; fix by treating
internal rows separately: when building candidates, create a separate
internalCandidates slice from loadInternalPromptHistory and keep chatCandidates
for user/assistant messages, then merge for ordering but when enforcing hr.limit
only count/trim chatCandidates (preserve internalCandidates regardless of
hr.limit), and update any image-injection logic that uses the loop index `i` to
instead use a counter of non-internal chat entries; reference
loadInternalPromptHistory, internalRows, candidates, replayCandidate
(id/role/ts/messages), hr.limit and the sort/trimming block when making this
change.
In `@bridges/ai/tools.go`:
- Around line 390-398: The comment and logic in executeMessageReact disagree:
empty emoji is being treated as a removal but executeMessageReactRemove requires
an explicit emoji. Update executeMessageReact to only call
executeMessageReactRemove when the remove flag is true (i.e., change the
condition from `remove || emoji == ""` to `remove`) and instead validate/return
an error when emoji is empty and remove is false; also update the top comment to
reflect that removals require remove:true (or explicit emoji in the args) and
reference executeMessageReactRemove in the comment so callers know which path
needs an explicit emoji.
In `@bridges/codex/directory_manager.go`:
- Around line 354-369: The code flips state.AwaitingCwdSetup to false and saves
the portal state before calling cc.ensureRPC and cc.ensureCodexThread, which can
leave the room out of welcome mode if those calls fail; either move the state
mutations (CodexCwd, CodexThreadID, AwaitingCwdSetup, ManagedImport, Title,
Slug) and the saveCodexPortalState call to after cc.ensureCodexThread succeeds,
or capture the prior state before mutating and, if cc.ensureRPC or
cc.ensureCodexThread returns an error, call saveCodexPortalState to restore the
previous state so the room remains in welcome mode; references:
state.AwaitingCwdSetup, saveCodexPortalState, cc.ensureRPC,
cc.ensureCodexThread, and cc.syncCodexRoomTopic.
In `@bridges/openclaw/catalog.go`:
- Around line 94-104: The persisted state fields OpenClawDefaultAgentID,
OpenClawKnownModelCount, OpenClawToolCount and OpenClawToolProfile must be reset
before re-enriching so stale values aren’t left when lookups are empty or the
default/selected agent changes; update the code around oc.agentDefaultID(),
state.OpenClawAgentID/ state.OpenClawDMTargetAgentID (used with
stringutil.TrimDefault), loadModelCatalog(ctx, false) and loadToolsCatalog(ctx,
agentID, false) to clear state.OpenClawDefaultAgentID (set to ""),
state.OpenClawKnownModelCount (set to 0), and
state.OpenClawToolCount/state.OpenClawToolProfile (set to 0 and nil/empty)
before attempting the loads, then repopulate them only when the corresponding
loads return non-empty results and use summarizeToolsCatalog only when catalog
is non-nil.
In `@bridges/openclaw/identifiers.go`:
- Around line 33-39: The current ID format built by openClawScopedGhostUserID
uses url.PathEscape but still allows colons to survive, causing ambiguous
parsing; change the encoding to an unambiguous one (e.g., hex or base64 URL-safe
encoding like base64.RawURLEncoding or hex.EncodeToString) for both the loginID
and agentID when constructing the string and add a versioned prefix (e.g.,
"v1:openclaw-agent:") to the ID; update the corresponding parser (the function
that parses/scans scoped ghost IDs) to decode the chosen encoding and honor the
version prefix so splitting on ":" is safe and unambiguous.
In `@bridges/openclaw/login.go`:
- Around line 262-269: If persisting credentials via saveOpenClawLoginState
fails after sdk.CreateAndCompleteLogin succeeded, implement a rollback or
coordination: either wrap the create+persist in a transaction/atomic operation
or, on saveOpenClawLoginState error, call the SDK cleanup to remove the newly
created login (e.g., invoke a delete/revert API for the created login using the
login ID) so you don't leave a half-created login without credentials; reference
the saveOpenClawLoginState call, the sdk.CreateAndCompleteLogin invocation, and
the openClawPersistedLoginState struct when adding the rollback/transaction
logic.
In `@bridges/openclaw/manager.go`:
- Around line 596-619: The code updates only the local sessionKey after
resolving a synthetic DM but doesn't persist it, so racey events can miss the
real session; before calling gateway.SendMessage(...) set
state.OpenClawSessionKey = strings.TrimSpace(resolvedKey) (or the trimmed
sessionKey if no resolve) and persist the portal/state using the existing
persistence method in this manager (so the saved state reflects the real session
key), ensuring the saved key is in place before SendMessage; alternatively, if
you prefer sync refresh, call m.syncSessions synchronously (using
m.client.BackgroundContext(ctx)) immediately after setting/persisting the key
and before SendMessage so the portal state is consistent.
In `@bridges/openclaw/provisioning.go`:
- Around line 282-322: The code may create the Matrix room via
sdk.EnsurePortalLifecycle before the updated OpenClaw state is durably persisted
and before portalMeta(portal).IsOpenClawRoom is set; to fix, persist the updated
state and set the in-memory marker before calling sdk.EnsurePortalLifecycle:
after oc.enrichPortalState(ctx, state) call save the state with
saveOpenClawPortalState(ctx, portal, oc.UserLogin, state) and set
portalMeta(portal).IsOpenClawRoom = true, handling and returning any error
immediately, then proceed to call sdk.EnsurePortalLifecycle; this ensures
failure to persist aborts before the room is created and the in-memory marker is
set consistently.
In `@pkg/integrations/memory/integration.go`:
- Around line 168-174: The PurgeForLogin implementation currently returns before
shutting down in-process managers when resolveStateDB() is nil; move the call to
StopManagersForLogin(scope.BridgeID, scope.LoginID) so it runs unconditionally
before the nil check, then keep the existing nil return and the subsequent
PurgeTables(ctx, db, scope.BridgeID, scope.LoginID) call when a DB exists;
update the function in Integration (PurgeForLogin) that calls resolveStateDB(),
StopManagersForLogin and PurgeTables to ensure managers are always stopped even
if MemoryStateDB() is unavailable.
---
Outside diff comments:
In `@bridges/ai/chat.go`:
- Around line 1095-1132: Before creating a new default portal, add a legacy-room
fallback: after the GetExistingPortalByKey check fails (portal == nil) call
oc.listAllChatPortals(ctx, oc.UserLogin) and pass the result into
oc.chooseDefaultChatPortal(...) to see if an older portal should be reused; if
chooseDefaultChatPortal returns a portal, reuse it (materialize if MXID missing
via oc.materializePortalRoom) and return nil instead of proceeding to create a
new portal with oc.initPortalForChat; keep existing logging behavior and error
handling around materializePortalRoom and initPortalForChat, and reference the
existing symbols GetExistingPortalByKey, listAllChatPortals,
chooseDefaultChatPortal, materializePortalRoom, and initPortalForChat when
making the change.
In `@bridges/ai/command_registry.go`:
- Around line 197-210: Remove the dead helper buildCommandDescriptionContent and
its associated unit test that directly invokes it (the test in events_test that
targets buildCommandDescriptionContent); update references so
BroadcastCommandDescriptions continues to construct sdk.Command objects directly
and delete both the function buildCommandDescriptionContent and the test file's
test case that calls it. Ensure no other code references
buildCommandDescriptionContent remain and run the test suite to confirm removal
is safe.
In `@bridges/ai/login.go`:
- Around line 239-248: The code calls ol.User.NewLogin(...) which creates a
login and then calls saveAIUserLogin(...); if saveAIUserLogin fails you must
roll back the created login to avoid stranded state. Modify the error path after
saveAIUserLogin to call the appropriate deletion method (e.g.,
ol.User.DeleteLogin(ctx, loginID) or equivalent) to remove the created login
(and handle/log any deletion error), then return the original wrapped save
error; ensure you reference ol.User.NewLogin, saveAIUserLogin, and loginID when
implementing the rollback so creation is undone on failure.
In `@bridges/ai/tool_approvals_rules.go`:
- Around line 59-80: The approval checks isMcpAlwaysAllowed and
isBuiltinAlwaysAllowed currently call
hasToolApprovalRule/hasBuiltinToolApprovalRule with context.Background(), making
DB lookups uncancellable; change these functions to accept a context.Context
parameter (e.g., ctx) and pass that ctx into oc.hasToolApprovalRule and
oc.hasBuiltinToolApprovalRule, or alternatively create a derived context with a
short timeout inside each function and use that for the DB call; update all
callers of isMcpAlwaysAllowed and isBuiltinAlwaysAllowed to supply the caller
context so the DB queries are cancellable.
In `@bridges/codex/directory_manager.go`:
- Around line 133-164: The current flow calls sdk.EnsurePortalLifecycle (in
bridges/codex/directory_manager.go) before persisting codexPortalState with
saveCodexPortalState, which can create the Matrix room without saved state; move
the saveCodexPortalState call to occur immediately after initializing the
codexPortalState and before calling sdk.EnsurePortalLifecycle, so the state is
persisted (and errors returned) prior to creating the room; keep the calls to
portalMeta(portal).IsCodexRoom = true and
sdk.ConfigureDMPortal/cc.composeCodexChatInfo/sendSystemNotice intact, but
ensure saveCodexPortalState is executed and its error handled before invoking
EnsurePortalLifecycle to avoid orphaned rooms without persisted state.
In `@pkg/aidb/db_test.go`:
- Around line 53-68: TestUpgradeV1Fresh is missing assertions for newly added v1
tables; update the test (TestUpgradeV1Fresh in pkg/aidb/db_test.go) to include
checks for the three tables created by pkg/aidb/001-init.sql:
aichats_internal_messages, aichats_login_state, and aichats_tool_approval_rules
so the fresh-schema path fails if any of those tables are not created during
upgrade.
In `@sdk/conversation_state.go`:
- Around line 75-106: The code currently allows storing/reading under an empty
key when conversationStateKey(portal) returns "", causing unrelated portals to
share state; update conversationStateStore.get and conversationStateStore.set to
treat an empty key as invalid: after computing key :=
conversationStateKey(portal) return a new empty sdkConversationState (without
accessing s.rooms) if key == "" in get, and return immediately without writing
to s.rooms if key == "" in set; reference conversationStateKey,
conversationStateStore.get, conversationStateStore.set, s.rooms and ensure the
same nil checks remain.
- Around line 134-176: loadConversationState currently only checks the cache and
DB, so add a legacy fallback: when state is still nil/default after consulting
the store and DB (inside loadConversationState), inspect portal.Metadata for the
legacy conversation-state entry (the key used previously for storing state in
portal metadata), unmarshal that JSON into an sdkConversationState, and use it
as the loaded state (without mutating portal.Metadata). After loading from
legacy metadata, call state.ensureDefaults() and store.set(portal, state) as
currently done so the rest of the code treats it like a normal load; keep
loadConversationStateFromDB unchanged.
In `@sdk/conversation.go`:
- Around line 47-52: The getIntent method on Conversation now dereferences c
unconditionally which will panic for a nil *Conversation and cascade into
SendMedia, SendNotice, sendMessageContent, and SetTyping; restore the nil
receiver guard by checking if c == nil at the top of Conversation.getIntent and
return a sensible error (e.g., fmt.Errorf or a package-level err like
ErrNoConversation) instead of calling resolveMatrixIntent, and keep the existing
intentOverride branch (i.e., if c != nil && c.intentOverride != nil return
c.intentOverride(ctx)); ensure callers that expect an error continue to
propagate it.
---
Nitpick comments:
In `@bridges/ai/identifiers.go`:
- Line 171: The call to loadPortalStateIntoMetadata is using
context.Background() inside portalMeta(), preventing cancellation/timeouts from
propagating; change portalMeta to accept a context.Context parameter and
propagate that ctx into loadPortalStateIntoMetadata (i.e., call
loadPortalStateIntoMetadata(ctx, portal, meta) instead of using Background()),
then update portalMeta call sites to pass the available request/context where
present (and only use context.Background() as an explicit fallback in rare
callers that truly lack a context), ensuring all DB-backed portal state loads
honor cancellation.
In `@bridges/ai/internal_prompt_db.go`:
- Around line 102-134: Move the post-fetch filters into the SQL query to reduce
rows returned: update the scope.db.Query call that selects from
aiInternalMessagesTable to include "exclude_from_history = false" and, when
resetAt > 0, add a "created_at_ms >= $N" predicate (and bind resetAt) so rows
with excludeFromHistory and older createdAtMs are filtered out in SQL; keep the
opts.excludeMessageID check in Go unless you also want to add it as another
WHERE clause (in which case bind opts.excludeMessageID and compare event_id !=
$M). Target the Query call and the variables excludeFromHistory, createdAtMs,
resetAt, and opts.excludeMessageID when applying these changes.
In `@bridges/ai/login_config_db.go`:
- Around line 69-84: The current ensureAILoginConfigTable function contains
inline DDL (CREATE TABLE IF NOT EXISTS) for aiLoginConfigTable; replace this
ad-hoc creation with a proper schema migration: remove the inline CREATE TABLE
from ensureAILoginConfigTable and instead register a migration (e.g., a new
migration entry or function like migrateCreateAILoginConfigTable) that performs
the DDL via your project's migration framework or migration runner, ensure the
migration is idempotent and versioned, and update any bootstrap code that called
ensureAILoginConfigTable to run/verify migrations (refer to symbols
ensureAILoginConfigTable and aiLoginConfigTable to locate the code to change).
In `@bridges/ai/logout_cleanup.go`:
- Around line 60-71: The three DELETEs use hardcoded table names; replace them
with the centralized table-name variables (like the existing aiSessionsTable
pattern) so table names are consistent and configurable; update the
bestEffortExec calls to use the corresponding variables (e.g.,
internalMessagesTable, toolApprovalRulesTable, loginStateTable or whatever names
are defined in the same package), and if those variables don't exist add them
alongside aiSessionsTable where other AI table constants are declared so all
DELETEs reference the shared table-name variables rather than literal strings.
- Around line 72-74: The code performs the same type assertion on login.Client
to *AIClient twice; consolidate by doing a single assertion (e.g., if client, ok
:= login.Client.(*AIClient); ok && client != nil { ... }) and call both
client.purgeLoginIntegrations(ctx) and client.clearLoginState(ctx) inside that
block, then remove the duplicate assertion and the separate clearLoginState call
so both methods run under one nil-checked *AIClient branch.
In `@bridges/openclaw/metadata.go`:
- Around line 127-143: The ensureOpenClawPortalStateTable function runs CREATE
TABLE IF NOT EXISTS on every call; add a cheap in-process cache to avoid
repeated DDL: introduce a package-level sync.Once or a map[<dbIdentifier>]bool
guarded by a mutex to record that the openclaw_portal_state table has been
created, check that cache at the top of ensureOpenClawPortalStateTable (using
the same scope derived via openClawPortalDBScopeFor/ scope.db to derive a stable
key), and only call scope.db.Exec when the cache says the table hasn’t been
created yet, updating the cache after a successful Exec (or using Once.Do to run
the Exec exactly once).
- Around line 115-116: The code builds a composite portalKey using a null byte
separator which can cause tooling/logging issues; replace the null ("\x00")
separator in the expression that assigns portalKey (currently
string(portal.PortalKey.ID) + "\x00" + string(portal.PortalKey.Receiver)) with a
printable delimiter (e.g., "|" or another safe character) or, better, persist ID
and Receiver as separate fields instead of a single concatenated string; update
any consumers of portalKey to parse the new delimiter or to use the separate
fields accordingly (look for usages of portalKey and portal.PortalKey.ID /
portal.PortalKey.Receiver).
In `@bridges/opencode/opencode_instance_state.go`:
- Around line 100-110: The sessionIDs() method returns session IDs in
non-deterministic map order; lock seenMu, collect keys from inst.knownSessions
as done, then sort the slice (e.g., using sort.Strings) before returning to
ensure deterministic ordering for callers, tests, and logs; update the function
(sessionIDs) to import the sort package and call sort.Strings(out) right before
the return.
In `@cmd/agentremote/profile.go`:
- Around line 26-42: Update the doc comments for configRoot and profileRoot to
reflect that the path uses the dynamic binaryName variable rather than the
hardcoded "sdk" string: change the comment above configRoot to indicate it
returns ~/.config/<binaryName> and the comment above profileRoot to indicate it
returns ~/.config/<binaryName>/profiles/<profile>, referencing the functions
configRoot and profileRoot so future readers know the path is derived from
binaryName.
In `@cmd/internal/bridgeentry/bridgeentry.go`:
- Around line 21-50: The AI Definition's Description ("AI Chats bridge for
Beeper") is inconsistent with the other bridge Definitions (Codex, OpenCode,
OpenClaw, DummyBridge) which use the "X bridge built with the AgentRemote SDK."
pattern; update the AI variable's Description in the Definition for AI to match
that pattern (e.g., "AI bridge built with the AgentRemote SDK.") or, if the
different wording is intentional, add a brief inline comment above the AI
Definition explaining the special case so reviewers understand the deviation.
In `@pkg/integrations/memory/sessions.go`:
- Around line 63-67: The code redundantly calls m.saveSessionState(ctx, key,
state) when !force and hash == state.contentHash even though state wasn't
modified; remove the save call from that branch (the if-block in sessions.go
that checks !force && hash == state.contentHash) so it simply continues, and
ensure any necessary saves remain where state is actually mutated (keep
m.saveSessionState only in places like saveSessionState invocations after state
changes).
In `@pkg/shared/toolspec/toolspec.go`:
- Around line 143-145: The schema currently documents that "emoji" is required
when "remove": true for the "react" action but doesn't enforce it; update the
toolspec schema construction in toolspec.go (the code building properties like
StringProperty("emoji") and BooleanProperty("remove") for the "react" action) to
add a conditional JSON Schema rule that enforces emoji when remove is true — for
example add an if/then clause scoped to action === "react" (if:
{properties:{action:{const:"react"}, remove:{const:true}}} then:
{required:["emoji"]}) or express the same via oneOf alternatives; ensure the new
rule is attached to the same schema object used by the validator so malformed
react/remove payloads are rejected.
In `@sdk/conversation_test.go`:
- Around line 47-49: The helper currently panics on save errors which obscures
test attribution; modify the test helper (e.g., newTestConversation) to accept a
*testing.T (or change it to return an error) and replace the panic(err) after
conv.saveState(...) with t.Fatalf("saveState failed: %v", err) so failures are
reported as test failures and attributed to the calling test; update all callers
of newTestConversation to pass the *testing.T (or handle the returned error)
accordingly.
🪄 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 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: build-agentremote-docker (amd64)
- GitHub Check: build-docker
- GitHub Check: Lint
🧰 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/ai/integrations.gobridges/ai/abort_helpers_test.gobridges/ai/bridge_info.gobridges/ai/client_capabilities_test.gobridges/ai/command_registry.gobridges/ai/constructors_test.gocmd/agentremote/run_bridge.gobridges/ai/agentstore.gobridges/dummybridge/runtime_test.gosdk/connector_builder_test.gobridges/ai/bridge_db.gobridges/ai/login_loaders.gobridges/codex/connector.gobridges/codex/constructors.gobridges/ai/commands.gobridges/codex/runtime_helpers.gobridges/ai/connector.gosdk/connector_builder.gobridges/ai/constructors.gobridges/ai/sdk_agent_catalog.gobridges/dummybridge/connector.gobridges/ai/handleai.gobridges/opencode/connector.gobridges/openclaw/connector.gocmd/internal/bridgeentry/bridgeentry.gosdk/connector.gosdk/types.gobridges/dummybridge/runtime.gobridges/ai/client.go
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
bridges/codex/constructors.go (1)
42-55:⚠️ Potential issue | 🟠 MajorGuard
aidb.EnsureSchemawhen the bridge DB is unavailable.
InitConnectorconditionally initializescc.dbonly whenbridge,bridge.DB, andbridge.DB.Databaseare all non-nil. However,StartConnectorunconditionally passes the result ofcc.bridgeDB()toaidb.EnsureSchema, which explicitly errors on nil input rather than short-circuiting. This causes startup failures in no-DB scenarios.Suggested fix
StartConnector: func(ctx context.Context, _ *bridgev2.Bridge) error { db := cc.bridgeDB() - if err := aidb.EnsureSchema(ctx, db); err != nil { - return err + if db != nil { + if err := aidb.EnsureSchema(ctx, db); err != nil { + return err + } } cc.applyRuntimeDefaults() sdk.PrimeUserLoginCache(ctx, cc.br) return nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/codex/constructors.go` around lines 42 - 55, StartConnector calls aidb.EnsureSchema with cc.bridgeDB() even when InitConnector left cc.db nil; wrap the EnsureSchema call in a nil-check so it only runs when cc.bridgeDB() != nil (e.g. assign db := cc.bridgeDB(); if db == nil { return nil } else if err := aidb.EnsureSchema(ctx, db); err != nil { return err }), ensuring StartConnector short-circuits cleanly in no-DB scenarios and avoids passing nil to aidb.EnsureSchema.bridges/ai/chat.go (2)
46-52:⚠️ Potential issue | 🟠 MajorTreat an unset
Agentsflag as enabled.This helper returns
falsewhencfg.Agentsis nil, butshouldEnsureDefaultChatstill treats nil as “enabled”. That leaves default-config logins in a split state where bootstrap creates the agent chat while search/create-chat flows reject agents.Suggested fix
func (oc *AIClient) agentsEnabledForLogin() bool { if oc == nil || oc.UserLogin == nil { return false } cfg := oc.loginConfigSnapshot(context.Background()) - return cfg.Agents != nil && *cfg.Agents + return cfg.Agents == nil || *cfg.Agents }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/chat.go` around lines 46 - 52, agentsEnabledForLogin currently returns false when cfg.Agents is nil, causing a mismatch with shouldEnsureDefaultChat which treats nil as enabled; update agentsEnabledForLogin (in AIClient) to treat a nil Agents pointer as enabled by returning true when cfg.Agents == nil, otherwise return the boolean value (*cfg.Agents) from the loginConfigSnapshot result so default-config logins consistently enable agents.
239-268:⚠️ Potential issue | 🟠 MajorReturn the bridge ghost ID for agent contacts.
agentContactResponsenow exposesnetworkid.UserID(agent.ID), but the rest of the bridge consistently usesoc.agentUserID(agentID)for agent ghosts. Search/contact results can therefore hand back an ID that doesn't hydrate to the actual ghost or open the right DM.Suggested fix
func (oc *AIClient) agentContactResponse(ctx context.Context, agent *sdk.Agent) *bridgev2.ResolveIdentifierResponse { if agent == nil || !oc.agentsEnabledForLogin() { return nil } + userID := networkid.UserID(agent.ID) + if agentID := catalogAgentID(agent); agentID != "" { + userID = oc.agentUserID(agentID) + } resp := &bridgev2.ResolveIdentifierResponse{ - UserID: networkid.UserID(agent.ID), + UserID: userID, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/chat.go` around lines 239 - 268, The response currently sets resp.UserID to networkid.UserID(agent.ID) which doesn't match how agent ghosts are stored; update agentContactResponse so the UserID returned for agent contacts uses the bridge ghost ID via oc.agentUserID(agentID) (using the catalogAgentID result) instead of networkid.UserID(agent.ID); if catalogAgentID(agent) is empty, keep the existing fallback behavior, and ensure this change happens before calling UserLogin.Bridge.GetGhostByID so the ghost hydrate uses the correct ID (refer to functions agentContactResponse, catalogAgentID, oc.ResolveResponderForAgent, and oc.agentUserID).bridges/ai/metadata.go (1)
313-325:⚠️ Potential issue | 🟠 Major
cloneUserLoginMetadatanow strips all transient login state.After the tag changes above, this JSON round-trip preserves essentially only
Provider. Any caller that clones before mutating or persisting login state will silently lose credentials, caches, profile data, custom agents, and error counters. This needs an explicit clone path for the non-serialized fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/metadata.go` around lines 313 - 325, cloneUserLoginMetadata performs a JSON round-trip which strips non-serialized/transient fields (credentials, caches, profile data, custom agents, error counters), so update cloneUserLoginMetadata to perform the JSON marshal/unmarshal for the serializable fields and then explicitly copy the transient fields from src into the resulting clone (e.g., copy Credentials, Cache, Profile, CustomAgents, ErrorCounters or their pointer/deep copies as appropriate) so callers that clone before mutating/persisting don't lose runtime state; locate and modify cloneUserLoginMetadata to perform the manual transfer after unmarshal and ensure pointer safety (deep-copy if needed) before returning the clone.
♻️ Duplicate comments (5)
bridges/ai/handleai.go (3)
387-390:⚠️ Potential issue | 🟠 MajorPersist
WelcomeSentbefore sending the notice.This has the same one-shot persistence gap as the auto-greeting path: if the save doesn't stick, the welcome notice can be emitted again later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handleai.go` around lines 387 - 390, Set meta.WelcomeSent = true and persist that change with oc.savePortalQuiet using a cancellable context (bgCtx from context.WithTimeout(oc.backgroundContext(ctx), 10*time.Second)) before any code that emits the welcome notice; ensure you call oc.savePortalQuiet immediately after setting meta.WelcomeSent and check/handle its error (log/stop the notice emission) so the notice is only sent if the persistence succeeds.
319-321:⚠️ Potential issue | 🟠 MajorPersist
AutoGreetingSentbefore dispatching the greeting.This still sends the one-shot greeting even if the flag wasn't durably saved. A restart or retry can resend it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handleai.go` around lines 319 - 321, The AutoGreetingSent flag is being set in-memory but not guaranteed persisted before sending the greeting; change the order and add error handling so persistence happens first: set currentMeta.AutoGreetingSent = true, call oc.savePortalQuiet(bgCtx, current, "auto greeting state") and check its error (do not call oc.dispatchInternalMessage if save failed), logging or returning the save error; only after a successful save call oc.dispatchInternalMessage(..., "auto-greeting", true). This uses the existing symbols currentMeta.AutoGreetingSent, oc.savePortalQuiet, and oc.dispatchInternalMessage.
456-463:⚠️ Potential issue | 🟠 MajorSync the generated title to Matrix after saving it.
Lines 461-463 only update
portal.Name/NameSetand persist the portal record. Without a lifecycle/materialization sync, clients won't see the generated room title update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handleai.go` around lines 456 - 463, The portal title is only persisted locally (portal.Name/portal.NameSet and oc.savePortalQuiet) but not pushed to Matrix; after oc.savePortalQuiet(bgCtx, portal, "room title") call the existing lifecycle/materialization sync that pushes portal metadata to Matrix (i.e., the oc method responsible for materializing/syncing portals) so the updated portal.Title/portal.Name is sent to clients; if no such helper exists, add and call a method like oc.MaterializePortal(ctx, portal) or oc.SyncPortalTitleToMatrix(ctx, portal) immediately after saving to trigger the external update.bridges/ai/tools.go (1)
390-397:⚠️ Potential issue | 🟡 MinorRequire
remove:truebefore routing to reaction removal.Line 396 still treats an empty
emojias a removal request. That sends malformed add-reaction calls into the remove handler instead of rejecting them up front, and it still doesn't match the comment about “explicit emoji” removal.Suggested fix
-// Supports adding reactions (with emoji) and removing reactions (with remove:true or explicit emoji). +// Supports adding reactions with `emoji` and removing reactions with `remove:true`. func executeMessageReact(ctx context.Context, args map[string]any, btc *BridgeToolContext) (string, error) { emoji, _ := args["emoji"].(string) remove, _ := args["remove"].(bool) // Check if this is a removal request. - if remove || emoji == "" { + if remove { return executeMessageReactRemove(ctx, args, btc) } + if strings.TrimSpace(emoji) == "" { + return "", errors.New("action=react requires 'emoji' unless remove=true") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/tools.go` around lines 390 - 397, The routing in executeMessageReact incorrectly treats an empty emoji as a removal request; change the conditional so only remove == true forwards to executeMessageReactRemove (i.e., if remove { return executeMessageReactRemove(...) }), and add explicit validation before attempting an add reaction that rejects empty or missing emoji (return an error) so malformed add-reaction calls are rejected instead of sent to executeMessageReactRemove; reference executeMessageReact and executeMessageReactRemove when making the change.bridges/ai/integration_host.go (1)
238-250:⚠️ Potential issue | 🟠 MajorCap
SessionTranscripthistory fetches.Line 242 counts the full portal history, and Line 250 immediately asks
getAIHistoryMessagesfor that full count. Long-lived chats can turn this into an unbounded in-memory load.Suggested fix
func (h *runtimeIntegrationHost) SessionTranscript(ctx context.Context, portalKey networkid.PortalKey) ([]integrationruntime.MessageSummary, error) { if h == nil || h.client == nil || h.client.UserLogin == nil || h.client.UserLogin.Bridge == nil || h.client.UserLogin.Bridge.DB == nil { return nil, nil } count, err := h.client.UserLogin.Bridge.DB.Message.CountMessagesInPortal(ctx, portalKey) if err != nil || count <= 0 { return nil, err } + const maxTranscriptMessages = 500 + if count > maxTranscriptMessages { + count = maxTranscriptMessages + } portal, err := h.client.UserLogin.Bridge.GetPortalByKey(h.client.backgroundContext(ctx), portalKey)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/integration_host.go` around lines 238 - 250, SessionTranscript currently requests the entire portal message count (CountMessagesInPortal) and passes it unbounded into getAIHistoryMessages which can blow up memory for long-lived chats; cap the fetch size by defining a sensible max (e.g. maxHistoryMessages) and clamp the count before calling getAIHistoryMessages (use min(count, maxHistoryMessages)), update SessionTranscript to pass the capped value to getAIHistoryMessages and consider logging or annotating when the returned history was truncated; reference functions/values: SessionTranscript, CountMessagesInPortal, getAIHistoryMessages.
🧹 Nitpick comments (8)
bridges/ai/image_generation_tool.go (5)
255-272: Redundant nil check onbtc.Client.UserLogin.Metadata.Line 256 checks
btc.Client.UserLogin.Metadata == nil, but after migrating toeffectiveLoginMetadata, this check is no longer meaningful sinceeffectiveLoginMetadatadoesn't directly useUserLogin.Metadata. The subsequent nil check onloginMeta(line 260) is sufficient.♻️ Suggested simplification
func supportsOpenAIImageGen(btc *BridgeToolContext) bool { - if btc == nil || btc.Client == nil || btc.Client.UserLogin == nil || btc.Client.UserLogin.Metadata == nil { + if btc == nil || btc.Client == nil || btc.Client.UserLogin == nil { return false } loginMeta := btc.Client.effectiveLoginMetadata(context.Background())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/image_generation_tool.go` around lines 255 - 272, The nil check for btc.Client.UserLogin.Metadata in supportsOpenAIImageGen is redundant because effectiveLoginMetadata() is the authoritative source; remove the btc.Client.UserLogin.Metadata == nil check from the initial guard and rely on the existing nil check for loginMeta returned by btc.Client.effectiveLoginMetadata(context.Background()), keeping the rest of the logic (including Provider switch, magic proxy credential checks via loginCredentialAPIKey/loginCredentialBaseURL, and resolveOpenAIAPIKey) unchanged.
623-660: Same redundantMetadatanil check pattern inresolveOpenRouterImageGenEndpoint.The
btc.Client.UserLogin.Metadata == nilcheck at line 624 can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/image_generation_tool.go` around lines 623 - 660, In resolveOpenRouterImageGenEndpoint, remove the redundant nil check for btc.Client.UserLogin.Metadata from the initial guard (the check that includes btc.Client.UserLogin.Metadata == nil) because you call btc.Client.effectiveLoginMetadata(...) later which already handles missing Metadata; keep the other nil checks (btc, btc.Client, btc.Client.UserLogin) intact and ensure the function still returns the same false/empty values when required.
280-295: Same redundantMetadatanil check pattern.Similar to
supportsOpenAIImageGen, thebtc.Client.UserLogin.Metadata == nilcheck at line 281 is no longer necessary after the migration toeffectiveLoginMetadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/image_generation_tool.go` around lines 280 - 295, Remove the redundant btc.Client.UserLogin.Metadata nil check in supportsGeminiImageGen: rely on effectiveLoginMetadata(context.Background()) for login metadata validation (as in supportsOpenAIImageGen). Update the initial guard to only check btc, btc.Client and btc.Client.UserLogin as needed, call btc.Client.effectiveLoginMetadata(...) and return false if it is nil, then keep the existing provider switch (ProviderMagicProxy -> false, default -> false); reference supportsGeminiImageGen, BridgeToolContext, btc.Client.UserLogin.Metadata, and effectiveLoginMetadata to locate and edit the code.
480-507: Same redundantMetadatanil check pattern inbuildOpenAIImagesBaseURL.The
btc.Client.UserLogin.Metadata == nilcheck at line 481 can be removed for consistency with the new pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/image_generation_tool.go` around lines 480 - 507, Remove the redundant btc.Client.UserLogin.Metadata == nil check from the initial guard in buildOpenAIImagesBaseURL; keep the nil checks for btc, btc.Client, and btc.Client.UserLogin only, and rely on loginMeta := btc.Client.effectiveLoginMetadata(...) to handle metadata presence and return the appropriate error if nil—update the initial conditional accordingly so the function compiles and behavior is consistent with effectiveLoginMetadata.
509-533: Same redundantMetadatanil check pattern inbuildGeminiBaseURL.The
btc.Client.UserLogin.Metadata == nilcheck at line 510 can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/image_generation_tool.go` around lines 509 - 533, Remove the redundant nil check for UserLogin.Metadata in buildGeminiBaseURL: keep the existing nil guards for btc, btc.Client, and btc.Client.UserLogin but delete the btc.Client.UserLogin.Metadata == nil condition since effectiveLoginMetadata (btc.Client.effectiveLoginMetadata) is already called and validated; ensure buildGeminiBaseURL still returns the same errors when loginMeta is nil and that Provider-specific logic (ProviderMagicProxy branch, connector.resolveServiceConfig, normalizeProxyBaseURL, joinProxyPath) remains unchanged.bridges/ai/logout_cleanup.go (1)
94-109:bestEffortExecis redundant and should be removed.The function at lines 107–109 is a direct pass-through to
execDelete. Update the 6 callers acrossdelete_chat.go,internal_prompt_db.go, andlogin_state_db.goto callexecDeletedirectly instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/logout_cleanup.go` around lines 94 - 109, Remove the redundant wrapper function bestEffortExec and replace its uses with direct calls to execDelete; specifically delete the bestEffortExec declaration and update the six call sites that currently call bestEffortExec (across delete_chat.go, internal_prompt_db.go, and login_state_db.go) to call execDelete(ctx, db, logger, query, args...) directly so argument expansion and behavior remain unchanged.bridges/ai/mcp_helpers.go (1)
82-101: Type switch should include a defensive default case for consistency.The type switch at lines 95–100 lacks a default case, unlike the similar switches in
loginCredentials()andensureLoginCredentials()(which both return nil or do nothing for unsupported types). While all current callers pass*aiLoginConfigfromloginConfigSnapshot(), adding a default case maintains defensive consistency and guards against future refactoring where unexpected types might be passed.♻️ Suggested defensive handling
if loginCredentialsEmpty(creds) { switch v := owner.(type) { case *UserLoginMetadata: v.Credentials = nil case *aiLoginConfig: v.Credentials = nil + default: + // Unsupported owner type; credentials not cleared } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/mcp_helpers.go` around lines 82 - 101, The type switch in clearLoginMCPServer does not include a default branch, making it inconsistent with loginCredentials() and ensureLoginCredentials(); update the switch in clearLoginMCPServer (the one switching on owner in function clearLoginMCPServer) to add a defensive default case that does nothing (no-op) so unsupported owner types are ignored safely; keep the existing cases for *UserLoginMetadata and *aiLoginConfig and ensure the default mirrors the harmless behavior used in loginCredentials()/ensureLoginCredentials().bridges/ai/media_understanding_runner.go (1)
926-926: Consider passing the actual context instead ofcontext.Background().Using
context.Background()here loses any tracing, logging context, or cancellation signals from the caller. IfeffectiveLoginMetadataperforms any I/O or logging that should respect the request context, consider passing the actualctxparameter (or the surrounding function's context) instead.This pattern appears at multiple call sites in this file (lines 926, 942, 1032, 1038, 1054).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/media_understanding_runner.go` at line 926, The call to oc.effectiveLoginMetadata currently uses context.Background() which discards caller tracing/cancellation; update calls like oc.connector.resolveServiceConfig(oc.effectiveLoginMetadata(context.Background())) to pass the upstream context (e.g., the local ctx variable) instead so effectiveLoginMetadata and resolveServiceConfig receive the real request context; apply the same change at all similar call sites in this file (references: effectiveLoginMetadata, resolveServiceConfig).
🤖 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/ai/custom_agents_db.go`:
- Around line 107-135: In saveCustomAgentForLogin ensure the trimmed agent.ID is
non-empty before using it: call strings.TrimSpace(agent.ID) into a local id
variable, and if id=="" return an error (or non-nil) so we don't write an
empty-key into loginMetadata (meta.CustomAgents) or insert a DB row; update the
branches that use strings.TrimSpace(agent.ID) (the fallback map insertion and
the scope.db.Exec call) to use that validated id; this prevents saving blank IDs
while keeping references to customAgentScopeForLogin, loginMetadata,
cloneAgentDefinitionContentMap, and aiCustomAgentsTable intact.
In `@bridges/ai/desktop_api_sessions.go`:
- Line 161: The call to effectiveLoginMetadata is using context.Background(),
dropping request cancellation/deadlines; update desktopAPIInstances to accept a
context.Context parameter (rename to desktopAPIInstances(ctx)) and thread that
ctx into the call that builds creds (use
loginCredentials(effectiveLoginMetadata(ctx))). Also update all internal calls
and call sites (e.g., places in sessions_tools.go and other callers) to pass the
incoming request context; this ensures the chain through loginConfigSnapshot →
ensureLoginConfigLoaded → loadAILoginConfig(ctx) receives the proper context and
cancels I/O when requests are cancelled.
In `@bridges/ai/gravatar.go`:
- Around line 38-43: The function ensureGravatarState dereferences
state.Gravatar without checking that the pointer state is non-nil; update
ensureGravatarState to first check if state == nil and either return a new
GravatarState (or allocate and return a new loginRuntimeState with Gravatar set)
or explicitly document/require non-nil callers — modify ensureGravatarState (and
any callers if you choose to allocate) to avoid panics by handling a nil
*loginRuntimeState before accessing state.Gravatar or state.Gravatar =
&GravatarState{}.
In `@bridges/ai/handleai.go`:
- Around line 129-156: The health-transition logic currently reads state via
loginStateSnapshot() then mutates via updateLoginState(), causing racey
decisions; fix by making the decision and mutation atomic inside
updateLoginState() for both recordProviderFailure (the failure path) and
recordProviderSuccess: inside the updateLoginState callback read the current
state.ConsecutiveErrors, compute nextErrors (or wasUnhealthy→nextHealthy) based
on the change, update state.ConsecutiveErrors and state.LastErrorAt there, and
set a boolean/enum captured by the outer scope indicating whether a
health-transition event should be emitted; after updateLoginState returns,
consult that captured result and call oc.UserLogin.BridgeState.Send(...) if and
only if the atomic transition crossed the healthWarningThreshold (use the same
threshold constant in both places). Ensure you reference and change the logic in
the functions that call loginStateSnapshot(), updateLoginState(), and
oc.UserLogin.BridgeState.Send (recordProviderFailure/recordProviderSuccess
flows) so all decisioning happens inside updateLoginState.
In `@bridges/ai/login_state_db.go`:
- Around line 64-75: The fallback conversion in loginRuntimeStateFromMetadata
currently omits the legacy NextChatIndex and LastHeartbeatEvent fields so chat
numbering and heartbeat seeding are lost; update loginRuntimeStateFromMetadata
to copy meta.NextChatIndex and meta.LastHeartbeatEvent into the returned
*loginRuntimeState (and likewise update the inverse mapper(s) that convert
runtime state back to UserLoginMetadata in the same file — any functions
handling mapping around loginRuntimeState/UserLoginMetadata between lines
~103-139) so both NextChatIndex and LastHeartbeatEvent are preserved across
migrations.
- Around line 249-265: The updateLoginState function currently lets fn mutate
oc.loginState in-place before persistence, so a failed save leaves the in-memory
cache out of sync; fix this by making a deep copy of oc.loginState (type
loginRuntimeState) inside updateLoginState, call fn on that copy, and only if fn
returns true attempt saveLoginRuntimeState with the copied state; on successful
save replace oc.loginState with the saved copy, otherwise leave oc.loginState
unchanged and return the error. Keep locking via oc.loginStateMu around the
load/copy/replace steps and use loadLoginRuntimeState when oc.loginState is nil
as currently done.
In `@bridges/ai/message_state_db.go`:
- Around line 127-136: The placeholder numbering uses the loop index i which
doesn't account for skipped blank IDs, causing gaps like $4,$6; fix the loop in
the code that builds args/placeholders (the block that iterates messageIDs and
populates args and placeholders) so the placeholder is derived from the actual
arg position rather than i—either keep an explicit counter starting at 4 and
increment only when you append a non-blank ID, or append the ID to args first
and then generate the placeholder with fmt.Sprintf("$%d", len(args)) so the
placeholder number always matches the argument index.
In `@bridges/ai/metadata.go`:
- Around line 95-114: The portal-state gap: add a metadata fallback and
documentation for portal reads—update loadAIPortalState to fallback to bridgev2
metadata (implement an aiPortalStateFromMetadata helper mirroring
aiLoginConfigFromMetadata/loginRuntimeStateFromMetadata) so aichats_portal_state
missing rows will populate Portal state from UserLoginMetadata, and ensure
CustomAgents persistence by retaining fallback in aichats_custom_agents reads to
UserLoginMetadata.CustomAgents; also add a brief comment near loadAILoginConfig,
loadLoginRuntimeState, loadAIPortalState explaining the runtime fallback chain
and call sites must load portal state before use.
In `@bridges/ai/status_text.go`:
- Line 203: The loop in lastAssistantUsage that iterates over history (populated
via oc.getAIHistoryMessages/GetLastNInPortal) is iterating backwards and thus
returns the oldest assistant message with tokens; change the loop to iterate
forward (for i := 0; i < len(history); i++) so it checks newest-first order as
returned by GetLastNInPortal and returns the most recent assistant message with
token usage; update any comments accordingly to reflect newest-first semantics.
In `@bridges/ai/tool_configured.go`:
- Line 57: The call to effectiveLoginMetadata currently uses
context.Background() (via meta =
oc.effectiveLoginMetadata(context.Background())), which detaches DB/metadata
lookups from the caller's cancellation/deadlines; update function signatures and
call-sites so the request context is threaded through: change
effectiveToolConfig to accept a ctx context.Context and use that when calling
effectiveLoginMetadata(ctx), and update effectiveSearchConfig and
effectiveFetchConfig to accept and forward the same ctx into
effectiveToolConfig; finally replace the context.Background() call with the
propagated ctx so all loginConfigSnapshot/database operations use the caller's
context.
---
Outside diff comments:
In `@bridges/ai/chat.go`:
- Around line 46-52: agentsEnabledForLogin currently returns false when
cfg.Agents is nil, causing a mismatch with shouldEnsureDefaultChat which treats
nil as enabled; update agentsEnabledForLogin (in AIClient) to treat a nil Agents
pointer as enabled by returning true when cfg.Agents == nil, otherwise return
the boolean value (*cfg.Agents) from the loginConfigSnapshot result so
default-config logins consistently enable agents.
- Around line 239-268: The response currently sets resp.UserID to
networkid.UserID(agent.ID) which doesn't match how agent ghosts are stored;
update agentContactResponse so the UserID returned for agent contacts uses the
bridge ghost ID via oc.agentUserID(agentID) (using the catalogAgentID result)
instead of networkid.UserID(agent.ID); if catalogAgentID(agent) is empty, keep
the existing fallback behavior, and ensure this change happens before calling
UserLogin.Bridge.GetGhostByID so the ghost hydrate uses the correct ID (refer to
functions agentContactResponse, catalogAgentID, oc.ResolveResponderForAgent, and
oc.agentUserID).
In `@bridges/ai/metadata.go`:
- Around line 313-325: cloneUserLoginMetadata performs a JSON round-trip which
strips non-serialized/transient fields (credentials, caches, profile data,
custom agents, error counters), so update cloneUserLoginMetadata to perform the
JSON marshal/unmarshal for the serializable fields and then explicitly copy the
transient fields from src into the resulting clone (e.g., copy Credentials,
Cache, Profile, CustomAgents, ErrorCounters or their pointer/deep copies as
appropriate) so callers that clone before mutating/persisting don't lose runtime
state; locate and modify cloneUserLoginMetadata to perform the manual transfer
after unmarshal and ensure pointer safety (deep-copy if needed) before returning
the clone.
In `@bridges/codex/constructors.go`:
- Around line 42-55: StartConnector calls aidb.EnsureSchema with cc.bridgeDB()
even when InitConnector left cc.db nil; wrap the EnsureSchema call in a
nil-check so it only runs when cc.bridgeDB() != nil (e.g. assign db :=
cc.bridgeDB(); if db == nil { return nil } else if err := aidb.EnsureSchema(ctx,
db); err != nil { return err }), ensuring StartConnector short-circuits cleanly
in no-DB scenarios and avoids passing nil to aidb.EnsureSchema.
---
Duplicate comments:
In `@bridges/ai/handleai.go`:
- Around line 387-390: Set meta.WelcomeSent = true and persist that change with
oc.savePortalQuiet using a cancellable context (bgCtx from
context.WithTimeout(oc.backgroundContext(ctx), 10*time.Second)) before any code
that emits the welcome notice; ensure you call oc.savePortalQuiet immediately
after setting meta.WelcomeSent and check/handle its error (log/stop the notice
emission) so the notice is only sent if the persistence succeeds.
- Around line 319-321: The AutoGreetingSent flag is being set in-memory but not
guaranteed persisted before sending the greeting; change the order and add error
handling so persistence happens first: set currentMeta.AutoGreetingSent = true,
call oc.savePortalQuiet(bgCtx, current, "auto greeting state") and check its
error (do not call oc.dispatchInternalMessage if save failed), logging or
returning the save error; only after a successful save call
oc.dispatchInternalMessage(..., "auto-greeting", true). This uses the existing
symbols currentMeta.AutoGreetingSent, oc.savePortalQuiet, and
oc.dispatchInternalMessage.
- Around line 456-463: The portal title is only persisted locally
(portal.Name/portal.NameSet and oc.savePortalQuiet) but not pushed to Matrix;
after oc.savePortalQuiet(bgCtx, portal, "room title") call the existing
lifecycle/materialization sync that pushes portal metadata to Matrix (i.e., the
oc method responsible for materializing/syncing portals) so the updated
portal.Title/portal.Name is sent to clients; if no such helper exists, add and
call a method like oc.MaterializePortal(ctx, portal) or
oc.SyncPortalTitleToMatrix(ctx, portal) immediately after saving to trigger the
external update.
In `@bridges/ai/integration_host.go`:
- Around line 238-250: SessionTranscript currently requests the entire portal
message count (CountMessagesInPortal) and passes it unbounded into
getAIHistoryMessages which can blow up memory for long-lived chats; cap the
fetch size by defining a sensible max (e.g. maxHistoryMessages) and clamp the
count before calling getAIHistoryMessages (use min(count, maxHistoryMessages)),
update SessionTranscript to pass the capped value to getAIHistoryMessages and
consider logging or annotating when the returned history was truncated;
reference functions/values: SessionTranscript, CountMessagesInPortal,
getAIHistoryMessages.
In `@bridges/ai/tools.go`:
- Around line 390-397: The routing in executeMessageReact incorrectly treats an
empty emoji as a removal request; change the conditional so only remove == true
forwards to executeMessageReactRemove (i.e., if remove { return
executeMessageReactRemove(...) }), and add explicit validation before attempting
an add reaction that rejects empty or missing emoji (return an error) so
malformed add-reaction calls are rejected instead of sent to
executeMessageReactRemove; reference executeMessageReact and
executeMessageReactRemove when making the change.
---
Nitpick comments:
In `@bridges/ai/image_generation_tool.go`:
- Around line 255-272: The nil check for btc.Client.UserLogin.Metadata in
supportsOpenAIImageGen is redundant because effectiveLoginMetadata() is the
authoritative source; remove the btc.Client.UserLogin.Metadata == nil check from
the initial guard and rely on the existing nil check for loginMeta returned by
btc.Client.effectiveLoginMetadata(context.Background()), keeping the rest of the
logic (including Provider switch, magic proxy credential checks via
loginCredentialAPIKey/loginCredentialBaseURL, and resolveOpenAIAPIKey)
unchanged.
- Around line 623-660: In resolveOpenRouterImageGenEndpoint, remove the
redundant nil check for btc.Client.UserLogin.Metadata from the initial guard
(the check that includes btc.Client.UserLogin.Metadata == nil) because you call
btc.Client.effectiveLoginMetadata(...) later which already handles missing
Metadata; keep the other nil checks (btc, btc.Client, btc.Client.UserLogin)
intact and ensure the function still returns the same false/empty values when
required.
- Around line 280-295: Remove the redundant btc.Client.UserLogin.Metadata nil
check in supportsGeminiImageGen: rely on
effectiveLoginMetadata(context.Background()) for login metadata validation (as
in supportsOpenAIImageGen). Update the initial guard to only check btc,
btc.Client and btc.Client.UserLogin as needed, call
btc.Client.effectiveLoginMetadata(...) and return false if it is nil, then keep
the existing provider switch (ProviderMagicProxy -> false, default -> false);
reference supportsGeminiImageGen, BridgeToolContext,
btc.Client.UserLogin.Metadata, and effectiveLoginMetadata to locate and edit the
code.
- Around line 480-507: Remove the redundant btc.Client.UserLogin.Metadata == nil
check from the initial guard in buildOpenAIImagesBaseURL; keep the nil checks
for btc, btc.Client, and btc.Client.UserLogin only, and rely on loginMeta :=
btc.Client.effectiveLoginMetadata(...) to handle metadata presence and return
the appropriate error if nil—update the initial conditional accordingly so the
function compiles and behavior is consistent with effectiveLoginMetadata.
- Around line 509-533: Remove the redundant nil check for UserLogin.Metadata in
buildGeminiBaseURL: keep the existing nil guards for btc, btc.Client, and
btc.Client.UserLogin but delete the btc.Client.UserLogin.Metadata == nil
condition since effectiveLoginMetadata (btc.Client.effectiveLoginMetadata) is
already called and validated; ensure buildGeminiBaseURL still returns the same
errors when loginMeta is nil and that Provider-specific logic
(ProviderMagicProxy branch, connector.resolveServiceConfig,
normalizeProxyBaseURL, joinProxyPath) remains unchanged.
In `@bridges/ai/logout_cleanup.go`:
- Around line 94-109: Remove the redundant wrapper function bestEffortExec and
replace its uses with direct calls to execDelete; specifically delete the
bestEffortExec declaration and update the six call sites that currently call
bestEffortExec (across delete_chat.go, internal_prompt_db.go, and
login_state_db.go) to call execDelete(ctx, db, logger, query, args...) directly
so argument expansion and behavior remain unchanged.
In `@bridges/ai/mcp_helpers.go`:
- Around line 82-101: The type switch in clearLoginMCPServer does not include a
default branch, making it inconsistent with loginCredentials() and
ensureLoginCredentials(); update the switch in clearLoginMCPServer (the one
switching on owner in function clearLoginMCPServer) to add a defensive default
case that does nothing (no-op) so unsupported owner types are ignored safely;
keep the existing cases for *UserLoginMetadata and *aiLoginConfig and ensure the
default mirrors the harmless behavior used in
loginCredentials()/ensureLoginCredentials().
In `@bridges/ai/media_understanding_runner.go`:
- Line 926: The call to oc.effectiveLoginMetadata currently uses
context.Background() which discards caller tracing/cancellation; update calls
like
oc.connector.resolveServiceConfig(oc.effectiveLoginMetadata(context.Background()))
to pass the upstream context (e.g., the local ctx variable) instead so
effectiveLoginMetadata and resolveServiceConfig receive the real request
context; apply the same change at all similar call sites in this file
(references: effectiveLoginMetadata, resolveServiceConfig).
🪄 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: cd0b8b37-f583-4ab0-a29a-c4740330be73
📒 Files selected for processing (70)
.github/workflows/docker-agentremote.yml.github/workflows/go.yml.github/workflows/publish-release.ymlREADME.mdbridges/ai/agentstore.gobridges/ai/bridge_db.gobridges/ai/bridge_info.gobridges/ai/bridge_info_test.gobridges/ai/broken_login_client.gobridges/ai/chat.gobridges/ai/client.gobridges/ai/commands.gobridges/ai/constructors.gobridges/ai/custom_agents_db.gobridges/ai/delete_chat.gobridges/ai/desktop_api_sessions.gobridges/ai/gravatar.gobridges/ai/handleai.gobridges/ai/handlematrix.gobridges/ai/heartbeat_events.gobridges/ai/heartbeat_session.gobridges/ai/image_generation_tool.gobridges/ai/image_understanding.gobridges/ai/integration_host.gobridges/ai/integrations_config.gobridges/ai/login.gobridges/ai/login_config_db.gobridges/ai/login_loaders.gobridges/ai/login_state_db.gobridges/ai/logout_cleanup.gobridges/ai/mcp_client.gobridges/ai/mcp_helpers.gobridges/ai/mcp_servers.gobridges/ai/media_understanding_cli.gobridges/ai/media_understanding_runner.gobridges/ai/message_state_db.gobridges/ai/metadata.gobridges/ai/model_catalog.gobridges/ai/prompt_builder.gobridges/ai/provisioning.gobridges/ai/response_finalization.gobridges/ai/scheduler_heartbeat_test.gobridges/ai/session_store.gobridges/ai/sessions_tools.gobridges/ai/status_text.gobridges/ai/subagent_announce.gobridges/ai/timezone.gobridges/ai/token_resolver.gobridges/ai/tool_approvals_policy.gobridges/ai/tool_configured.gobridges/ai/tool_policy_chain.gobridges/ai/tool_policy_chain_test.gobridges/ai/tools.gobridges/ai/tools_beeper_feedback.gobridges/codex/config.gobridges/codex/connector.gobridges/codex/connector_test.gobridges/codex/constructors.gobridges/openclaw/README.mdbridges/openclaw/gateway_client.gobridges/openclaw/gateway_client_test.gocmd/agentremote/commands.godocker/agentremote/README.mddocs/bridge-orchestrator.mdpkg/agents/prompt.gopkg/aidb/001-init.sqlpkg/aidb/db.gopkg/aidb/db_test.gosdk/room_features.gotools/generate-homebrew-cask.sh
✅ Files skipped from review due to trivial changes (18)
- docs/bridge-orchestrator.md
- bridges/ai/media_understanding_cli.go
- .github/workflows/go.yml
- pkg/agents/prompt.go
- .github/workflows/docker-agentremote.yml
- bridges/ai/tool_policy_chain_test.go
- docker/agentremote/README.md
- bridges/ai/tools_beeper_feedback.go
- bridges/ai/integrations_config.go
- bridges/ai/mcp_client.go
- bridges/ai/tool_policy_chain.go
- tools/generate-homebrew-cask.sh
- README.md
- bridges/openclaw/gateway_client.go
- bridges/codex/config.go
- bridges/openclaw/README.md
- bridges/ai/provisioning.go
- .github/workflows/publish-release.yml
🚧 Files skipped from review as they are similar to previous changes (10)
- bridges/ai/broken_login_client.go
- bridges/ai/bridge_info.go
- bridges/ai/commands.go
- pkg/aidb/db_test.go
- bridges/ai/handlematrix.go
- bridges/ai/login_loaders.go
- bridges/codex/connector_test.go
- bridges/ai/login_config_db.go
- bridges/ai/session_store.go
- bridges/ai/constructors.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-cli-docker (amd64)
- GitHub Check: build-docker
- GitHub Check: Lint
- GitHub Check: build-docker
- GitHub Check: Lint
🧰 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/ai/tool_configured.gobridges/ai/bridge_info_test.gobridges/ai/media_understanding_runner.gobridges/ai/token_resolver.gobridges/ai/bridge_db.gobridges/ai/login.gobridges/codex/constructors.gobridges/codex/connector.gobridges/ai/handleai.gobridges/ai/chat.gobridges/ai/client.go
🔇 Additional comments (46)
bridges/openclaw/gateway_client_test.go (1)
83-84: Nice alignment with production user-agent composition.Using
openClawGatewayUserAgentBase + resolveGatewayClientVersion()makes this test less brittle than a hardcoded literal and keeps it consistent with the gateway client implementation.sdk/room_features.go (1)
74-74: Namespace fallback update looks correct.Switching the default capability ID to the
agentremoteSDK namespace is consistent with the refactor and keeps the fallback aligned with current package identity.bridges/ai/bridge_info_test.go (3)
38-52: Rename and call-site migration are consistent.
TestApplyAIChatsBridgeInfoand the updatedapplyAIChatsBridgeInfo(...)invocation are aligned, and the visible-DM expectations still validate the intended behavior.
58-71: Beeper-room protocol assertion now matches new contract.Switching this assertion to
aiBridgeProtocolIDcorrectly reflects the fixed AI protocol behavior for beeper-addressed rooms.
88-88: Background-room path was correctly migrated too.The
applyAIChatsBridgeInfo(...)call update in the internal/background test keeps normalization coverage intact.cmd/agentremote/commands.go (6)
302-303: Branding update fordoctorcommand looks consistent.Nice cleanup on the user-facing wording here; it aligns with the new AgentRemote Manager naming.
347-358: Centralized command-spec normalization is a solid refactor.Applying the
binaryNamerewrite in one place is clean and helps keep help text/completions in sync.
476-477: Usage banner changes are clear and user-friendly.Good move using branded header text while keeping invocation dynamic via
binaryName.
503-577: Bash completion parameterization is implemented cleanly.All key wiring points now respect
binaryName, which keeps completion generation aligned with renamed binaries.
585-635: Zsh completion updates are consistent and complete.Nice job propagating
binaryNamethrough compdef/function naming and top-level command descriptions.
676-736: Fish completion migration tobinaryNameis thorough.The command target replacement is complete across top-level, positional, and flag completion entries.
bridges/codex/connector.go (1)
53-76: Runtime defaulting migration looks consistent.The switch to the SDK helpers keeps the defaults centralized, and the new client-info fallbacks avoid hardcoded duplicates in this path.
bridges/ai/timezone.go (1)
44-46: Snapshot-based timezone resolution looks correct.The new config-snapshot lookup keeps the existing normalize/fallback behavior intact and safely falls back to env/default when needed.
bridges/ai/bridge_db.go (3)
11-21: AI table-name constants centralization is a good cleanup.Consolidating these names reduces hard-coded drift across DB call sites.
29-29:db_section=ailogger scoping change looks good.This aligns child-DB logs with the new AI schema ownership.
77-82:bridgeDBFromPortalnil guards are solid.The helper is defensive and consistent with the existing login-based DB resolver flow.
bridges/ai/heartbeat_session.go (1)
41-42: Including bridge/login insessionStoreRefis the right direction.This improves cross-login/session isolation compared to agent-only scoping.
bridges/ai/mcp_servers.go (1)
148-148: Snapshot-based MCP token sourcing looks correct.This is consistent with the ongoing move away from direct login metadata reads.
bridges/ai/model_catalog.go (1)
220-220: Using effective login metadata here is a good fix.The call site now matches the newer metadata resolution flow while keeping the existing guard path.
bridges/ai/scheduler_heartbeat_test.go (1)
123-124:aidb.EnsureSchemamigration in test setup looks good.This keeps test initialization aligned with the current AI schema bootstrap API.
bridges/ai/image_understanding.go (1)
81-82: No nil-contract issue;loginStateSnapshotis guaranteed non-nil.The implementation always returns a non-nil
*loginRuntimeState:
ensureLoginStateLoadedhandles load errors by allocating&loginRuntimeState{}(line 239) and never returns nilcloneLoginRuntimeStatechecks for nil input and returns an allocated struct in all cases (lines 50–51)- Therefore,
loginState.ModelCacheat line 85 is accessed on a guaranteed non-nil pointer without panic risk.bridges/ai/image_generation_tool.go (1)
182-186: LGTM!The migration from
loginMetadata(btc.Client.UserLogin)tobtc.Client.effectiveLoginMetadata(context.Background())is consistent with the broader refactor. The nil check onloginMetais preserved.bridges/ai/mcp_helpers.go (1)
68-80: LGTM!The refactor to accept
owner anywithensureLoginCredentials(owner)provides flexibility for both*UserLoginMetadataand*aiLoginConfigtypes.bridges/ai/gravatar.go (1)
184-189: LGTM!The migration to
loginStateSnapshotis correct with proper nil checks for bothloginStateand nestedGravatar/Primaryfields.bridges/ai/sessions_tools.go (3)
118-127: LGTM!The migration to
getAIHistoryMessagesmaintains the existing error handling (only using messages on success) and properly limits the result set.
270-286: LGTM!Consistent migration to
getAIHistoryMessageswith proper error handling that returns an error result on failure.
572-581: LGTM!The migration in
lastMessageTimestampcorrectly handles errors and empty results by returning 0.bridges/ai/tool_approvals_policy.go (1)
17-28: LGTM!The narrowed approval bypass list correctly aligns with the removal of corresponding message tool actions (
reactions,read,member-info,channel-info,list-pins). The remaining exemptions are appropriately limited to read-only operations.bridges/ai/subagent_announce.go (2)
45-74: LGTM!The migration to
getAIHistoryMessagesinreadLatestAssistantReplypreserves the error handling and message filtering logic.
105-138: LGTM!The migration in
buildSubagentStatsLineappropriately ignores errors (using_) since stats are optional and the function already handles empty message slices gracefully.bridges/ai/token_resolver.go (1)
185-225: LGTM!The refactor cleanly separates metadata-based resolution from config-based resolution. The extracted
resolveProviderAPIKeyForConfigmaintains the same provider-specific logic while enabling config-driven API key resolution.bridges/ai/logout_cleanup.go (1)
12-91: LGTM on expanded cleanup coverage.The comprehensive cleanup now covers all AI-specific tables, and the in-memory state reset ensures the client is properly cleared on logout.
bridges/ai/prompt_builder.go (1)
164-172: Internal prompt rows can evict chat history from replay.The
hr.limitis applied after merging internal prompt rows intocandidates. This means a burst of internal prompt rows could push out actual user/assistant messages entirely.Additionally, the image-injection logic at line 201 (
i < maxHistoryImageMessages) now counts internal rows too, potentially corrupting the vision injection window.Consider:
- Tracking internal rows separately and not counting them toward
hr.limit- Using a counter of non-internal entries for image injection rather than the loop index
bridges/ai/agentstore.go (2)
547-547:CreateRoomreturns success even when portal persistence fails silently.
savePortalQuietlogs errors but does not return them. The caller receives a successful room creation response even if metadata persistence failed. This could lead to inconsistent state where the Matrix room exists but portal metadata is missing.Consider using a persistence function that returns errors, or accepting this as a trade-off for user experience (room works but some metadata may be missing).
36-66: LGTM - Clean migration to login-scoped table helpers.The
LoadAgentsfunction properly:
- Accepts context parameter for DB operations
- Loads custom agents via
listCustomAgentsForLogin- Gates presets based on provider
- Handles nil checks appropriately
bridges/ai/delete_chat.go (1)
62-93: LGTM - Comprehensive cleanup of persisted session artifacts.The delete function now properly cleans up multiple tables:
- AI sessions
- System events
- Portal state
- Message state
This aligns with the migration to DB-backed tables for portal/session state.
Regarding the past review concern about login-level portal references: given that this PR migrates away from in-memory
UserLoginmetadata to DB-backed tables, stale references in login metadata should no longer be a concern as long as the new tables (aiPortalStateTable,aichats_message_state) are the source of truth.bridges/ai/media_understanding_runner.go (1)
601-601: Temp directory prefix updated to match new branding.The change from
agentremote-media-*toaichats-media-*aligns with the broader rename in this PR.bridges/ai/response_finalization.go (2)
32-47: LGTM - SDK migration for continuation messages.The function correctly migrates from
agentremote.EventTimingtosdk.EventTimingand usessdk.BuildContinuationMessage. The timing parameters are properly passed through.
135-137: LGTM - Proper session store keying with bridge and login IDs.The heartbeat session store reference now correctly derives
BridgeIDandLoginIDfromloginDBContext(oc), aligning with the broader session-store changes. This ensures proper scoping of heartbeat state per login.bridges/ai/heartbeat_events.go (2)
61-66: LGTM - Robust composite key for heartbeat login keying.The new
heartbeatLoginKeyfunction properly creates a composite key fromBridgeID|LoginIDwith appropriate nil checks. This is more robust than usingUserLoginIDalone.
117-126: LGTM - Proper state update with deduplication.The persistence logic correctly:
- Compares the new event against the previous state
- Only persists if there's a meaningful change
- Returns
falseto skip unnecessary writes when events are identicalbridges/ai/login.go (3)
33-36: LGTM - SDK error helper migration.The login error definitions properly migrate to
sdk.NewLoginRespErrorwith appropriate status codes and error codes.
213-257: LGTM - Robust login config handling with proper persistence.The login flow now:
- Loads existing config for relogin scenarios (line 219)
- Handles credentials via
aiLoginConfig(lines 239-242)- Validates using the new
loginMetadataViewpattern (line 243)- Properly persists config with error handling (lines 255-257)
Good addition of the
SAVE_LOGIN_FAILEDerror case.
322-337: The implementation correctly handles credential resolution for new logins.The
loginMetadataViewfunction properly reads credentials fromcfg.Credentialsand clones them into the temporary metadata object, which is then used in the API key resolution. The null-safety check forcfgensures this works correctly even when the configuration is not yet fully initialized.pkg/aidb/db.go (1)
27-39: LGTM — Simplified schema initialization for unreleased bridge.The
EnsureSchemafunction is appropriate for a bridge that hasn't been released yet. The comment clearly documents the rationale, and the schema file correctly uses idempotent statements (CREATE TABLE IF NOT EXISTS,CREATE INDEX IF NOT EXISTS) that allow safe re-application on every startup.Once this bridge is released, implement proper migration logic if schema changes are required.
pkg/aidb/001-init.sql (1)
1-2: Plan migration strategy before first release.The bridge has not been released yet (as documented in the code), so the concern about existing installs breaking is not currently applicable. However,
EnsureSchemaonly applies001-init.sqlwithout any incremental migration support. Once the first release ships, any subsequent schema changes will have no path to upgrade existing databases. Add a migration or version-tracking mechanism now rather than waiting for the first schema update request.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
bridges/ai/session_store.go (1)
76-109:⚠️ Potential issue | 🟠 MajorUse the IDs captured in
sessionStoreReffor the SQL path too.
sessionStoreRefnow carriesBridgeID/LoginID, and the lock key uses them, but the actualSELECT/INSERTstill re-read scope fromoc. If the client context changes afterresolveSessionStoreRef, you can lock one login's session and read/write another login's row. The DB predicates should useref.BridgeIDandref.LoginIDso the ref fully defines the storage target.Suggested direction
func (oc *AIClient) getSessionEntry(ctx context.Context, ref sessionStoreRef, sessionKey string) (sessionEntry, bool) { if oc == nil || strings.TrimSpace(sessionKey) == "" { return sessionEntry{}, false } scope := oc.sessionDBScope() - if scope == nil { + if scope == nil || strings.TrimSpace(ref.BridgeID) == "" || strings.TrimSpace(ref.LoginID) == "" { return sessionEntry{}, false } ... err := scope.db.QueryRow(ctx, ` SELECT ... FROM `+aiSessionsTable+` WHERE bridge_id=$1 AND login_id=$2 AND store_agent_id=$3 AND session_key=$4 `, - scope.bridgeID, scope.loginID, normalizeAgentID(ref.AgentID), strings.TrimSpace(sessionKey), + ref.BridgeID, ref.LoginID, normalizeAgentID(ref.AgentID), strings.TrimSpace(sessionKey), ).Scan(...) } func (oc *AIClient) upsertSessionEntry(ctx context.Context, ref sessionStoreRef, sessionKey string, entry sessionEntry) error { scope := oc.sessionDBScope() - if scope == nil { + if scope == nil || strings.TrimSpace(ref.BridgeID) == "" || strings.TrimSpace(ref.LoginID) == "" { return nil } ... _, err := scope.db.Exec(ctx, ` INSERT INTO `+aiSessionsTable+` ( ... ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16) ... `, - scope.bridgeID, - scope.loginID, + ref.BridgeID, + ref.LoginID, normalizeAgentID(ref.AgentID), strings.TrimSpace(sessionKey), ... ) return err }Also applies to: 136-179, 263-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/session_store.go` around lines 76 - 109, The SQL queries in getSessionEntry (and the other affected functions around the 136-179 and 263-277 ranges) still use scope.bridgeID/scope.loginID but must use the IDs carried in sessionStoreRef so the ref fully defines the target; update the SELECT/INSERT/UPDATE/DELETE parameter lists and WHERE clauses to use ref.BridgeID and ref.LoginID (e.g., replace scope.bridgeID, scope.loginID with ref.BridgeID, ref.LoginID) while keeping normalizeAgentID(ref.AgentID) for agent ID and preserving sessionKey handling; do the same substitution wherever those other functions (the counterparts to getSessionEntry) currently rely on scope instead of the ref.bridges/ai/chat.go (1)
46-52:⚠️ Potential issue | 🟠 MajorKeep the default agent-enable behavior consistent.
When
cfg.Agentsis unset, this returnsfalse, butshouldEnsureDefaultChat()still treats the same unset value as enabled. That means a fresh login can bootstrap a default Beeper chat while agent search/resolve/create paths reject agent usage. Pick one default and use it consistently on both sides.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/chat.go` around lines 46 - 52, agentsEnabledForLogin currently treats a nil cfg.Agents as false while shouldEnsureDefaultChat treats nil as enabled; make the default behavior consistent by changing agentsEnabledForLogin to treat a nil cfg.Agents as enabled (i.e., return true when cfg.Agents == nil) so it returns true by default and otherwise returns *cfg.Agents; update the implementation in the agentsEnabledForLogin function (which calls loginConfigSnapshot) to reflect this consistent default with shouldEnsureDefaultChat.bridges/ai/handlematrix.go (1)
364-383:⚠️ Potential issue | 🟠 MajorUse the repaired transcript role for edit handling.
This block loads and normalizes
transcriptMeta, but the canonical-turn update and the regenerate gate still key offmsgMeta.Role. Ifedit.EditTarget.Metadatais missing/stale while the transcript row still says"user", the edit is persisted and regeneration is skipped incorrectly. Derive an effective role fromtranscriptMetafirst, then fall back tomsgMeta.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handlematrix.go` around lines 364 - 383, The code currently uses msgMeta.Role to decide canonical-turn updates and whether to regenerate, but transcriptMeta may contain the authoritative role; change the logic to derive an effectiveRole from transcriptMeta (e.g., use transcriptMeta.Role when present, otherwise fall back to msgMeta.Role) and use that effectiveRole in both the canonical-turn update block (where setCanonicalTurnDataFromPromptMessages and cloneCanonicalTurnData are called) and the regenerate gate at the end (the if that currently checks msgMeta.Role != "user"); ensure you still persist via persistAITranscriptMessage and call oc.notifySessionMutation as before.
♻️ Duplicate comments (9)
bridges/ai/tool_configured.go (1)
16-18:⚠️ Potential issue | 🟠 MajorPass the caller context into
loginConfigSnapshot.Line 59 swaps the request context for
context.Background(), so the DB-backed config snapshot can outlive caller cancellation and deadlines. Thread the incomingctxthrougheffectiveSearchConfig,effectiveFetchConfig, andeffectiveToolConfiginstead.Suggested fix
-func (oc *AIClient) effectiveSearchConfig(_ context.Context) *search.Config { +func (oc *AIClient) effectiveSearchConfig(ctx context.Context) *search.Config { return effectiveToolConfig( + ctx, oc, func(connector *OpenAIConnector) *search.Config { if connector == nil || connector.Config.Tools.Web == nil { return nil @@ -func (oc *AIClient) effectiveFetchConfig(_ context.Context) *fetch.Config { +func (oc *AIClient) effectiveFetchConfig(ctx context.Context) *fetch.Config { return effectiveToolConfig( + ctx, oc, func(connector *OpenAIConnector) *fetch.Config { if connector == nil || connector.Config.Tools.Web == nil { return nil @@ func effectiveToolConfig[T any]( + ctx context.Context, oc *AIClient, load func(*OpenAIConnector) *T, applyTokens func(*T, string, *aiLoginConfig, *OpenAIConnector) *T, withDefaults func(*T) *T, ) *T { @@ cfg = load(connector) if oc.UserLogin != nil { provider = loginMetadata(oc.UserLogin).Provider - loginCfg = oc.loginConfigSnapshot(context.Background()) + loginCfg = oc.loginConfigSnapshot(ctx) } }Also applies to: 30-32, 44-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/tool_configured.go` around lines 16 - 18, The code is using context.Background() when calling loginConfigSnapshot which allows DB-backed config snapshots to outlive caller cancellation; thread the incoming ctx through by changing effectiveSearchConfig and effectiveFetchConfig to accept a context.Context and pass that ctx into effectiveToolConfig, and update effectiveToolConfig to pass the received ctx into loginConfigSnapshot (use the same ctx instead of context.Background()); update all call sites to forward the original caller ctx to these functions (refer to effectiveSearchConfig, effectiveFetchConfig, effectiveToolConfig, and loginConfigSnapshot).bridges/ai/tools.go (1)
390-397:⚠️ Potential issue | 🟡 MinorDon't treat a missing emoji as an implicit remove.
executeMessageReactRemovestill expects an explicit emoji, butemoji == ""is routed there even whenremoveis false. That turns an invalid add-reaction request into the remove path and gives the wrong validation flow.🐛 Proposed fix
-// Supports adding reactions (with emoji) and removing reactions (with remove:true or explicit emoji). +// Supports adding reactions with an emoji and removing reactions with remove:true. func executeMessageReact(ctx context.Context, args map[string]any, btc *BridgeToolContext) (string, error) { emoji, _ := args["emoji"].(string) remove, _ := args["remove"].(bool) // Check if this is a removal request. - if remove || emoji == "" { + if remove { return executeMessageReactRemove(ctx, args, btc) } + if strings.TrimSpace(emoji) == "" { + return "", errors.New("action=react requires 'emoji' unless remove=true") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/tools.go` around lines 390 - 397, The current branching in executeMessageReact treats emoji == "" as an implicit remove and calls executeMessageReactRemove even when remove is false; change the logic in executeMessageReact so only remove triggers the removal path: call executeMessageReactRemove(ctx, args, btc) only when remove is true, and if remove is false but emoji is empty return a validation error indicating an emoji is required for adding a reaction (or otherwise handle the missing-emoji case explicitly) so the add-reaction flow validates correctly; update checks around the emoji and remove variables and any callers of executeMessageReact accordingly.bridges/ai/prompt_builder.go (1)
152-171:⚠️ Potential issue | 🟠 MajorKeep internal prompt rows outside the history cap.
hr.limitis still applied afterinternalRowsare merged intocandidates, so a burst of internal prompt history can push out the newest user/assistant turns entirely. It also shrinks the vision replay window because the lateri < maxHistoryImageMessagescheck now counts internal rows too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/prompt_builder.go` around lines 152 - 171, The current code applies hr.limit and the image-message cap after merging internalRows into candidates so internal rows can evict recent user/assistant turns; change the flow to keep internalRows out of the cap by treating them separately: build/collect the regular candidates (the ones not from loadInternalPromptHistory), apply slices.SortStableFunc + the hr.limit truncation and the i < maxHistoryImageMessages logic only to that non-internal list, and only after truncation append the internalRows converted to replayCandidate (from loadInternalPromptHistory) into the final candidates slice so they are not counted against hr.limit or the image-message window; refer to loadInternalPromptHistory, internalRows, candidates, replayCandidate, hr.limit and the i < maxHistoryImageMessages check when making this change.bridges/ai/handlematrix.go (1)
942-959:⚠️ Potential issue | 🟠 MajorReturn persistence errors from
savePortalQuiet.This helper logs and continues on
saveAIPortalState/portal.Savefailures, so callers cannot stop one-shot flows after the state write failed. That leaves flags likeWelcomeSent,AutoGreetingSent, or generated title state vulnerable to duplicate side effects after retries/restarts. Make this return an error and let callers bail when persistence fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handlematrix.go` around lines 942 - 959, savePortalQuiet currently swallows persistence errors from saveAIPortalState and portal.Save so callers can't abort flows on failed writes; change savePortalQuiet (and its callers) to return an error instead of void, propagate any non-context.Canceled error from saveAIPortalState or portal.Save up to the caller, and update all call sites to handle the returned error (bail out / abort the one-shot flow) so flags like WelcomeSent / AutoGreetingSent / title-generation are not left in an inconsistent state; reference the function savePortalQuiet, the helper saveAIPortalState, and the portal.Save call when making the signature and call-site changes.bridges/ai/handleai.go (1)
454-461:⚠️ Potential issue | 🔴 CriticalPersisting
portal.Namealone won't update the Matrix room title.
savePortalQuietonly saves local portal state, so the generated title won't reach the Matrix room UI unless you follow this with the normal room-sync path (materializePortalRoom(...)orEnsurePortalLifecycle(...)). As written, the DB will say the room is titled, but the room name shown to users can stay unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handleai.go` around lines 454 - 461, The code only updates local state (meta, portal.Name, portal.NameSet) and calls oc.savePortalQuiet(...) which does not push changes to Matrix; after saving, invoke the room-sync path (e.g. call materializePortalRoom(bgCtx, portal) or EnsurePortalLifecycle(bgCtx, portal) as used elsewhere) so the generated title is propagated to the Matrix room UI; keep the existing oc.savePortalQuiet(...) call but add the appropriate materialize/EnsurePortalLifecycle call using the same bgCtx and portal objects.bridges/ai/login_state_db.go (2)
213-229:⚠️ Potential issue | 🟠 MajorDon't mutate the cached state before the write succeeds.
fncurrently mutatesoc.loginStatein place, thensaveLoginRuntimeStateruns afterward. If the save fails, memory keeps the new values even though the DB does not, so later reads observe state that was never persisted. Mutate a cloned copy, persist that copy, and only swapoc.loginStateon success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/login_state_db.go` around lines 213 - 229, The updateLoginState function currently lets fn mutate oc.loginState in-place before persisting; change it to clone the cached state first (e.g., create a new loginRuntimeState copy of oc.loginState), call fn on that cloned copy, and if fn returns true then call saveLoginRuntimeState(ctx, oc, clonedState); only after save succeeds swap oc.loginState = clonedState while holding oc.loginStateMu. Keep the existing loadLoginRuntimeState path and locking (oc.loginStateMu) but ensure all mutations and the save operate on the clone so the in-memory cache is only updated on successful persistence.
70-103:⚠️ Potential issue | 🟠 MajorKeep a fallback path for pre-migration login state.
On
sql.ErrNoRowsthis now returns a brand-new state immediately, so upgraded logins lose legacy runtime fields until something repopulatesaichats_login_state. That will reset things like chat numbering / heartbeat seeding for existing users during rollout. Preserve the legacy metadata fallback and backfill the new row on first load instead of treating “missing row” as “empty state.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/login_state_db.go` around lines 70 - 103, When QueryRow against aiLoginStateTable in loadLoginRuntimeState returns sql.ErrNoRows, don't return a brand-new state immediately; instead attempt to load legacy runtime metadata (from the pre-migration aichats_login_state or via a helper like loadLegacyLoginState) using scope.bridgeID and scope.loginID, populate the loginRuntimeState fields (NextChatIndex, LastHeartbeatEvent, model/file/gravatar caches, ConsecutiveErrors, LastErrorAt) from that legacy data if present, and then upsert/backfill a row into aiLoginStateTable so future loads hit the migrated schema; handle and return any unexpected DB errors as before. Ensure this logic is implemented inside loadLoginRuntimeState and references aiLoginStateTable, scope.bridgeID, scope.loginID, and the new-row upsert path.bridges/openclaw/identifiers.go (1)
31-36:⚠️ Potential issue | 🟠 Major
PathEscapestill leaves this scoped ID format ambiguous.
openClawScopedGhostUserIDstill buildsopenclaw-agent:<escaped-login>:<escaped-agent>, butparseOpenClawGhostIDstill splits on:. AloginIDlikeuser:namesurvivesurl.PathEscapeand will be parsed as login=user, agent=name:.... Please switch to an encoding that cannot emit:(for example base64url or hex) and version the format before rollout.Also applies to: 47-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/identifiers.go` around lines 31 - 36, Change the scoped-ID encoding from url.PathEscape to a non-colon-producing binary-to-text scheme and add a version token: in openClawScopedGhostUserID encode loginID and agentID with base64 URL-safe (no padding) or hex, prefix the ID with a version (e.g. "openclaw-agent:v1:") and join with ":"; update parseOpenClawGhostID to recognize the "v1" form and decode the base64url/hex segments back to original values, and keep backward-compatible handling for the old PathEscape format during rollout; also apply the same change to the other factory/parser pair referenced around the 47-70 block so both creation and parsing use the same versioned, colon-safe encoding.bridges/openclaw/manager.go (1)
594-617:⚠️ Potential issue | 🟠 MajorPersist the resolved DM session key before the first send.
This still only updates the local
sessionKey. UntilsyncSessionscatches up, the saved portal state keeps pointing at the synthetic key, so a fast reply or approval on the real session can missresolvePortal(...)and drop the first upstream event.Suggested fix
sessionKey := strings.TrimSpace(state.OpenClawSessionKey) if state.OpenClawDMCreatedFromContact && state.OpenClawSessionID == "" && isOpenClawSyntheticDMSessionKey(state.OpenClawSessionKey) { if resolvedKey, err := gateway.ResolveSessionKey(ctx, state.OpenClawSessionKey); err == nil && strings.TrimSpace(resolvedKey) != "" { sessionKey = strings.TrimSpace(resolvedKey) + state.OpenClawSessionKey = sessionKey + if err := saveOpenClawPortalState(ctx, msg.Portal, m.client.UserLogin, state); err != nil { + return nil, err + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/manager.go` around lines 594 - 617, The code only updates the local sessionKey variable after ResolveSessionKey; persist the resolved session key into the portal state so upstream events use the real key before the first SendMessage. Specifically, when gateway.ResolveSessionKey returns a non-empty resolvedKey (the block that currently sets sessionKey), assign strings.TrimSpace(resolvedKey) back to state.OpenClawSessionKey and persist that state using the existing portal-state persistence API your manager exposes (so that the saved portal points to the real session key) before calling gateway.SendMessage; keep the isOpenClawSyntheticDMSessionKey and m.syncSessions logic intact.
🧹 Nitpick comments (3)
bridges/codex/portal_state_db.go (1)
136-149: Prefer per-row tolerance for malformedstate_jsonvalues.At Line 147, a single bad JSON blob aborts the entire listing. Since invalid keys are already skipped at Line 142, consider skipping malformed JSON rows too, so one corrupted row doesn’t hide all other portals.
💡 Suggested change
state := &codexPortalState{} if strings.TrimSpace(stateRaw) != "" { if err := json.Unmarshal([]byte(stateRaw), state); err != nil { - return nil, err + continue } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/codex/portal_state_db.go` around lines 136 - 149, In the rows.Next() loop inside the portal listing code, don't abort the entire List when json.Unmarshal into codexPortalState fails; instead, handle malformed state_json on a per-row basis by skipping that row (continue) and optionally logging the parse error/context. Update the block where parseCodexPortalKey and json.Unmarshal are used (referencing parseCodexPortalKey, codexPortalState, and the rows.Next loop) so that a JSON unmarshal error does not return from the function but is ignored for that portal entry.bridges/ai/portal_send.go (1)
139-139: Add minimal context whensdk.SendEditViaPortalfails.Line 139 currently returns the SDK error as-is; wrapping it with portal/target context makes failures much easier to triage.
Suggested patch
- return sdk.SendEditViaPortal(oc.UserLogin, portal, sender, targetMsgID, timestamp, streamOrder, "ai_edit_target", converted) + if err := sdk.SendEditViaPortal(oc.UserLogin, portal, sender, targetMsgID, timestamp, streamOrder, "ai_edit_target", converted); err != nil { + return fmt.Errorf("send edit via portal failed (portal=%s target=%s): %w", portal.MXID, targetMsgID, err) + } + return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/portal_send.go` at line 139, Return a wrapped error that includes minimal context when sdk.SendEditViaPortal fails: catch the error returned from the call to sdk.SendEditViaPortal (the invocation using oc.UserLogin, portal, sender, targetMsgID, timestamp, streamOrder, "ai_edit_target", converted) and return a new error that includes the portal identifier and targetMsgID (e.g., via fmt.Errorf or errors.Wrapf) so the failure message contains portal and target context for easier triage.bridges/ai/media_understanding_runner_openai_test.go (1)
5-9: Use the parameterized connector directly instead of via helper replacement.
newTestAIClientWithProvidercurrently creates a bare&OpenAIConnector{}that gets immediately replaced. While this doesn't drop any state today, this pattern masks intent and could hide issues if the helper is extended with connector initialization in the future. Consider making tests that need a configured connector pass it directly rather than wrapping and replacing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/media_understanding_runner_openai_test.go` around lines 5 - 9, The test helper newMediaTestClient should not create a bare client via newTestAIClientWithProvider and then replace its connector; instead construct or initialize the AIClient with the provided OpenAIConnector directly (referencing newMediaTestClient, newTestAIClientWithProvider, AIClient, OpenAIConnector, and setTestLoginConfig) so the connector passed in (oc) is used as-is; update newMediaTestClient to create an AIClient with the intended provider and connector and then call setTestLoginConfig(client, cfg) before returning.
🤖 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/ai/bridge_db.go`:
- Around line 112-117: loginScopeForClient currently uses raw values from
loginDBContext so IDs with surrounding whitespace will be treated as non-empty
but won't match scopes created by other constructors; update loginScopeForClient
to trim bridgeID and loginID (e.g. strings.TrimSpace) immediately after calling
loginDBContext, check emptiness on the trimmed values, and return
&loginScope{db: db, bridgeID: trimmedBridgeID, loginID: trimmedLoginID}; this
mirrors the normalization performed in loginScopeForLogin and
portalScopeForPortal.
In `@bridges/ai/client.go`:
- Line 1703: The call to loadAITranscriptMessage uses the wrong args/types;
remove the extra oc and pass the Portal pointer and a networkid.MessageID for
messageID. Replace the call `loadAITranscriptMessage(ctx, oc, portal.MXID,
msg.ID)` with `loadAITranscriptMessage(ctx, portal, msg.ID)` (or
`loadAITranscriptMessage(ctx, portal, networkid.MessageID(msg.ID))` if an
explicit cast is needed) so the call matches the signature `(ctx
context.Context, portal *bridgev2.Portal, messageID networkid.MessageID)`.
In `@bridges/ai/delete_chat.go`:
- Around line 85-87: The DELETE uses room_id=$3 but transcripts are keyed by
portal_id; update the execDelete call that references aiTranscriptTable (in
delete_chat.go) to use `WHERE bridge_id=$1 AND login_id=$2 AND portal_id=$3` and
pass the portal identifier (e.g., portalID) instead of sessionKey so it matches
how persistAITranscriptMessage and transcript_db.go store/query rows.
In `@bridges/ai/handlematrix.go`:
- Line 357: The call to loadAITranscriptMessage is passing too many arguments
(transcriptMsg, err := loadAITranscriptMessage(ctx, oc, portal.MXID,
edit.EditTarget.ID); the helper's signature changed), so update the invocation
to match the helper's current signature or change the helper if you intended a
room-scoped lookup: either remove the extra parameters (e.g. drop portal.MXID or
edit.EditTarget.ID) and pass only the expected args from ctx and oc, or modify
loadAITranscriptMessage's function definition to accept (ctx, oc, mxid, roomID)
and update its implementation accordingly; locate loadAITranscriptMessage and
handlematrix.go references to keep the call and the helper signatures
consistent.
In `@bridges/ai/transcript_db.go`:
- Around line 211-214: The call to execDelete is passing portal.Bridge.Log by
value but execDelete expects a *zerolog.Logger; update the call site in the
transcript deletion to pass the logger pointer by using the address of
portal.Bridge.Log (i.e., change portal.Bridge.Log to &portal.Bridge.Log) so
execDelete receives *zerolog.Logger for the DELETE of aiTranscriptTable in the
function that uses scope.bridgeID, scope.loginID, scope.portalID.
In `@bridges/openclaw/manager.go`:
- Around line 1967-1971: The loop currently skips any message missing
turn/run/idempotency markers (using
openClawMessageTurnMarker/openClawMessageRunMarker/openClawMessageIdempotencyKey)
which prevents shouldMirrorLatestUserMessageFromHistory from ever hitting its
15-minute timestamp fallback; change the filter so messages without those
markers are still considered by shouldMirrorLatestUserMessageFromHistory (i.e.,
call shouldMirrorLatestUserMessageFromHistory(normalizeOpenClawLiveMessage(...))
before continuing or only continue when markers are missing AND
shouldMirrorLatestUserMessageFromHistory returns false) so gateways that omit
markers can still trigger the timestamp fallback.
In `@bridges/openclaw/metadata.go`:
- Around line 133-149: loadOpenClawPortalState currently returns an empty
openClawPortalState on a missing DB row which causes pre-upgrade portals to lack
session bindings; change the nil-state path to read legacy portal metadata from
the provided portal (e.g., portal.Metadata or existing legacy keys), construct
an openClawPortalState populated with OpenClawSessionKey/room state, persist it
back into the blob store (use openClawPortalStateBlob and
aidb.Save[openClawPortalState] or the project’s save helper) under the same
scope obtained from openClawPortalDBScopeFor, and then return the hydrated state
instead of an empty struct.
- Around line 258-285: The loadOpenClawLoginState function currently treats
sql.ErrNoRows as empty tokens, causing existing provisioned logins to lose
credentials after upgrade; instead, when QueryRow returns sql.ErrNoRows in
loadOpenClawLoginState, read the legacy credentials from the UserLogin's legacy
metadata (the fields on the bridgev2.UserLogin that previously stored
gateway_token/gateway_password/device_token), populate an
openClawPersistedLoginState with those values and return it (and persist it via
ensureOpenClawLoginStateTable if needed) so Start will see the seeded
credentials; update loadOpenClawLoginState to use bridgev2.UserLogin's legacy
metadata as the first-read fallback before returning an empty state.
- Around line 295-315: The code is trimming opaque credential fields before
saving which can corrupt valid credentials; in the DB write for
openclaw_login_state (the scope.db.Exec call that inserts/updates gateway_token,
gateway_password, device_token) stop calling strings.TrimSpace on
state.GatewayToken, state.GatewayPassword, and state.DeviceToken and pass the
raw values instead so the exact credential strings (from state) are persisted;
leave other fields (SessionsSynced, LastSyncAt, updated_at_ms) unchanged.
In `@pkg/aidb/json_blob_table.go`:
- Around line 47-56: The current QueryRow(...).Scan(&raw) result is checked for
empty raw before verifying err, which can hide real DB errors; change the logic
in the function that calls db.QueryRow(...).Scan(&raw) so you first inspect err
returned by Scan (using db.QueryRow and Scan on raw), handle sql.ErrNoRows by
returning (nil, nil) and return any other err immediately, and only after err is
nil check strings.TrimSpace(raw) == "" to return (nil, nil); refer to
t.TableName and t.KeyColumn in the same block to locate the query.
- Around line 17-20: JSONBlobTable's public TableName and KeyColumn need
defensive validation before being concatenated into SQL in Ensure, Load, Save,
and Delete; add a helper (e.g. validateIdentifier or
JSONBlobTable.validateIdentifiers) that enforces an SQL identifier allowlist
(e.g. regex ^[A-Za-z_][A-Za-z0-9_]*$) and call it at the start of Ensure, Load,
Save, and Delete, returning an error if either TableName or KeyColumn fails
validation to avoid unsafe SQL assembly.
---
Outside diff comments:
In `@bridges/ai/chat.go`:
- Around line 46-52: agentsEnabledForLogin currently treats a nil cfg.Agents as
false while shouldEnsureDefaultChat treats nil as enabled; make the default
behavior consistent by changing agentsEnabledForLogin to treat a nil cfg.Agents
as enabled (i.e., return true when cfg.Agents == nil) so it returns true by
default and otherwise returns *cfg.Agents; update the implementation in the
agentsEnabledForLogin function (which calls loginConfigSnapshot) to reflect this
consistent default with shouldEnsureDefaultChat.
In `@bridges/ai/handlematrix.go`:
- Around line 364-383: The code currently uses msgMeta.Role to decide
canonical-turn updates and whether to regenerate, but transcriptMeta may contain
the authoritative role; change the logic to derive an effectiveRole from
transcriptMeta (e.g., use transcriptMeta.Role when present, otherwise fall back
to msgMeta.Role) and use that effectiveRole in both the canonical-turn update
block (where setCanonicalTurnDataFromPromptMessages and cloneCanonicalTurnData
are called) and the regenerate gate at the end (the if that currently checks
msgMeta.Role != "user"); ensure you still persist via persistAITranscriptMessage
and call oc.notifySessionMutation as before.
In `@bridges/ai/session_store.go`:
- Around line 76-109: The SQL queries in getSessionEntry (and the other affected
functions around the 136-179 and 263-277 ranges) still use
scope.bridgeID/scope.loginID but must use the IDs carried in sessionStoreRef so
the ref fully defines the target; update the SELECT/INSERT/UPDATE/DELETE
parameter lists and WHERE clauses to use ref.BridgeID and ref.LoginID (e.g.,
replace scope.bridgeID, scope.loginID with ref.BridgeID, ref.LoginID) while
keeping normalizeAgentID(ref.AgentID) for agent ID and preserving sessionKey
handling; do the same substitution wherever those other functions (the
counterparts to getSessionEntry) currently rely on scope instead of the ref.
---
Duplicate comments:
In `@bridges/ai/handleai.go`:
- Around line 454-461: The code only updates local state (meta, portal.Name,
portal.NameSet) and calls oc.savePortalQuiet(...) which does not push changes to
Matrix; after saving, invoke the room-sync path (e.g. call
materializePortalRoom(bgCtx, portal) or EnsurePortalLifecycle(bgCtx, portal) as
used elsewhere) so the generated title is propagated to the Matrix room UI; keep
the existing oc.savePortalQuiet(...) call but add the appropriate
materialize/EnsurePortalLifecycle call using the same bgCtx and portal objects.
In `@bridges/ai/handlematrix.go`:
- Around line 942-959: savePortalQuiet currently swallows persistence errors
from saveAIPortalState and portal.Save so callers can't abort flows on failed
writes; change savePortalQuiet (and its callers) to return an error instead of
void, propagate any non-context.Canceled error from saveAIPortalState or
portal.Save up to the caller, and update all call sites to handle the returned
error (bail out / abort the one-shot flow) so flags like WelcomeSent /
AutoGreetingSent / title-generation are not left in an inconsistent state;
reference the function savePortalQuiet, the helper saveAIPortalState, and the
portal.Save call when making the signature and call-site changes.
In `@bridges/ai/login_state_db.go`:
- Around line 213-229: The updateLoginState function currently lets fn mutate
oc.loginState in-place before persisting; change it to clone the cached state
first (e.g., create a new loginRuntimeState copy of oc.loginState), call fn on
that cloned copy, and if fn returns true then call saveLoginRuntimeState(ctx,
oc, clonedState); only after save succeeds swap oc.loginState = clonedState
while holding oc.loginStateMu. Keep the existing loadLoginRuntimeState path and
locking (oc.loginStateMu) but ensure all mutations and the save operate on the
clone so the in-memory cache is only updated on successful persistence.
- Around line 70-103: When QueryRow against aiLoginStateTable in
loadLoginRuntimeState returns sql.ErrNoRows, don't return a brand-new state
immediately; instead attempt to load legacy runtime metadata (from the
pre-migration aichats_login_state or via a helper like loadLegacyLoginState)
using scope.bridgeID and scope.loginID, populate the loginRuntimeState fields
(NextChatIndex, LastHeartbeatEvent, model/file/gravatar caches,
ConsecutiveErrors, LastErrorAt) from that legacy data if present, and then
upsert/backfill a row into aiLoginStateTable so future loads hit the migrated
schema; handle and return any unexpected DB errors as before. Ensure this logic
is implemented inside loadLoginRuntimeState and references aiLoginStateTable,
scope.bridgeID, scope.loginID, and the new-row upsert path.
In `@bridges/ai/prompt_builder.go`:
- Around line 152-171: The current code applies hr.limit and the image-message
cap after merging internalRows into candidates so internal rows can evict recent
user/assistant turns; change the flow to keep internalRows out of the cap by
treating them separately: build/collect the regular candidates (the ones not
from loadInternalPromptHistory), apply slices.SortStableFunc + the hr.limit
truncation and the i < maxHistoryImageMessages logic only to that non-internal
list, and only after truncation append the internalRows converted to
replayCandidate (from loadInternalPromptHistory) into the final candidates slice
so they are not counted against hr.limit or the image-message window; refer to
loadInternalPromptHistory, internalRows, candidates, replayCandidate, hr.limit
and the i < maxHistoryImageMessages check when making this change.
In `@bridges/ai/tool_configured.go`:
- Around line 16-18: The code is using context.Background() when calling
loginConfigSnapshot which allows DB-backed config snapshots to outlive caller
cancellation; thread the incoming ctx through by changing effectiveSearchConfig
and effectiveFetchConfig to accept a context.Context and pass that ctx into
effectiveToolConfig, and update effectiveToolConfig to pass the received ctx
into loginConfigSnapshot (use the same ctx instead of context.Background());
update all call sites to forward the original caller ctx to these functions
(refer to effectiveSearchConfig, effectiveFetchConfig, effectiveToolConfig, and
loginConfigSnapshot).
In `@bridges/ai/tools.go`:
- Around line 390-397: The current branching in executeMessageReact treats emoji
== "" as an implicit remove and calls executeMessageReactRemove even when remove
is false; change the logic in executeMessageReact so only remove triggers the
removal path: call executeMessageReactRemove(ctx, args, btc) only when remove is
true, and if remove is false but emoji is empty return a validation error
indicating an emoji is required for adding a reaction (or otherwise handle the
missing-emoji case explicitly) so the add-reaction flow validates correctly;
update checks around the emoji and remove variables and any callers of
executeMessageReact accordingly.
In `@bridges/openclaw/identifiers.go`:
- Around line 31-36: Change the scoped-ID encoding from url.PathEscape to a
non-colon-producing binary-to-text scheme and add a version token: in
openClawScopedGhostUserID encode loginID and agentID with base64 URL-safe (no
padding) or hex, prefix the ID with a version (e.g. "openclaw-agent:v1:") and
join with ":"; update parseOpenClawGhostID to recognize the "v1" form and decode
the base64url/hex segments back to original values, and keep backward-compatible
handling for the old PathEscape format during rollout; also apply the same
change to the other factory/parser pair referenced around the 47-70 block so
both creation and parsing use the same versioned, colon-safe encoding.
In `@bridges/openclaw/manager.go`:
- Around line 594-617: The code only updates the local sessionKey variable after
ResolveSessionKey; persist the resolved session key into the portal state so
upstream events use the real key before the first SendMessage. Specifically,
when gateway.ResolveSessionKey returns a non-empty resolvedKey (the block that
currently sets sessionKey), assign strings.TrimSpace(resolvedKey) back to
state.OpenClawSessionKey and persist that state using the existing portal-state
persistence API your manager exposes (so that the saved portal points to the
real session key) before calling gateway.SendMessage; keep the
isOpenClawSyntheticDMSessionKey and m.syncSessions logic intact.
---
Nitpick comments:
In `@bridges/ai/media_understanding_runner_openai_test.go`:
- Around line 5-9: The test helper newMediaTestClient should not create a bare
client via newTestAIClientWithProvider and then replace its connector; instead
construct or initialize the AIClient with the provided OpenAIConnector directly
(referencing newMediaTestClient, newTestAIClientWithProvider, AIClient,
OpenAIConnector, and setTestLoginConfig) so the connector passed in (oc) is used
as-is; update newMediaTestClient to create an AIClient with the intended
provider and connector and then call setTestLoginConfig(client, cfg) before
returning.
In `@bridges/ai/portal_send.go`:
- Line 139: Return a wrapped error that includes minimal context when
sdk.SendEditViaPortal fails: catch the error returned from the call to
sdk.SendEditViaPortal (the invocation using oc.UserLogin, portal, sender,
targetMsgID, timestamp, streamOrder, "ai_edit_target", converted) and return a
new error that includes the portal identifier and targetMsgID (e.g., via
fmt.Errorf or errors.Wrapf) so the failure message contains portal and target
context for easier triage.
In `@bridges/codex/portal_state_db.go`:
- Around line 136-149: In the rows.Next() loop inside the portal listing code,
don't abort the entire List when json.Unmarshal into codexPortalState fails;
instead, handle malformed state_json on a per-row basis by skipping that row
(continue) and optionally logging the parse error/context. Update the block
where parseCodexPortalKey and json.Unmarshal are used (referencing
parseCodexPortalKey, codexPortalState, and the rows.Next loop) so that a JSON
unmarshal error does not return from the function but is ignored for that portal
entry.
🪄 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: ebf6521c-20e6-4475-88ec-5ff0c18d4ae4
📒 Files selected for processing (78)
bridges/ai/agent_loop_request_builders_test.gobridges/ai/agent_loop_routing_test.gobridges/ai/approval_prompt_presentation.gobridges/ai/bridge_db.gobridges/ai/chat.gobridges/ai/chat_bootstrap_test.gobridges/ai/chat_login_redirect_test.gobridges/ai/chat_resolve_agent_identifier_test.gobridges/ai/client.gobridges/ai/client_capabilities_test.gobridges/ai/custom_agents_db.gobridges/ai/defaults_alignment_test.gobridges/ai/delete_chat.gobridges/ai/desktop_api_sessions.gobridges/ai/desktop_api_sessions_test.gobridges/ai/handleai.gobridges/ai/handlematrix.gobridges/ai/image_generation_tool.gobridges/ai/image_generation_tool_magic_proxy_test.gobridges/ai/integration_host.gobridges/ai/internal_prompt_db.gobridges/ai/login.gobridges/ai/login_config_db.gobridges/ai/login_loaders.gobridges/ai/login_loaders_test.gobridges/ai/login_state_db.gobridges/ai/logout_cleanup.gobridges/ai/magic_proxy_test.gobridges/ai/mcp_helpers.gobridges/ai/mcp_servers_test.gobridges/ai/media_understanding_runner.gobridges/ai/media_understanding_runner_openai_test.gobridges/ai/message_parts.gobridges/ai/metadata.gobridges/ai/model_catalog.gobridges/ai/model_catalog_test.gobridges/ai/portal_send.gobridges/ai/portal_state_db.gobridges/ai/prompt_builder.gobridges/ai/provisioning.gobridges/ai/provisioning_test.gobridges/ai/reply_mentions.gobridges/ai/responder_metadata_test.gobridges/ai/responder_resolution_test.gobridges/ai/scheduler_db.gobridges/ai/scheduler_heartbeat_test.gobridges/ai/sdk_agent_catalog_test.gobridges/ai/session_store.gobridges/ai/streaming_init_test.gobridges/ai/streaming_output_handlers.gobridges/ai/streaming_persistence.gobridges/ai/streaming_tool_selection_test.gobridges/ai/system_events_db.gobridges/ai/test_login_helpers_test.gobridges/ai/token_resolver.gobridges/ai/tool_approvals_rules.gobridges/ai/tool_availability_configured_test.gobridges/ai/tool_configured.gobridges/ai/tool_policy_apply_patch_test.gobridges/ai/tools.gobridges/ai/tools_search_fetch.gobridges/ai/tools_search_fetch_test.gobridges/ai/tools_tts_test.gobridges/ai/transcript_db.gobridges/codex/backfill.gobridges/codex/portal_state_db.gobridges/openclaw/identifiers.gobridges/openclaw/manager.gobridges/openclaw/metadata.gobridges/opencode/opencode_identifiers.gobridges/opencode/opencode_manager.gopkg/aidb/001-init.sqlpkg/aidb/db_test.gopkg/aidb/json_blob_table.gopkg/shared/stringutil/hash.gosdk/approval_prompt.gosdk/message_metadata.goturns/session_target_test.go
✅ Files skipped from review due to trivial changes (5)
- bridges/ai/desktop_api_sessions.go
- bridges/ai/login.go
- bridges/ai/login_config_db.go
- bridges/ai/media_understanding_runner.go
- bridges/ai/internal_prompt_db.go
🚧 Files skipped from review as they are similar to previous changes (14)
- bridges/ai/streaming_output_handlers.go
- bridges/ai/mcp_helpers.go
- pkg/aidb/db_test.go
- bridges/ai/sdk_agent_catalog_test.go
- bridges/ai/model_catalog.go
- bridges/ai/image_generation_tool.go
- bridges/ai/login_loaders.go
- bridges/ai/system_events_db.go
- bridges/opencode/opencode_identifiers.go
- bridges/ai/custom_agents_db.go
- bridges/ai/portal_state_db.go
- bridges/codex/backfill.go
- bridges/ai/token_resolver.go
- bridges/ai/scheduler_db.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-cli-docker (amd64)
- GitHub Check: build-docker
- GitHub Check: Lint
- GitHub Check: Lint
- 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/ai/chat_bootstrap_test.gobridges/ai/client_capabilities_test.gobridges/ai/magic_proxy_test.gobridges/ai/tool_configured.gobridges/ai/tool_availability_configured_test.gobridges/ai/streaming_tool_selection_test.gobridges/ai/tool_policy_apply_patch_test.gobridges/ai/defaults_alignment_test.gobridges/ai/tools_tts_test.gobridges/ai/test_login_helpers_test.gobridges/ai/provisioning.gobridges/ai/client.gobridges/ai/handleai.gobridges/ai/tools_search_fetch.go
🪛 GitHub Check: Package Smoke
bridges/ai/transcript_db.go
[failure] 211-211:
cannot use portal.Bridge.Log (variable of struct type zerolog.Logger) as *zerolog.Logger value in argument to execDelete
bridges/ai/handlematrix.go
[failure] 357-357:
too many arguments in call to loadAITranscriptMessage
bridges/ai/client.go
[failure] 1703-1703:
too many arguments in call to loadAITranscriptMessage
🔇 Additional comments (44)
bridges/ai/approval_prompt_presentation.go (3)
6-6: SDK import direction change is clean.The import swap is straightforward and consistent with the rest of this file’s updated types/helpers.
9-20: Builtin approval presentation flow looks solid.Trimming at ingress, conditional detail emission, and bounded arg expansion are implemented cleanly, and the final builder call is coherent.
22-37: MCP approval presentation path is well-structured.The server/tool normalization and input fallback behavior (
mapdetails vs summarized value) are clear and robust.bridges/ai/login_loaders_test.go (3)
4-4: SDK migration changes are consistent and correct.The updated import and broken-client type assertion align with the direct
agentremote/sdkusage and keep this test in sync with the new client type wiring.Also applies to: 13-13, 70-70
30-33: Rebuild predicate tests are well-updated for config-backed state.This test now correctly exercises
aiClientNeedsRebuildConfigfor equivalent config values and all key rebuild triggers (API key, provider, base URL, and nil cached client).Also applies to: 35-47
61-61:loadAIUserLogincall-site update is correct.Passing
context.Background()matches the new method signature and keeps the behavior-focused assertions intact.bridges/opencode/opencode_manager.go (11)
17-18: LGTM!The new imports for
stringutilandsdkare appropriately added and used throughout the file for the approval flow migration and hash generation.
27-38: LGTM!The struct field type updates to
sdk.ApprovalFlow[*permissionApprovalRef]andsdk.ApprovalPromptPresentationare consistent with the SDK migration pattern.
40-61: LGTM!The function correctly builds the approval presentation using SDK types. The pre-allocation with capacity 8 is reasonable for the expected number of detail entries.
94-111: LGTM!The
DeliverDecisioncallback correctly handles the nil data edge case by returningsdk.ErrApprovalUnknown, and usessdk.DecisionToStringto map decisions to the expected response strings.
265-277: LGTM!The
knownSessionsmap is properly initialized as a set-like structure to track session IDs. This enables the new session lifecycle tracking viarememberSessionandforgetSessioncalls.
811-822: LGTM!The
SendPromptcall correctly uses the SDK's nested parameter structure. The 10-minute expiration aligns with the approval registration timeout.
849-855: LGTM!The three-tier fallback logic for resolution origin (ResolvedBy → Source → User) is defensive and handles cases where the origin isn't specified in the external reply event.
874-880: LGTM!The
ResolveExternalcall correctly constructs thesdk.ApprovalDecisionPayloadto handle permission replies that originated outside the Matrix bridge.
1227-1244: Good scoping improvement.Changing from
listAllChatPortalstolistInstanceChatPortalsefficiently limits portal enumeration to the affected instance. The InstanceID check at line 1236 is now somewhat redundant but provides a reasonable safety guard.
504-504: Session lifecycle tracking is correctly implemented.The
rememberSessionandforgetSessionmethods are properly defined onopenCodeInstanceand handle the trimmed session IDs appropriately.
1255-1261: No algorithm change—ShortHash already uses SHA-256 with hex encoding.
stringutil.ShortHash(trimmed, 8)implements SHA-256 + hex encoding of the first n bytes, the same approach the review assumes was the original. This is not a breaking change; if the prior code used the same method, the output is identical.> Likely an incorrect or invalid review comment.bridges/codex/portal_state_db.go (2)
44-116: Good defensive scope handling and persistence guards.The nil-guarded scope construction and Ensure-before-CRUD flow are clean and reduce accidental panics in this path.
159-172: The suggested fix references a non-existent API and the current implementation is correct.The
networkid.ParsePortalKey()function does not exist in the maunium.net/go/mautrix/bridgev2/networkid package. The current code's approach of usingstrings.Cut(raw, "/")to split the serialized string is the canonical method for parsingPortalKey.String()output, which produces either"{ID}/{Receiver}"or"{ID}"format.Additionally, portal IDs in this codebase are designed with formats like
"codex:loginid:type:identifier"and"service:loginid:sessionid", which do not contain "/" characters, making the "/" delimiter safe and appropriate for round-trip serialization. The parsing logic is correct and does not suffer from the data loss concern described.> Likely an incorrect or invalid review comment.bridges/ai/metadata.go (1)
310-313: Nice deep-copy forTypingIntervalSeconds.This removes a remaining pointer alias from
clonePortalMetadata, so later runtime edits won't mutate previously captured snapshots.bridges/ai/reply_mentions.go (1)
71-76: Nice switch to the portal-aware part loader.This keeps reply detection aligned with the scoped message-part path instead of reaching around it with a direct message-table lookup.
bridges/ai/portal_send.go (1)
188-188: Portal-scoped MXID lookup is a solid fix.Using
loadPortalMessagePartByMXIDon Line 188 is a good correctness improvement over global lookup.bridges/ai/provisioning_test.go (1)
14-56: Nice update to config-backed test coverage.The rewritten assertions validate both profile lifecycle and timezone persistence/clearing against
aiLoginConfigcleanly.bridges/ai/chat_bootstrap_test.go (1)
11-38: Type migration in table tests looks consistent.
shouldEnsureDefaultChatinputs and cases now correctly track*aiLoginConfig.bridges/ai/magic_proxy_test.go (2)
40-47: Good signature migration for service config resolution.Passing
ProviderMagicProxy+ login config directly keeps these tests aligned with the new resolver contract.
99-104: Exa proxy resolution test is correctly adapted.Line 104 now validates the login-config-driven base URL path as intended.
bridges/ai/tool_policy_apply_patch_test.go (1)
6-13: Helper refactor improves test fixture consistency.Using shared client/login-state setup here is clean and matches the rest of the suite.
bridges/ai/streaming_tool_selection_test.go (1)
26-49: Refactored test client bootstrap looks solid.The explicit runtime-state setup keeps tool-selection tests deterministic and readable.
bridges/ai/client_capabilities_test.go (2)
15-19: Good move to shared AIClient/login-state helpers.This keeps capability tests aligned with the new runtime-state model.
112-112: UsingNewAIConnector()here is the right call.Line 112 ensures the test validates capabilities against constructor defaults, not a partial literal.
bridges/ai/agent_loop_routing_test.go (1)
6-10: Helper rewrite is clean and consistent.The shared setup pattern makes model-routing tests easier to maintain.
bridges/ai/image_generation_tool_magic_proxy_test.go (1)
8-13: Shared Magic Proxy fixture migration looks good.These cases still cover the same provider/base-URL behavior while moving setup onto
aiLoginConfig, which keeps the tests aligned with the new login config/state flow.Also applies to: 28-33, 48-53, 69-74, 90-95, 129-134, 146-151
bridges/ai/tool_availability_configured_test.go (1)
17-27: Helper-based state setup is a solid cleanup.Using the shared client/login-state helpers keeps these availability tests focused on tool gating instead of low-level fixture wiring.
Also applies to: 43-55, 65-77, 90-94
bridges/ai/desktop_api_sessions_test.go (1)
6-16: Nice simplification of the desktop API fixture.This keeps the test scoped to the token merge behavior and removes a lot of incidental login wiring.
bridges/ai/mcp_servers_test.go (1)
6-10: The MCP test helper cleanup looks good.Injecting MCP server config through the shared login helper makes the fixture easier to reuse and keeps it consistent with the rest of the suite.
bridges/ai/defaults_alignment_test.go (1)
17-23: These defaults-alignment fixtures are in better shape now.Using the DB-backed/custom-agent helpers here makes the tests closer to the paths the production code actually reads from.
Also applies to: 38-44, 59-65
bridges/ai/agent_loop_request_builders_test.go (1)
13-25: The request-builder tests are cleaner with the shared helpers.The new setup keeps the assertions on the builder outputs while avoiding duplicated login metadata scaffolding.
Also applies to: 62-80
bridges/ai/tools_search_fetch_test.go (1)
11-22: The updated search-config call sites look right.Passing the provider explicitly makes these tests clearer about which branch they are exercising, and the important token-handling assertions are still intact.
Also applies to: 49-49, 61-74
bridges/ai/responder_resolution_test.go (1)
9-19: Nice test-helper migration.These setups are much less brittle now, and the assertions still exercise the responder-resolution behavior rather than bridge fixture wiring.
Also applies to: 43-55, 95-102
bridges/ai/tools_tts_test.go (1)
5-8: Helper cleanup looks good.This keeps the provider/login-config setup centralized while preserving the important fallback and no-connector cases.
Also applies to: 13-15, 28-30, 50-50, 62-62
bridges/ai/message_parts.go (1)
12-30: Good portal-scoped guard.Verifying the fetched part against
portal.PortalKeyis the right safety check for the new shared lookup helper.bridges/ai/chat_login_redirect_test.go (1)
38-54: This test setup is much cleaner now.Using the shared login-state/config helpers keeps these cases focused on search and redirect behavior instead of metadata plumbing.
Also applies to: 84-85, 169-177
bridges/ai/scheduler_heartbeat_test.go (1)
58-62: Nice alignment with the new runtime/schema wiring.Using
EnsureSchema, the shared login-config helper, and the table constant should make these tests less fragile against future schema churn.Also applies to: 84-84, 118-137
bridges/ai/streaming_persistence.go (1)
120-158: Good persistence backstop.Deriving a fallback message ID and persisting the transcript separately should make completed assistant turns more resilient across the different send paths.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (5)
bridges/openclaw/manager.go (2)
1959-1964:⚠️ Potential issue | 🟠 MajorThis filter still makes the timestamp fallback unreachable.
Skipping every candidate that lacks turn/run/idempotency markers means
shouldMirrorLatestUserMessageFromHistorynever reaches its 15-minute timestamp fallback for gateways that omit those markers. Let the helper inspect marker-less messages instead of continuing here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/manager.go` around lines 1959 - 1964, The loop currently skips any normalized message that lacks turn/run/idempotency markers, which prevents shouldMirrorLatestUserMessageFromHistory from applying its 15-minute timestamp fallback; remove the early continue so that normalizeOpenClawLiveMessage(payload.TS, history.Messages[idx]) results are always passed into shouldMirrorLatestUserMessageFromHistory and let that helper decide whether to mirror marker-less messages (i.e., stop filtering by openClawMessageTurnMarker/openClawMessageRunMarker/openClawMessageIdempotencyKey in the loop and call shouldMirrorLatestUserMessageFromHistory for every message).
594-617:⚠️ Potential issue | 🟠 MajorPersist or synchronously resync the real session before the first send.
When a contact DM resolves from a synthetic key to a real gateway session, only the local
sessionKeyis updated here. The portal still points at the synthetic session until the asyncsyncSessionsfinishes, so the firstchat/approval event on the real key can arrive before the room is remapped and get dropped. Update/save the resolved key beforeSendMessage, or make this first resync synchronous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/manager.go` around lines 594 - 617, When ResolveSessionKey returns a real session for a synthetic contact key, don’t only update the local sessionKey variable — persist or synchronously remap the portal before sending so the first message isn’t dropped. Modify the block that calls gateway.ResolveSessionKey (checking state.OpenClawDMCreatedFromContact, state.OpenClawSessionID and isOpenClawSyntheticDMSessionKey) to either (a) assign the resolvedKey back into state.OpenClawSessionKey and persist that state using the existing contact/state persistence routine before calling gateway.SendMessage, or (b) call m.syncSessions synchronously (wait for it to complete) before gateway.SendMessage so the portal is remapped; keep the ResolveSessionKey and gateway.SendMessage calls but ensure state persistence or synchronous m.syncSessions happens first.bridges/ai/bridge_db.go (1)
112-117:⚠️ Potential issue | 🟡 MinorTrim
bridgeIDandloginIDhere too.This path still builds a scope from raw
loginDBContextvalues, so whitespace-padded IDs won't match scopes created byloginScopeForLogin/portalScopeForPortal.Suggested fix
func loginScopeForClient(client *AIClient) *loginScope { db, bridgeID, loginID := loginDBContext(client) + bridgeID = strings.TrimSpace(bridgeID) + loginID = strings.TrimSpace(loginID) if db == nil || bridgeID == "" || loginID == "" { return nil } return &loginScope{db: db, bridgeID: bridgeID, loginID: loginID} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/bridge_db.go` around lines 112 - 117, loginScopeForClient currently builds a scope from raw loginDBContext values so whitespace-padded IDs can mismatch scopes created by loginScopeForLogin or portalScopeForPortal; call strings.TrimSpace (or equivalent trimming) on the bridgeID and loginID returned from loginDBContext, validate the trimmed values (db != nil && bridgeID != "" && loginID != ""), and construct the returned &loginScope using the trimmed bridgeID and loginID so IDs are normalized consistent with loginScopeForLogin/portalScopeForPortal.bridges/ai/handleai.go (2)
317-319:⚠️ Potential issue | 🟠 MajorPersist the one-shot flags before sending the message.
These paths still keep going if
savePortalQuietfails. That can leaveAutoGreetingSent/WelcomeSentfalse in storage even though the greeting/notice was already sent, so retries or a restart can emit duplicates.Also applies to: 385-389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handleai.go` around lines 317 - 319, Persist the one-shot flags before sending the message: when setting currentMeta.AutoGreetingSent (and similarly WelcomeSent) call oc.savePortalQuiet(bgCtx, current, "...") and if that save returns an error, stop and return the error instead of proceeding to oc.dispatchInternalMessage; ensure the code checks the error from savePortalQuiet and only calls oc.dispatchInternalMessage when persistence succeeded so the flags in storage always reflect sent status (apply same change in the AutoGreetingSent block around dispatchInternalMessage and the WelcomeSent block at the 385-389 range).
454-461:⚠️ Potential issue | 🟠 MajorSync the generated title to Matrix after saving it.
This only persists
portal.Name/portal.NameSet; it doesn't invoke the room-lifecycle sync that actually propagates the rename to the Matrix room, so the DB and room UI can drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handleai.go` around lines 454 - 461, The code sets portal.Name and portal.NameSet and calls oc.savePortalQuiet(portal, "room title") but never triggers the room-lifecycle sync that propagates the rename to Matrix; after the save, call the existing room-lifecycle sync mechanism (the same routine used elsewhere to push portal changes to Matrix) so the generated title is sent to the Matrix room UI — locate the block around portalMeta(portal) and oc.savePortalQuiet and add the post-save call to the room sync routine to ensure DB and Matrix stay in sync.
🧹 Nitpick comments (2)
pkg/agents/tools/params.go (2)
44-48: Silent truncation when converting float to int.
int(n)truncates decimals silently (e.g.,3.9→3). If tool parameters could contain decimal values that should be rejected or rounded, consider validating. If whole numbers are always expected, this is fine as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/agents/tools/params.go` around lines 44 - 48, ReadInt currently casts the float returned by ReadNumber to int which silently truncates decimals; update ReadInt(params map[string]any, key string, required bool) to validate that the returned float64 from ReadNumber is an integer (e.g., compare n to math.Trunc(n) or use math.Modf) and return a clear error if it has a fractional part, only then convert to int and return; keep using ReadNumber as the source of the numeric value and include the param key in the error message for clarity.
60-70: Inconsistent error handling compared toReadStringandReadMap.When
required=true, this function always returns"parameter %q is required"even when the key exists but has the wrong type.ReadStringandReadMapdistinguish between missing/nil and wrong-type cases with different error messages.For consistency, consider adding the same check:
Proposed fix
func ReadStringSlice(params map[string]any, key string, required bool) ([]string, error) { arr := maputil.StringSliceArg(params, key) if arr != nil { return arr, nil } if required { + if _, ok := params[key]; !ok || params[key] == nil { + return nil, fmt.Errorf("parameter %q is required", key) + } + return nil, fmt.Errorf("parameter %q must be a string array", key) - return nil, fmt.Errorf("parameter %q is required", key) } return nil, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/agents/tools/params.go` around lines 60 - 70, ReadStringSlice currently returns the generic "parameter %q is required" when required=true even if the key exists but has the wrong type; update ReadStringSlice (and its use of maputil.StringSliceArg) to mimic ReadString/ReadMap: first check params[key] presence (v, ok := params[key]); if v is present but maputil.StringSliceArg returns nil, return a type error such as fmt.Errorf("parameter %q must be an array of strings", key); only return the "required" missing error when the key is absent or nil and required==true, otherwise return nil,nil for optional missing keys.
🤖 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/ai/client.go`:
- Around line 648-670: updateBridgeMessageMetadata currently overwrites
existing.Metadata with an empty MessageMetadata{} instead of applying the
computed meta; change it to set the found row's Metadata to the passed-in meta
(or a defensive copy of meta) before calling
oc.UserLogin.Bridge.DB.Message.Update so the caller's metadata (including
GeneratedFiles overlay) is persisted; specifically update existing.Metadata =
meta (or clone meta) in the updateBridgeMessageMetadata function prior to the DB
update.
In `@bridges/ai/handlematrix.go`:
- Around line 384-390: The code is zeroing out the bridge message metadata by
assigning edit.EditTarget.Metadata = &MessageMetadata{} before writing back;
instead preserve or merge the edited transcript metadata into the bridge row so
the edited body/role/canonical fields are mirrored. Remove the unconditional
overwrite of edit.EditTarget.Metadata with an empty MessageMetadata and either
leave edit.EditTarget.Metadata as supplied or merge its non-nil fields into the
existing bridge metadata before calling
oc.UserLogin.Bridge.DB.Message.Update(ctx, edit.EditTarget), referencing
edit.EditTarget, MessageMetadata and oc.UserLogin.Bridge.DB.Message.Update to
locate the change.
In `@bridges/ai/internal_prompt_db.go`:
- Around line 118-120: When DecodeTurnData(raw) returns ok==false, we should log
a warning instead of silently continuing: add a warning log inside the loop that
includes the event identifiers (event_id and portal_id from the current
row/context) and any minimal context available (e.g., raw/canonical_turn_data or
a digest) so corrupted rows are debuggable; modify the block around
DecodeTurnData/raw/turnData to call the existing logger in scope (e.g., logger,
log, or s.log) with a message like "skipping malformed canonical_turn_data" plus
event_id and portal_id before continuing.
In `@bridges/openclaw/manager.go`:
- Around line 1804-1807: If loadOpenClawPortalState(ctx, portal,
m.client.UserLogin) returns an error you must not return early; instead fall
back to an empty/default state for rendering and still call ResolveExternal(...)
and clearApprovalHint(...) to drop the local approval entry. Concretely: call
loadOpenClawPortalState and if it errors, log or capture the error, set state to
an empty/default OpenClawPortalState value, then proceed to call the existing
ResolveExternal and clearApprovalHint code paths (using portal and
m.client.UserLogin as before) so the Matrix-side approval is always
resolved/dropped even when state loading fails.
- Around line 2365-2374: startRunRecovery currently captures the full
*openClawPortalState and passes it into a goroutine, which can cause stale state
to be persisted later by recoverRunPreview/saveOpenClawPortalState; instead,
change startRunRecovery to send only immutable identifiers (e.g., portal.MXID,
turnID, runID, agentID) into the goroutine and have
waitForRunCompletion/recoverRunPreview reload the latest openClawPortalState
from storage (by calling whatever load method is used for portal state)
immediately before any mutation or before calling saveOpenClawPortalState so you
always persist a fresh snapshot; apply the same pattern to the similar usage
referenced around recoverRunPreview (the other occurrence noted).
In `@pkg/agents/tools/params.go`:
- Around line 12-22: The current logic treats an intentionally provided empty
string as a non-string and emits "parameter %q must be a string"; update the
required-param handling so an empty string is recognized as a string but
rejected with a clear message. In the block using s := maputil.StringArg(params,
key) and variables params, key, required, after s != "" check, if required and
params contains the key but s == "" return fmt.Errorf("parameter %q is required
and cannot be empty"), otherwise keep the existing missing-key error and the
"must be a string" error for non-string cases.
In `@pkg/aidb/001-init.sql`:
- Around line 202-205: Several *_json columns (e.g., last_heartbeat_event_json,
model_cache_json, gravatar_json, file_annotation_cache_json and the other *_json
columns noted) use DEFAULT '' which is not valid JSON; change each DEFAULT '' to
a valid JSON literal that matches the column's expected shape (use DEFAULT '{}'
for JSON objects and DEFAULT '[]' for JSON arrays), update the column
definitions (the *_json columns in this file) accordingly, and adjust any
INSERT/seed SQL that relies on empty-string defaults to instead rely on the new
valid JSON defaults.
---
Duplicate comments:
In `@bridges/ai/bridge_db.go`:
- Around line 112-117: loginScopeForClient currently builds a scope from raw
loginDBContext values so whitespace-padded IDs can mismatch scopes created by
loginScopeForLogin or portalScopeForPortal; call strings.TrimSpace (or
equivalent trimming) on the bridgeID and loginID returned from loginDBContext,
validate the trimmed values (db != nil && bridgeID != "" && loginID != ""), and
construct the returned &loginScope using the trimmed bridgeID and loginID so IDs
are normalized consistent with loginScopeForLogin/portalScopeForPortal.
In `@bridges/ai/handleai.go`:
- Around line 317-319: Persist the one-shot flags before sending the message:
when setting currentMeta.AutoGreetingSent (and similarly WelcomeSent) call
oc.savePortalQuiet(bgCtx, current, "...") and if that save returns an error,
stop and return the error instead of proceeding to oc.dispatchInternalMessage;
ensure the code checks the error from savePortalQuiet and only calls
oc.dispatchInternalMessage when persistence succeeded so the flags in storage
always reflect sent status (apply same change in the AutoGreetingSent block
around dispatchInternalMessage and the WelcomeSent block at the 385-389 range).
- Around line 454-461: The code sets portal.Name and portal.NameSet and calls
oc.savePortalQuiet(portal, "room title") but never triggers the room-lifecycle
sync that propagates the rename to Matrix; after the save, call the existing
room-lifecycle sync mechanism (the same routine used elsewhere to push portal
changes to Matrix) so the generated title is sent to the Matrix room UI — locate
the block around portalMeta(portal) and oc.savePortalQuiet and add the post-save
call to the room sync routine to ensure DB and Matrix stay in sync.
In `@bridges/openclaw/manager.go`:
- Around line 1959-1964: The loop currently skips any normalized message that
lacks turn/run/idempotency markers, which prevents
shouldMirrorLatestUserMessageFromHistory from applying its 15-minute timestamp
fallback; remove the early continue so that
normalizeOpenClawLiveMessage(payload.TS, history.Messages[idx]) results are
always passed into shouldMirrorLatestUserMessageFromHistory and let that helper
decide whether to mirror marker-less messages (i.e., stop filtering by
openClawMessageTurnMarker/openClawMessageRunMarker/openClawMessageIdempotencyKey
in the loop and call shouldMirrorLatestUserMessageFromHistory for every
message).
- Around line 594-617: When ResolveSessionKey returns a real session for a
synthetic contact key, don’t only update the local sessionKey variable — persist
or synchronously remap the portal before sending so the first message isn’t
dropped. Modify the block that calls gateway.ResolveSessionKey (checking
state.OpenClawDMCreatedFromContact, state.OpenClawSessionID and
isOpenClawSyntheticDMSessionKey) to either (a) assign the resolvedKey back into
state.OpenClawSessionKey and persist that state using the existing contact/state
persistence routine before calling gateway.SendMessage, or (b) call
m.syncSessions synchronously (wait for it to complete) before
gateway.SendMessage so the portal is remapped; keep the ResolveSessionKey and
gateway.SendMessage calls but ensure state persistence or synchronous
m.syncSessions happens first.
---
Nitpick comments:
In `@pkg/agents/tools/params.go`:
- Around line 44-48: ReadInt currently casts the float returned by ReadNumber to
int which silently truncates decimals; update ReadInt(params map[string]any, key
string, required bool) to validate that the returned float64 from ReadNumber is
an integer (e.g., compare n to math.Trunc(n) or use math.Modf) and return a
clear error if it has a fractional part, only then convert to int and return;
keep using ReadNumber as the source of the numeric value and include the param
key in the error message for clarity.
- Around line 60-70: ReadStringSlice currently returns the generic "parameter %q
is required" when required=true even if the key exists but has the wrong type;
update ReadStringSlice (and its use of maputil.StringSliceArg) to mimic
ReadString/ReadMap: first check params[key] presence (v, ok := params[key]); if
v is present but maputil.StringSliceArg returns nil, return a type error such as
fmt.Errorf("parameter %q must be an array of strings", key); only return the
"required" missing error when the key is absent or nil and required==true,
otherwise return nil,nil for optional missing keys.
🪄 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: 977baf40-acc9-4e24-b111-8baed5fec90e
📒 Files selected for processing (31)
bridges/ai/bridge_db.gobridges/ai/client.gobridges/ai/delete_chat.gobridges/ai/handleai.gobridges/ai/handlematrix.gobridges/ai/internal_dispatch.gobridges/ai/internal_prompt_db.gobridges/ai/message_parts.gobridges/ai/portal_cleanup.gobridges/ai/prompt_builder.gobridges/ai/reaction_handling.gobridges/ai/reactions.gobridges/ai/streaming_persistence.gobridges/ai/subagent_spawn.gobridges/ai/transcript_db.gobridges/openclaw/manager.gobridges/opencode/opencode_manager.gopkg/agents/store.gopkg/agents/tools/boss.gopkg/agents/tools/params.gopkg/agents/tools/types.gopkg/aidb/001-init.sqlpkg/integrations/cron/tool_exec.gopkg/shared/maputil/bool_arg.gopkg/shared/maputil/number_arg.gopkg/shared/maputil/slice_arg.gopkg/shared/maputil/string_arg.gopkg/shared/toolspec/toolspec.gosdk/approval_flow.gosdk/approval_reaction_helpers.gosdk/helpers.go
✅ Files skipped from review due to trivial changes (2)
- pkg/agents/store.go
- pkg/agents/tools/boss.go
🚧 Files skipped from review as they are similar to previous changes (7)
- bridges/ai/portal_cleanup.go
- bridges/ai/internal_dispatch.go
- bridges/ai/subagent_spawn.go
- bridges/ai/reaction_handling.go
- bridges/ai/reactions.go
- bridges/ai/transcript_db.go
- bridges/ai/prompt_builder.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). (6)
- GitHub Check: build-agentremote-cli-docker (arm64)
- GitHub Check: build-agentremote-cli-docker (amd64)
- GitHub Check: Lint
- GitHub Check: build-docker
- GitHub Check: Lint
- 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/ai/handleai.gobridges/ai/client.go
🔇 Additional comments (25)
pkg/agents/tools/types.go (2)
10-17: Backwards-compatible type aliasing looks correct.Using aliases here cleanly preserves existing
tools.ToolType/tools.ToolInfocall sites while moving the canonical definitions topkg/shared/toolspec.
19-23: Constant re-exports are aligned with the new shared source of truth.The mapping keeps existing comparisons (e.g., plugin-type checks) intact while avoiding duplicated enum definitions.
bridges/opencode/opencode_manager.go (11)
17-18: LGTM!The import changes correctly bring in the new SDK package and the
stringutilhelper, aligning with the migration from the priorgithub.com/beeper/agentremotemodule types.
27-38: LGTM!The type changes for
approvalFlowandPresentationfields correctly use the new SDK types while maintaining the generic parameter forpermissionApprovalRef.
40-53: LGTM!The function correctly uses SDK helpers (
ApprovalDetail,ValueSummary,AppendDetailsFromMap,BuildApprovalPresentation) to construct the approval presentation. The logic flow for extracting permission, patterns, and metadata is sound.
60-111: LGTM!The approval flow configuration correctly uses SDK types. The
DeliverDecisioncallback properly handles nil refs by returningsdk.ErrApprovalUnknown, and usessdk.DecisionToStringwith appropriate OpenCode-specific response strings ("once", "always", "reject").
263-263: LGTM!Pre-initializing
knownSessionsduring instance construction is a good practice. WhilerememberSessionhas a defensive nil check (per the context snippet), eager initialization here avoids potential race conditions during concurrent access.
496-496: LGTM!Correctly tracks session IDs during sync using
rememberSessionwith trimmed session ID.
642-642: LGTM!Properly removes session from tracking during deletion using
forgetSessionwith trimmed session ID.
803-814: LGTM!The
SendPromptcall correctly uses SDK param types. The 10-minute expiration matches the registration timeout set on line 773, ensuring consistency.
841-872: LGTM!The resolution handling correctly parses the origin with a proper fallback chain (
ResolvedBy→Source→ApprovalResolutionOriginUser). TheResolveExternalcall properly constructs theApprovalDecisionPayloadwith approval state derived from the reply.
1219-1219: LGTM!Narrowing the query to
listInstanceChatPortalsis a good improvement. While the subsequent filter on line 1228 would catch mismatches, querying only relevant portals is more efficient and makes the intent clearer.
1247-1253: LGTM!Using
stringutil.ShortHashsimplifies the code by leveraging a shared utility. The function uses SHA-256 for deterministic hashing, taking 8 bytes and encoding as hex (16 characters), prefixed with "msg_mx_" to identify Matrix-originated messages. This provides reasonable uniqueness for session-scoped message IDs.pkg/agents/tools/params.go (6)
25-28: LGTM!Clean delegation to the shared helper.
30-42: LGTM!Error handling properly distinguishes missing vs wrong-type cases.
50-53: LGTM!
55-58: LGTM!
72-75: LGTM!
77-90: LGTM!Proper distinction between missing/nil and wrong-type errors. Good consistency with
ReadString.bridges/ai/delete_chat.go (1)
62-90: Verify deleted-portal login references are still cleaned up.I don't see an equivalent to the old deleted-portal reference cleanup in this path anymore. If
DefaultChatPortalID/LastActiveRoomByAgentstill exist anywhere, deleting a chat can leave stale routing pointers behind.Run this to confirm whether those fields still exist and whether any delete-chat cleanup still clears them:
#!/bin/bash set -euo pipefail printf '== remaining login-scoped portal reference fields ==\n' rg -n -C2 'DefaultChatPortalID|LastActiveRoomByAgent' bridges/ai printf '\n== delete-chat cleanup path ==\n' sed -n '1,140p' bridges/ai/delete_chat.go printf '\n== any remaining cleanup helpers for deleted portal references ==\n' rg -n -C2 'forgetDeletedPortalReferences|DefaultChatPortalID.*=|LastActiveRoomByAgent.*=|clear.*portal.*reference' bridges/aiIf the first search still finds live fields/usages and the third search finds no cleanup, this flow still leaves stale references behind.
bridges/ai/streaming_persistence.go (1)
134-160: Nice separation of transport persistence vs. AI transcript persistence.Keeping transcript persistence best-effort here is a good fit for the live send path, and the warning logs preserve visibility when the overlay write fails.
bridges/ai/message_parts.go (1)
15-54: Good defensive lookup helpers.Returning
(nil, nil)for missing prerequisites and missing rows keeps the callers simple and lets the reaction/edit flows degrade safely.Also applies to: 56-97
pkg/aidb/001-init.sql (3)
70-73:content_hashsession tracking looks correct and cleaner.This segment is straightforward and consistent with the new state model.
183-197:aichats_internal_messageskey/index design is solid.Primary key and history index align well for portal-scoped dedupe + time-ordered retrieval.
238-251: New lookup indexes are well targeted for scoped reads.These additions match the new
(bridge_id, login_id, ...)access pattern and should help keep hot-path queries efficient.Also applies to: 272-276, 291-292
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
bridges/ai/transcript_db.go (1)
175-176: Avoid failing the full history load on one malformed row.A single bad
metadata_jsoncurrently aborts all history retrieval for this portal. Prefer skipping corrupt rows (and logging upstream) to preserve partial results.Proposed fix
- if err = json.Unmarshal([]byte(metadataRaw), &meta); err != nil { - return nil, err - } + if err = json.Unmarshal([]byte(metadataRaw), &meta); err != nil { + // TODO: log malformed row context (messageID) via scoped logger. + continue + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/transcript_db.go` around lines 175 - 176, The json.Unmarshal error for metadataRaw currently returns the error and aborts the whole history load; instead, catch json.Unmarshal errors (on metadataRaw -> meta) inside the loop in the function that reads rows (using variables metadataRaw and meta), log the parsing error and the offending metadataRaw (or row id) via the existing logger, skip that row (continue) and proceed to accumulate remaining results so one malformed metadata_json doesn't fail the entire load.
🤖 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/ai/transcript_db.go`:
- Around line 90-95: The UPSERT's DO UPDATE currently overwrites created_at_ms
on conflict which mutates original creation timestamps; modify the ON CONFLICT
... DO UPDATE SET clause in the transcript persistence SQL so it does not assign
created_at_ms (only update event_id, sender_id, metadata_json, updated_at_ms,
etc.), leaving created_at_ms immutable during updates to preserve original
chronology.
- Around line 82-85: The createdAt assignment incorrectly checks for zero by
comparing UnixMilli() == 0; instead detect zero-time using
msg.Timestamp.IsZero() before calling UnixMilli(). Change the logic around
createdAt and msg.Timestamp so you first if msg.Timestamp.IsZero() then set
createdAt = time.Now().UnixMilli(), otherwise set createdAt =
msg.Timestamp.UnixMilli(); update references to the createdAt variable
accordingly.
---
Nitpick comments:
In `@bridges/ai/transcript_db.go`:
- Around line 175-176: The json.Unmarshal error for metadataRaw currently
returns the error and aborts the whole history load; instead, catch
json.Unmarshal errors (on metadataRaw -> meta) inside the loop in the function
that reads rows (using variables metadataRaw and meta), log the parsing error
and the offending metadataRaw (or row id) via the existing logger, skip that row
(continue) and proceed to accumulate remaining results so one malformed
metadata_json doesn't fail the entire load.
🪄 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: 66a0c299-29bb-4294-b724-5289d380dc58
📒 Files selected for processing (5)
bridges/ai/custom_agents_db.gobridges/ai/login_loaders.gobridges/ai/transcript_db.gosdk/approval_flow.gosdk/approval_reaction_helpers_test.go
✅ Files skipped from review due to trivial changes (1)
- bridges/ai/custom_agents_db.go
🚧 Files skipped from review as they are similar to previous changes (1)
- bridges/ai/login_loaders.go
📜 Review details
🔇 Additional comments (1)
bridges/ai/transcript_db.go (1)
16-67: Clone helpers look robust and side-effect-safe.The cloning path for canonical turn data, metadata, and message wrappers is well-defensive and reduces shared-state mutation risk in AI history flows.
Validate and report bridge connection state and improve room meta handling. - AIClient: mark connecting, validate provider via ListModels with timeout, handle auth failures by emitting BadCredentials, and defer non-auth errors while logging; only set logged-in on success. - OpenCodeClient: emit connecting state, restore connections asynchronously, emit transient disconnect when restores fail or no instances reachable, add hasReachableOpenCodeInstance helper, and set logged-in only after successful restore. - Bridgev2 room meta handlers: validate incoming Matrix room name/topic/avatar messages, apply content to portal fields, add validateRoomMetaMessage helper, and update unit tests to supply content and assert in-memory changes. - Connector: remove hardcoded GetCapabilities override for OpenCode. - SDK: make GetCapabilities context-aware (accept ctx), call currentRoomFeatures with ctx; remove automatic Custom override in convertRoomFeatures; small doc/comment wording adjustments and minor refactors. These changes improve error reporting, make connection logic more robust, and ensure bridgev2 room metadata is validated and persisted in memory before saving.
Multiple cleanups and safety improvements across bridges and sdk: - bridges/ai: clear stale AI session routing when deleting sessions and add defensive ctx handling; use timestamp.IsZero() when persisting transcripts and remove an unused helper. - bridges/codex: apply copy-on-write for portal state updates (nextState) to avoid persisting partial state on failure and adjust thread startup flow. - bridges/openclaw: add legacy openClawLegacyLoginState and a migration helper to load/save legacy metadata into the new persisted state; stop trimming token strings when saving. - pkg/integrations/memory: ensure StopManagersForLogin is invoked before DB checks to avoid leaving managers running. - pkg/shared/toolspec: remove an unnecessary "remove": true constraint from the message schema. - pkg/agents/tools: remove several redundant parameter helper functions (ReadIntDefault, ReadBool, ReadStringSlice). - sdk: remove commands() accessors and an unused login helper to reduce API surface. These changes improve robustness (prevent partial state changes, clear stale routing), add backward compatibility for OpenClaw metadata, and simplify the codebase by removing unused/helpers.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
bridges/ai/sessions_tools.go (1)
572-581:⚠️ Potential issue | 🟠 MajorUse
messages[0]instead ofmessages[len(messages)-1].The SQL query at
bridges/ai/transcript_db.go:147orders results bycreated_at_ms DESC, meaninggetAIHistoryMessagesreturns messages with the most recent first. However,messages[len(messages)-1]accesses the last element, which is the oldest message. SincelastMessageTimestampis used to filter sessions by recent activity (line 92 checksupdatedAt < cutoff), this should return the most recent timestamp usingmessages[0].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/sessions_tools.go` around lines 572 - 581, In lastMessageTimestamp, getAIHistoryMessages returns messages ordered newest-first, so replace the oldest-element access (messages[len(messages)-1]) with the first element (messages[0]) to return the most recent timestamp; update the return expression in the AIClient.lastMessageTimestamp function to use messages[0].Timestamp.UnixMilli() and keep the existing nil/empty checks around getAIHistoryMessages.bridges/ai/handleai.go (1)
217-249:⚠️ Potential issue | 🟠 MajorDon't treat missing transcript rows as an empty chat.
This check now only consults AI transcript history plus internal prompts. Existing rooms that still have conversation history only in the bridge message table will look empty here, so auto-greetings can fire in chats that already contain user/assistant turns. Keep a bridge-history fallback when transcript history is empty, or gate this path behind transcript backfill.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handleai.go` around lines 217 - 249, The current conversation-start check (involving oc.getAIHistoryMessages, MessageMetadata and hasInternalPromptHistory) treats a missing AI transcript as empty and can trigger auto-greetings despite existing bridge-only history; modify the logic to fall back to checking bridge message history when getAIHistoryMessages returns an empty slice (or no user/assistant turns) instead of immediately returning false — e.g., after exhausting the transcript loop and before calling hasInternalPromptHistory, call the bridge-history lookup (the same table used for bridge events/messages) and only treat the chat as empty if both transcript AND bridge-history checks show no user/assistant messages; keep the existing metadata/role/exclude filters when inspecting the bridge messages so behavior remains consistent.bridges/ai/handlematrix.go (1)
428-455:⚠️ Potential issue | 🟠 MajorDon't rely on transcript-only history for edit regeneration.
oc.getAIHistoryMessagesonly readsaichats_transcript_messages. In an upgraded room where older turns still exist only in the bridge message table,editedMessagecan be found but the following assistant reply will not, soassistantResponsestays nil, the stale reply is never redacted, and regeneration posts a second answer instead of replacing the old one. Keep a bridge-history fallback here until transcript backfill is guaranteed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handlematrix.go` around lines 428 - 455, The history lookup currently relies only on oc.getAIHistoryMessages which reads aichats_transcript_messages, so when the assistant reply lives only in the bridge table the search for assistantResponse (using editedIdx and messageMeta) can miss it; update the logic in the block around oc.getAIHistoryMessages/editedIdx/assistantResponse to fall back to fetching bridge history (e.g., via an existing bridge-history reader or a new oc.getBridgeHistoryMessages function) when assistantResponse remains nil: merge or search the bridge messages with the same newest-first ordering and the same messageMeta checks to locate the assistant reply, handle errors from the fallback fetch, and keep the rest of the redaction/regeneration flow unchanged.
♻️ Duplicate comments (2)
bridges/ai/transcript_db.go (1)
82-85:⚠️ Potential issue | 🟠 MajorUse
Timestamp.IsZero()before callingUnixMilli().
time.Time{}does not yield0here, so this fallback never runs for uninitialized timestamps and transcript chronology can be skewed badly.In Go, what does time.Time{}.UnixMilli() return? Is it 0 or a negative value?Suggested fix
- createdAt := msg.Timestamp.UnixMilli() - if createdAt == 0 { - createdAt = time.Now().UnixMilli() - } + var createdAt int64 + if msg.Timestamp.IsZero() { + createdAt = time.Now().UnixMilli() + } else { + createdAt = msg.Timestamp.UnixMilli() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/transcript_db.go` around lines 82 - 85, The current logic uses msg.Timestamp.UnixMilli() and checks for zero, but time.Time{} does not produce 0 via UnixMilli so the fallback never triggers; change the code to first test msg.Timestamp.IsZero() and if true set createdAt = time.Now().UnixMilli(), otherwise set createdAt = msg.Timestamp.UnixMilli(); update the block around createdAt and msg.Timestamp in transcript_db.go (the createdAt variable assignment) accordingly.bridges/ai/login_state_db.go (1)
70-103:⚠️ Potential issue | 🟠 MajorSeed the new row from legacy login metadata on first load.
On
sql.ErrNoRows, this now returns a blankloginRuntimeState. For upgraded logins, that drops existing metadata-backed values like chat index / heartbeat seed / Gravatar state unless some separate migration has already populatedaichats_login_statebefore the first read. Please hydrate from the legacy login metadata on the no-row path, then persist the converted state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/login_state_db.go` around lines 70 - 103, When loadLoginRuntimeState hits sql.ErrNoRows, hydrate the returned loginRuntimeState from the legacy login metadata before returning and persist it into aiLoginStateTable so upgraded logins keep their existing chat index/heartbeat/gravatar state; specifically, in loadLoginRuntimeState use scope.loginID/scope.bridgeID to query the legacy metadata table (the same DB via scope.db), map legacy fields into the state struct (NextChatIndex, LastHeartbeatEventJSON, ModelCacheJSON, GravatarJSON, FileAnnotationJSON, ConsecutiveErrors, LastErrorAt) and then INSERT (or UPSERT) the populated state into aiLoginStateTable using scope.db so the converted row is stored for subsequent reads. Ensure the logic runs only on the sql.ErrNoRows branch and reuses the existing state variable before returning.
🧹 Nitpick comments (4)
bridges/ai/reaction_handling_test.go (1)
17-30: Add whitespace-trimming assertions to match the intended contract.If
portalRoomNameis expected to trim values, this test should include padded inputs (and optionally whitespace-onlyportal.Name) to lock that behavior in.🧪 Suggested test tightening
func TestPortalRoomNamePrefersBridgeOwnedName(t *testing.T) { portal := &bridgev2.Portal{ Portal: &database.Portal{ PortalKey: networkid.PortalKey{ID: networkid.PortalID("chat-1")}, MXID: id.RoomID("!chat:example.com"), - Name: "Bridge Name", + Name: " Bridge Name ", Metadata: &PortalMetadata{ - Slug: "sidecar-slug", + Slug: " sidecar-slug ", }, }, } if got := portalRoomName(portal); got != "Bridge Name" { t.Fatalf("expected bridge-owned room name, got %q", got) } - portal.Name = "" + portal.Name = " " if got := portalRoomName(portal); got != "sidecar-slug" { t.Fatalf("expected slug fallback when bridge name is empty, got %q", got) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/reaction_handling_test.go` around lines 17 - 30, The test should assert that portalRoomName trims whitespace: update the test for portalRoomName to include cases where portal.Name contains leading/trailing spaces (e.g., " Bridge Name ") and where portal.Name is whitespace-only (e.g., " ") so the function returns the trimmed name or falls back to PortalMetadata.Slug respectively; specifically modify the existing assertions that check portal.Name == "Bridge Name" and portal.Name == "" to instead use padded inputs and a whitespace-only input and assert portalRoomName(portal) returns "Bridge Name" and "sidecar-slug" (from PortalMetadata.Slug) accordingly, referencing portalRoomName, portal.Name and PortalMetadata.Slug to locate the code.bridges/ai/internal_prompt_db.go (1)
83-101: Dynamic SQL construction is safe but consider query readability.The parameter index computation via
strconv.Itoa(len(args))is safe since the values are controlled integers. However, the query string concatenation across multiple conditionals makes the final SQL harder to reason about.This is acceptable for now given the query's simplicity, but consider using a query builder or
strings.Builderwith explicit parameter tracking for more complex queries in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/internal_prompt_db.go` around lines 83 - 101, The dynamic SQL is built by concatenating strings and computing parameter indices with strconv.Itoa(len(args)) which is safe but reduces readability; refactor the construction in the function that builds the query (around aiInternalMessagesTable, args, resetAt, opts.excludeMessageID and the scope.db.Query call) to make the parameter tracking explicit—use a strings.Builder or a local slice of condition strings and a separate paramIndex counter so each conditional (resetAt, excludedEventID) appends a condition like "AND created_at_ms >= $N" using the incremented counter and pushes the value to args; keep the final ORDER BY/LIMIT assembly the same and ensure args order matches the $N placeholders passed to scope.db.Query.bridges/ai/streaming_persistence.go (1)
120-164: Consider extracting the duplicated SenderID computation.The sender ID logic at lines 125-130 and 145-150 is identical. Extracting it to a local variable would reduce duplication.
Also note:
time.UnixMilli(0)returns the Unix epoch (Jan 1, 1970), which is not zero but would be semantically invalid. The.IsZero()check at line 157 will be false for epoch time, so the fallback totime.Now()won't trigger forcompletedAtMs == 0after the else branch sets it.♻️ Suggested refactor
+ senderID := func() networkid.UserID { + if state.respondingGhostID != "" { + return networkid.UserID(state.respondingGhostID) + } + return modelUserID(oc.effectiveModel(meta)) + }() + sdk.UpsertAssistantMessage(ctx, sdk.UpsertAssistantMessageParams{ Login: oc.UserLogin, Portal: portal, - SenderID: func() networkid.UserID { - if state.respondingGhostID != "" { - return networkid.UserID(state.respondingGhostID) - } - return modelUserID(oc.effectiveModel(meta)) - }(), + SenderID: senderID, NetworkMessageID: networkMessageID, ... }) ... if messageID != "" && portal != nil { transcriptMsg := &database.Message{ ID: messageID, MXID: initialEventID, Room: portal.PortalKey, - SenderID: func() networkid.UserID { - if state.respondingGhostID != "" { - return networkid.UserID(state.respondingGhostID) - } - return modelUserID(oc.effectiveModel(meta)) - }(), + SenderID: senderID,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/streaming_persistence.go` around lines 120 - 164, Extract the duplicated SenderID computation into a local variable (e.g., senderID := func() networkid.UserID { if state.respondingGhostID != "" { return networkid.UserID(state.respondingGhostID) } return modelUserID(oc.effectiveModel(meta)) }()) and use senderID both in sdk.UpsertAssistantMessage (SenderID) and in transcriptMsg.SenderID to remove duplication. Also fix the timestamp logic around state.completedAtMs: set transcriptMsg.Timestamp = time.Now() when state.completedAtMs == 0, otherwise compute t := time.UnixMilli(state.completedAtMs) and if t.IsZero() fall back to time.Now(); assign the resulting time to transcriptMsg.Timestamp to avoid treating Unix epoch as valid when completedAtMs is zero.bridges/ai/chat.go (1)
1165-1205: Consider usingGetExistingPortalByKeyto avoid side effects.
GetPortalByKeymay create a portal if one doesn't exist for the given key. Since this function is listing existing chat portals from the AI state table, usingGetExistingPortalByKeywould be more semantically correct and avoid unintended portal creation.♻️ Suggested change
- portal, err := oc.UserLogin.Bridge.GetPortalByKey(ctx, networkid.PortalKey{ + portal, err := oc.UserLogin.Bridge.GetExistingPortalByKey(ctx, networkid.PortalKey{ ID: networkid.PortalID(portalID), Receiver: oc.UserLogin.ID, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/chat.go` around lines 1165 - 1205, The listAllChatPortals method uses oc.UserLogin.Bridge.GetPortalByKey which can create portals as a side effect; replace that call with oc.UserLogin.Bridge.GetExistingPortalByKey so you only fetch already-existing portals, and keep the current nil-check and error handling (i.e., if GetExistingPortalByKey returns (nil, nil) just skip, and if it returns an error propagate it). Update the reference at the call site in AIClient.listAllChatPortals that currently constructs networkid.PortalKey from portalID and oc.UserLogin.ID to call GetExistingPortalByKey instead.
🤖 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/ai/handlematrix.go`:
- Around line 956-967: savePortal currently calls saveAIPortalState before
portal.Save which can leave AI flags persisted if portal.Save fails; either flip
the order or, preferably, perform both writes inside the same transactional
boundary so they succeed or roll back together. Modify savePortal to begin a
DB/transactional context, call portal.Save(ctx) and saveAIPortalState(ctx,
portal, meta) within that transaction (or call portal.Save first then
saveAIPortalState if transaction support is unavailable), and ensure rollback on
any error (or revert the earlier write) so callers (auto-greeting/title
generation) don't see inconsistent AI flags; refer to savePortal,
saveAIPortalState, and portal.Save when locating the change.
In `@bridges/ai/login_state_db.go`:
- Around line 200-205: The current ensureLoginStateLoaded code assigns a new
empty &loginRuntimeState{} to oc.loginState when loadLoginRuntimeState fails,
which causes later successful updates to persist that empty snapshot; instead,
when loadLoginRuntimeState(ctx, oc) returns an error simply log the error and
return a temporary empty state without memoizing it (do not set oc.loginState).
Change the error branch in ensureLoginStateLoaded (the loadLoginRuntimeState
call) to create a local temp := &loginRuntimeState{} and return it (or continue
using it) while only assigning oc.loginState = state when loadLoginRuntimeState
succeeded; keep existing logging behavior via oc.loggerForContext(ctx).
In `@bridges/ai/transcript_db.go`:
- Around line 207-211: The helper getAllAIHistoryMessages is unused and causing
a staticcheck U1000 failure; either remove the function entirely or replace its
references by calling oc.getAIHistoryMessages(ctx, portal, 0) where intended; if
you remove it, also remove any tests or comments that reference
getAllAIHistoryMessages; if you prefer to keep it, add a single call site (or an
explicit //nolint:staticcheck comment) to avoid the unused symbol—refer to the
getAllAIHistoryMessages and getAIHistoryMessages identifiers when making the
change.
In `@bridges/codex/constructors.go`:
- Around line 52-60: The code currently returns nil when cc.bridgeDB() is nil,
causing silent success and skipping aidb.EnsureSchema, cc.applyRuntimeDefaults,
and sdk.PrimeUserLoginCache; change that to fail fast by replacing the early
return with an explicit error (or remove the check and let aidb.EnsureSchema
report the nil DB error). Concretely, in the block using cc.bridgeDB(), ensure
that when db == nil you return a descriptive error (e.g., "AI DB not available")
so aidb.EnsureSchema(ctx, db), cc.applyRuntimeDefaults(), and
sdk.PrimeUserLoginCache(ctx, cc.br) always run only on successful
initialization.
In `@bridges/codex/portal_state_db.go`:
- Around line 146-149: The code silently ignores json.Unmarshal errors for
stateRaw which can drop portals; update the error handling around
json.Unmarshal([]byte(stateRaw), state) so failures are surfaced with context
(include the portal key/identifier and the raw error) instead of just continue —
e.g., log a warning or return an error using the function’s logger/err path
(reference symbols: stateRaw, state, json.Unmarshal) so bad state_json rows are
visible during backfill/list operations.
- Around line 78-92: The function loadCodexPortalState hides the "no state" case
by returning a non-nil empty *codexPortalState, breaking callers that rely on
nil checks; change the behavior so that when scope == nil or when aidb.Load
returns nil state you return (nil, nil) instead of &codexPortalState{}; update
the return paths in loadCodexPortalState (which calls codexDBScopeForPortal,
uses codexPortalStateBlob and aidb.Load[codexPortalState]) to propagate a nil
pointer for "no state" so callers can detect it via state == nil.
- Around line 159-172: parseCodexPortalKey currently splits on the first "/"
which is brittle if PortalKey.String() allows "/" inside components; replace the
manual parsing with the canonical networkid parser (use
networkid.ParsePortalKey) to guarantee round-trip safety: call
networkid.ParsePortalKey on the trimmed raw string, handle the returned
(PortalKey, error) similarly to the current behavior (return empty PortalKey +
false on empty input or parse error, otherwise return the parsed
networkid.PortalKey and true), and remove the manual
strings.Cut/PortalID/UserLoginID construction in parseCodexPortalKey.
In `@bridges/openclaw/identifiers.go`:
- Around line 34-42: The generated ghost ID can contain an empty encoded loginID
which parseOpenClawGhostID rejects; update openClawScopedGhostUserID to
canonicalize loginID the same way it does agentID (e.g., set a default like
"gateway" when loginID == ""), so the produced string matches what
parseOpenClawGhostID expects; modify openClawScopedGhostUserID (and its use of
loginID before base64 encoding) to substitute that placeholder for empty values
to ensure round‑trippable IDs.
In `@bridges/openclaw/login.go`:
- Around line 244-276: Create the login first without starting auto-connect,
persist the credentials, then kick off the load/connect step: replace the single
call to sdk.CreateAndCompleteLogin with a two-step flow—use the create-only API
to obtain the login (so CreateAndCompleteLogin does not start
LoadConnectAndCompleteLogin), call saveOpenClawLoginState(login,
&openClawPersistedLoginState{...}) to persist
GatewayToken/GatewayPassword/DeviceToken, and only after a successful save
invoke the completion/load routine (LoadConnectAndCompleteLogin or the SDK's
complete-login entry) to start auto-connect; update error handling and rollback
accordingly around those symbols (sdk.CreateAndCompleteLogin,
saveOpenClawLoginState, openClawPersistedLoginState,
LoadConnectAndCompleteLogin).
---
Outside diff comments:
In `@bridges/ai/handleai.go`:
- Around line 217-249: The current conversation-start check (involving
oc.getAIHistoryMessages, MessageMetadata and hasInternalPromptHistory) treats a
missing AI transcript as empty and can trigger auto-greetings despite existing
bridge-only history; modify the logic to fall back to checking bridge message
history when getAIHistoryMessages returns an empty slice (or no user/assistant
turns) instead of immediately returning false — e.g., after exhausting the
transcript loop and before calling hasInternalPromptHistory, call the
bridge-history lookup (the same table used for bridge events/messages) and only
treat the chat as empty if both transcript AND bridge-history checks show no
user/assistant messages; keep the existing metadata/role/exclude filters when
inspecting the bridge messages so behavior remains consistent.
In `@bridges/ai/handlematrix.go`:
- Around line 428-455: The history lookup currently relies only on
oc.getAIHistoryMessages which reads aichats_transcript_messages, so when the
assistant reply lives only in the bridge table the search for assistantResponse
(using editedIdx and messageMeta) can miss it; update the logic in the block
around oc.getAIHistoryMessages/editedIdx/assistantResponse to fall back to
fetching bridge history (e.g., via an existing bridge-history reader or a new
oc.getBridgeHistoryMessages function) when assistantResponse remains nil: merge
or search the bridge messages with the same newest-first ordering and the same
messageMeta checks to locate the assistant reply, handle errors from the
fallback fetch, and keep the rest of the redaction/regeneration flow unchanged.
In `@bridges/ai/sessions_tools.go`:
- Around line 572-581: In lastMessageTimestamp, getAIHistoryMessages returns
messages ordered newest-first, so replace the oldest-element access
(messages[len(messages)-1]) with the first element (messages[0]) to return the
most recent timestamp; update the return expression in the
AIClient.lastMessageTimestamp function to use messages[0].Timestamp.UnixMilli()
and keep the existing nil/empty checks around getAIHistoryMessages.
---
Duplicate comments:
In `@bridges/ai/login_state_db.go`:
- Around line 70-103: When loadLoginRuntimeState hits sql.ErrNoRows, hydrate the
returned loginRuntimeState from the legacy login metadata before returning and
persist it into aiLoginStateTable so upgraded logins keep their existing chat
index/heartbeat/gravatar state; specifically, in loadLoginRuntimeState use
scope.loginID/scope.bridgeID to query the legacy metadata table (the same DB via
scope.db), map legacy fields into the state struct (NextChatIndex,
LastHeartbeatEventJSON, ModelCacheJSON, GravatarJSON, FileAnnotationJSON,
ConsecutiveErrors, LastErrorAt) and then INSERT (or UPSERT) the populated state
into aiLoginStateTable using scope.db so the converted row is stored for
subsequent reads. Ensure the logic runs only on the sql.ErrNoRows branch and
reuses the existing state variable before returning.
In `@bridges/ai/transcript_db.go`:
- Around line 82-85: The current logic uses msg.Timestamp.UnixMilli() and checks
for zero, but time.Time{} does not produce 0 via UnixMilli so the fallback never
triggers; change the code to first test msg.Timestamp.IsZero() and if true set
createdAt = time.Now().UnixMilli(), otherwise set createdAt =
msg.Timestamp.UnixMilli(); update the block around createdAt and msg.Timestamp
in transcript_db.go (the createdAt variable assignment) accordingly.
---
Nitpick comments:
In `@bridges/ai/chat.go`:
- Around line 1165-1205: The listAllChatPortals method uses
oc.UserLogin.Bridge.GetPortalByKey which can create portals as a side effect;
replace that call with oc.UserLogin.Bridge.GetExistingPortalByKey so you only
fetch already-existing portals, and keep the current nil-check and error
handling (i.e., if GetExistingPortalByKey returns (nil, nil) just skip, and if
it returns an error propagate it). Update the reference at the call site in
AIClient.listAllChatPortals that currently constructs networkid.PortalKey from
portalID and oc.UserLogin.ID to call GetExistingPortalByKey instead.
In `@bridges/ai/internal_prompt_db.go`:
- Around line 83-101: The dynamic SQL is built by concatenating strings and
computing parameter indices with strconv.Itoa(len(args)) which is safe but
reduces readability; refactor the construction in the function that builds the
query (around aiInternalMessagesTable, args, resetAt, opts.excludeMessageID and
the scope.db.Query call) to make the parameter tracking explicit—use a
strings.Builder or a local slice of condition strings and a separate paramIndex
counter so each conditional (resetAt, excludedEventID) appends a condition like
"AND created_at_ms >= $N" using the incremented counter and pushes the value to
args; keep the final ORDER BY/LIMIT assembly the same and ensure args order
matches the $N placeholders passed to scope.db.Query.
In `@bridges/ai/reaction_handling_test.go`:
- Around line 17-30: The test should assert that portalRoomName trims
whitespace: update the test for portalRoomName to include cases where
portal.Name contains leading/trailing spaces (e.g., " Bridge Name ") and where
portal.Name is whitespace-only (e.g., " ") so the function returns the trimmed
name or falls back to PortalMetadata.Slug respectively; specifically modify the
existing assertions that check portal.Name == "Bridge Name" and portal.Name ==
"" to instead use padded inputs and a whitespace-only input and assert
portalRoomName(portal) returns "Bridge Name" and "sidecar-slug" (from
PortalMetadata.Slug) accordingly, referencing portalRoomName, portal.Name and
PortalMetadata.Slug to locate the code.
In `@bridges/ai/streaming_persistence.go`:
- Around line 120-164: Extract the duplicated SenderID computation into a local
variable (e.g., senderID := func() networkid.UserID { if state.respondingGhostID
!= "" { return networkid.UserID(state.respondingGhostID) } return
modelUserID(oc.effectiveModel(meta)) }()) and use senderID both in
sdk.UpsertAssistantMessage (SenderID) and in transcriptMsg.SenderID to remove
duplication. Also fix the timestamp logic around state.completedAtMs: set
transcriptMsg.Timestamp = time.Now() when state.completedAtMs == 0, otherwise
compute t := time.UnixMilli(state.completedAtMs) and if t.IsZero() fall back to
time.Now(); assign the resulting time to transcriptMsg.Timestamp to avoid
treating Unix epoch as valid when completedAtMs is zero.
🪄 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: 32c700c9-ca86-44de-87aa-fe2887bfd192
📒 Files selected for processing (90)
bridges/ai/account_hints.gobridges/ai/agentstore.gobridges/ai/bridge_db.gobridges/ai/chat.gobridges/ai/chat_fork_test.gobridges/ai/client.gobridges/ai/command_registry.gobridges/ai/custom_agents_db.gobridges/ai/desktop_api_sessions.gobridges/ai/desktop_api_sessions_test.gobridges/ai/error_logging.gobridges/ai/events.gobridges/ai/events_test.gobridges/ai/gravatar.gobridges/ai/handleai.gobridges/ai/handlematrix.gobridges/ai/handler_interfaces.gobridges/ai/integration_host.gobridges/ai/internal_prompt_db.gobridges/ai/login.gobridges/ai/login_loaders.gobridges/ai/login_loaders_test.gobridges/ai/login_state_db.gobridges/ai/logout_cleanup.gobridges/ai/logout_cleanup_test.gobridges/ai/media_understanding_runner.gobridges/ai/media_understanding_runner_openai_test.gobridges/ai/message_parts.gobridges/ai/metadata.gobridges/ai/metadata_test.gobridges/ai/persistence_boundaries_test.gobridges/ai/portal_materialize.gobridges/ai/portal_state_db.gobridges/ai/prompt_builder.gobridges/ai/reaction_handling.gobridges/ai/reaction_handling_test.gobridges/ai/room_info.gobridges/ai/room_meta_bridgev2_test.gobridges/ai/scheduler_heartbeat_test.gobridges/ai/scheduler_rooms.gobridges/ai/sessions_tools.gobridges/ai/sessions_visibility_test.gobridges/ai/status_text.gobridges/ai/streaming_output_handlers.gobridges/ai/streaming_persistence.gobridges/ai/subagent_spawn.gobridges/ai/tool_approvals.gobridges/ai/tool_approvals_rules.gobridges/ai/tool_configured.gobridges/ai/tools.gobridges/ai/tools_message_actions.gobridges/ai/tools_message_desktop.gobridges/ai/transcript_db.gobridges/codex/constructors.gobridges/codex/directory_manager.gobridges/codex/portal_state_db.gobridges/openclaw/catalog.gobridges/openclaw/identifiers.gobridges/openclaw/login.gobridges/openclaw/manager.gobridges/openclaw/metadata.gobridges/openclaw/provisioning.gobridges/opencode/client.gobridges/opencode/connector.gobridges/opencode/opencode_instance_state.gocmd/agentremote/main.gocmd/agentremote/profile.gocmd/agentremote/profile_test.gocmd/agentremote/run_bridge.gocmd/internal/bridgeentry/bridgeentry.gopkg/agents/tools/params.gopkg/aidb/001-init.sqlpkg/aidb/json_blob_table.gopkg/integrations/memory/sessions.gopkg/matrixevents/matrixevents.gopkg/shared/toolspec/toolspec.gosdk/approval_flow.gosdk/approval_reaction_helpers.gosdk/approval_reaction_helpers_test.gosdk/client.gosdk/commands.gosdk/conversation.gosdk/conversation_state.gosdk/conversation_test.gosdk/helpers.gosdk/load_user_login.gosdk/login_handle.gosdk/matrix_actions.gosdk/portal_lifecycle.gosdk/room_features.go
💤 Files with no reviewable changes (5)
- bridges/ai/error_logging.go
- bridges/ai/chat_fork_test.go
- bridges/ai/events_test.go
- bridges/ai/events.go
- bridges/ai/command_registry.go
✅ Files skipped from review due to trivial changes (4)
- bridges/ai/media_understanding_runner.go
- bridges/ai/media_understanding_runner_openai_test.go
- bridges/ai/persistence_boundaries_test.go
- bridges/ai/custom_agents_db.go
🚧 Files skipped from review as they are similar to previous changes (22)
- bridges/ai/gravatar.go
- bridges/ai/portal_materialize.go
- bridges/ai/desktop_api_sessions_test.go
- bridges/ai/tool_configured.go
- bridges/ai/login_loaders_test.go
- bridges/ai/scheduler_rooms.go
- bridges/ai/scheduler_heartbeat_test.go
- bridges/ai/desktop_api_sessions.go
- bridges/ai/tool_approvals.go
- bridges/openclaw/catalog.go
- bridges/ai/message_parts.go
- bridges/codex/directory_manager.go
- bridges/opencode/connector.go
- bridges/ai/reaction_handling.go
- bridges/openclaw/metadata.go
- bridges/ai/integration_host.go
- bridges/openclaw/manager.go
- bridges/ai/agentstore.go
- bridges/ai/login_loaders.go
- bridges/ai/prompt_builder.go
- bridges/ai/login.go
- bridges/openclaw/provisioning.go
📜 Review details
🧰 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/ai/room_info.gobridges/ai/reaction_handling_test.gobridges/codex/constructors.gobridges/ai/handleai.gobridges/ai/client.go
🪛 GitHub Actions: Go
bridges/ai/transcript_db.go
[error] 207-207: go-staticcheck-repo-mod failed (staticcheck U1000): func (*AIClient).getAllAIHistoryMessages is unused
🪛 GitHub Check: Lint
bridges/ai/transcript_db.go
[failure] 207-207:
func (*AIClient).getAllAIHistoryMessages is unused (U1000)
🔇 Additional comments (84)
bridges/ai/account_hints.go (2)
37-37: Context-aware instance discovery is a good fix.This keeps desktop instance resolution aligned with the caller’s cancellation/deadline semantics and the rest of this method’s context usage.
65-65: Per-instance config lookup now uses consistent context propagation.Using the same
ctxfor config resolution and account listing avoids mixed-context behavior in hint generation.bridges/ai/reaction_handling_test.go (1)
12-31: Good coverage for precedence + fallback behavior.This test cleanly validates both branches (bridge-owned name first, slug fallback when name is empty). Nice targeted regression guard.
bridges/opencode/client.go (8)
12-12: LGTM! SDK package migration for imports and type definitions.The import and type migrations from
agentremotetosdkare consistent. TheClientBase,StreamTurnHost,Turn, andStreamPartStatetypes are all properly sourced from the newsdkpackage.Also applies to: 26-26, 31-31, 38-39
67-74: LGTM! Stream host initialization properly migrated to SDK types.The
NewStreamTurnHostandStreamTurnHostCallbackscorrectly use thesdkpackage. TheGetAbortercallback properly returnssdk.Aborterand thes.turn(of type*sdk.Turn) should satisfy this interface.
88-117: LGTM! Improved connection state management.The refactored
Connect()method now properly:
- Sets
LoggedIn(false)and emitsStateConnectingbefore attempting connection- Handles
RestoreConnectionsfailure by emittingTransientDisconnectinstead of silently continuing- Adds reachability validation via
hasReachableOpenCodeInstance()before declaring connected state- Only transitions to
Connectedwhen instances are actually reachableThe explicit
returnon line 113 correctly prevents fall-through to the synchronous connected state when the async path is taken.
137-151: LGTM! Reachability check logic is sound.The
hasReachableOpenCodeInstance()method correctly:
- Returns
truewhen no instances are configured (vacuously connected)- Returns
falsewith proper nil guards for missing bridge/manager- Returns
trueif at least one configured instance is connected- Returns
falseonly when instances exist but none are reachable
153-158: LGTM! Return type correctly migrated tosdk.ApprovalReactionHandler.The return type change aligns with the
ReactionTargetinterface defined insdk/base_reaction_handler.go, which expectsGetApprovalHandler()to returnApprovalReactionHandler. This is consistent with the broader SDK migration.
200-214: LGTM! Room features construction migrated to SDK builders.The use of
sdk.BuildRoomFeaturesandsdk.BuildMediaFileFeatureMapis consistent with the SDK migration pattern. The parameter structure is clear and the closure pattern for providing file features is appropriate.
231-236: LGTM! Client cache removal migrated to SDK helper.The
sdk.RemoveClientFromCacheusage is consistent with the SDK migration. The nil checks forconnectorandUserLoginbefore the call provide appropriate guards.
238-247: LGTM! Chat info construction migrated to SDK builder.The
sdk.BuildChatInfoWithFallbackprovides a clean abstraction for building chat info with fallback values. The parameters (primary title, fallback name, service identifier, topic) are appropriately passed.bridges/codex/constructors.go (1)
84-94: Nice capability declaration.Explicitly advertising the provisioning surface here is clearer than relying on implicit behavior and should help bridgev2 clients gate features correctly.
bridges/codex/portal_state_db.go (1)
44-76: Scope construction guards are solid.Nil checks plus trimming of bridge/login/portal identifiers are a good defensive boundary before DB access.
bridges/openclaw/identifiers.go (2)
52-91: Good parser hardening for v1 ghost IDs.The versioned prefix + Base64URL split/decode flow is a solid fix for the prior delimiter ambiguity and keeps legacy parsing isolated.
19-19: Clarify thatopenClawGatewayID()is new code with no backwards compatibility concerns.This function is newly introduced in
bridges/openclaw/identifiers.goand has no prior implementation to break compatibility with.stringutil.ShortHash(key, 8)correctly returns 16 hex characters (2*8 bytes), as documented. Since this is new code, there are no existing persisted gateway IDs with a different format to orphan. The gateway ID is deterministically recomputed from stablegatewayURLandlabelinputs, ensuring consistent format going forward.> Likely an incorrect or invalid review comment.bridges/ai/sessions_visibility_test.go (1)
25-27: LGTM!The test fixture correctly uses
Sluginstead ofTitle, aligning with the broader migration to derive room display logic fromPortalMetadata.Slug.bridges/ai/tools_message_desktop.go (1)
14-18: LGTM!The context threading through
resolveDesktopInstanceand all its call sites is correct. This enables proper cancellation and timeout propagation for desktop API operations.bridges/ai/internal_prompt_db.go (1)
118-130: LGTM!The malformed
canonical_turn_datahandling correctly logs a warning with contextual identifiers (event_id,portal_id) before skipping the row, as previously requested. This makes corrupted rows debuggable without failing the entire load operation.bridges/ai/subagent_spawn.go (1)
325-335: LGTM!The migration from direct
database.Messagecreation topersistInternalPromptcorrectly uses the new shared internal-prompt persistence system. The error handling appropriately logs and continues rather than failing the subagent spawn operation.bridges/ai/metadata.go (3)
95-117: LGTM!The simplified
UserLoginMetadata(now only storingProvider) and the credential accessor functions operating on*aiLoginConfigcorrectly align with the migration of login state to AI-owned sidecar tables.
210-244: LGTM!All persistent portal state fields are correctly marked with
json:"-"to prevent bridgev2 metadata serialization. The runtime-onlyportalStateLoadedflag enables the load-once semantics implemented inportal_state_db.go.
309-312: LGTM!The deep copy of
TypingIntervalSecondscorrectly dereferences and creates a new pointer, preventing aliasing between the original and cloned metadata.bridges/ai/portal_state_db.go (2)
124-132: LGTM!The error handling correctly separates
sql.ErrNoRows(return nil, nil) from other database errors (return nil, err) and the empty string check. This addresses the previous review concern about swallowing database errors.
163-178: LGTM!The load-once pattern with
portalStateLoadedflag prevents redundant database queries. The error handling correctly resets the flag on failure, allowing retry on subsequent access, while logging the error for debugging.bridges/ai/metadata_test.go (2)
49-68: LGTM!Good test coverage for the JSON serialization behavior. This test ensures the
json:"-"tags are working correctly and persistent state won't accidentally leak through bridgev2 metadata serialization.
70-122: LGTM!Comprehensive round-trip test covering the key persistence fields including
PDFConfig,SessionBootstrapByAgent,ModuleMeta, andTypingIntervalSeconds. This validates thepersistedPortalStateFromMeta→ marshal → unmarshal →applyPersistedPortalStateflow.bridges/ai/status_text.go (1)
203-220: LGTM!The iteration order fix correctly finds the most recent assistant message with token usage. The forward iteration (index 0 first) works correctly with
getAIHistoryMessageswhich returns messages in newest-first order, as confirmed by the past review analysis.bridges/ai/logout_cleanup.go (4)
42-93: Fire-and-forget cleanup behavior retained.The previous review flagged that each
DELETEis still fire-and-forget with only warning logs on failure. This design choice prioritizes logout completion over strict cleanup guarantees.
12-19: LGTM!Clear documentation of what
purgeLoginDatahandles vs. what bridgev2 handles automatically.
94-99: LGTM!The in-memory state cleanup correctly clears both
loginStateandloginConfigafter the DB cleanup, preventing stale state from persisting in the running client.
102-112: LGTM!The
execDeletehelper appropriately handles nil context by defaulting tocontext.Background()and logs warnings without failing the overall logout flow.bridges/ai/streaming_output_handlers.go (4)
14-15: LGTM!Import changes correctly migrate from the old
agentremotepackage to the consolidatedsdkpackage, and add thestringutilimport for the new hash function.
18-21: LGTM!The approval ID generation is simplified using
stringutil.ShortHashwith a length of 8 characters. The input construction (trimmed toolCallID, toolName, and input joined by newlines) maintains stable, deterministic IDs.
29-40: LGTM!Return type change to
sdk.ApprovalHandleand usage ofsdk.ApprovalReasonAutoApprovedcorrectly aligns with the SDK migration. The caller instreaming_actions.goexpects this type for thehandlefield.
240-240: LGTM!The
ctxparameter is now correctly passed toisMcpAlwaysAllowed, matching the updated function signature intool_approvals_rules.gothat requires context as the first argument.bridges/ai/room_info.go (1)
11-30: LGTM!The helper correctly implements the two-path room name update pattern:
- For established rooms (MXID set): uses
portal.UpdateInfowithExcludeChangesFromTimeline: true- For pre-creation: sets local
portal.Nameandportal.NameSetfieldsThis matches the SDK pattern shown in
sdk/matrix_actions.go.bridges/ai/sessions_tools.go (6)
119-126: LGTM!Migration to
getAIHistoryMessagescentralizes history retrieval. The slicing logicopenClawMessages[len(openClawMessages)-messageLimit:]correctly takes the last N messages afterbuildOpenClawSessionMessagesprocesses them.
138-138: LGTM!Context is now correctly threaded through to
desktopAPIInstanceNames.
217-222: LGTM!Context threading through
resolveDesktopInstanceNameanddesktopAPIClientaligns with the updated function signatures.
270-270: LGTM!Correctly migrated to use the centralized
getAIHistoryMessageshelper.
312-312: LGTM!Context threading for desktop session/instance resolution is consistent with other callers.
Also applies to: 355-355
473-486: LGTM!Session label and display name resolution now prefer
meta.Slugover the previousmeta.Title. The fallback chain remains correct: portal.Name → meta.Slug.bridges/ai/logout_cleanup_test.go (1)
8-42: LGTM!Good test coverage for the run-key table cleanup. The test properly:
- Sets up a DB-backed client
- Inserts test rows into both tables
- Calls
purgeLoginData- Verifies both tables are empty for the target login
Consider adding a test case that verifies data belonging to other logins remains untouched to prevent accidental over-deletion regressions.
bridges/ai/streaming_persistence.go (2)
39-41: LGTM!SDK migration for
GeneratedFileRefsFromPartsis correct.
55-77: LGTM!Migration to
sdk.BuildAssistantBaseMetadataandsdk.AssistantMessageMetadatacorrectly uses the SDK types for metadata construction.bridges/ai/bridge_db.go (4)
13-27: LGTM!Centralizing table name constants provides a single source of truth and reduces the risk of typos across the codebase.
35-35: LGTM!Logger label change from
"agentremote"to"ai"aligns with the broader rename in this PR.
83-88: LGTM!New helper provides portal-based DB access with proper nil-checking.
100-160: LGTM!Well-designed scope abstractions that:
- Centralize ID normalization with
strings.TrimSpace(addressing prior review feedback)- Provide consistent nil-safety checks
- Enable code reuse across the various
*_db.gofilesThe
portalScopeembedding*loginScopeis clean and avoids field duplication.bridges/ai/tools_message_actions.go (2)
27-39: Behavior change: explicit emoji now required.The "remove all reactions" functionality (previously triggered by empty
emoji) has been removed. This is an intentional behavior change per the summary, but note thatremoved: 1is now hardcoded rather than reflecting actual removal count.If
removeReactioncan fail silently for non-existent reactions, theremoved: 1may be misleading. Consider returning the actual result from the underlying call if available.
88-88: LGTM!Context is now correctly passed to
desktopAPIInstanceConfig, matching the updated function signature.bridges/ai/chat.go (14)
13-13: LGTM!Clean import migration from the aliased
agentremotestyle to directsdkpackage usage.
50-62: LGTM!The agent enablement logic correctly reads from persisted
aiLoginConfigwith a safe nil-defaulting pattern (cfg.Agents == nil || *cfg.Agentstreats unset as enabled).
127-132: LGTM!API key resolution now correctly flows through the persisted login config via
resolveProviderAPIKeyForConfig.
194-280: LGTM!Clean type migration to
*sdk.Agentfor agent matching, contact response, and catalog ID resolution. The field accesses remain consistent with the SDK type's interface.
630-630: LGTM!Using
savePortalQuietcorrectly persists agent metadata (includingResolvedTargetandRuntimeModelOverride) to the AI-owned state table while gracefully handling persistence failures.
674-683: LGTM!The chat index allocation now correctly persists through the AI-local runtime state via
updateLoginState, maintaining thread safety with the existingchatLockmutex.
693-705: LGTM!Intentionally omitting
Titlefrom forked portal metadata clone allows forked chats to receive fresh titles rather than inheriting from the source.
741-764: LGTM!Portal state is correctly persisted to the AI-owned state table before calling
sdk.ConfigureDMPortal. The error propagation is appropriate for initialization failures.
949-956: LGTM!Improved title resolution with proper trimming and a cleaner fallback chain (portal name → slug → model contact name).
990-999: LGTM!Clean abstraction using
sdk.BuildLoginDMChatInfofor DM chat info construction while still allowing model-specific member customization.
1066-1068: LGTM!System notice sending now uses the SDK helper
sdk.SendSystemMessagefor consistency across the codebase.
1080-1092: LGTM!Bootstrap simplified to focus on config-driven default chat creation, removing unnecessary persistence waits and counter sync operations.
1094-1163: LGTM!The refactored
ensureDefaultChatuses deterministic portal key lookup, cleanly separates portal existence checking from room materialization, and appropriately errors when the required "beep agent" is unavailable.
1235-1244: LGTM!Appropriately simplified to delegate message cleanup to the bridgev2 framework, keeping only the debug log for observability.
bridges/ai/client.go (19)
4-4: LGTM!Clean import additions:
cmppackage for comparison operations and directsdkpackage import.Also applies to: 32-32
264-346: LGTM!Clean structural migration to SDK types with properly separated mutexes for login state and config, and type-safe generic approval flow.
382-462: LGTM!Well-structured constructor with defensive config cloning, properly configured approval flow callbacks, and clean migration to DB-backed state loading.
469-471: LGTM!Return type correctly updated to
sdk.ApprovalReactionHandlerinterface.
486-519: LGTM!Clean refactor with
initProviderForLoginConfigenabling config-driven provider initialization while maintaining backward compatibility through the wrapperinitProviderForLogin.
614-618: LGTM!Correctly using
sdk.MatrixMessageStatusEventInfohelper for consistent message status event construction.
621-645: LGTM!Clean dual persistence pattern: the bridge DB gets a transport-safe message copy (with cleared metadata), while the AI-owned transcript table stores the full payload. Error handling appropriately logs auxiliary persistence failures without failing the primary operation.
852-897: LGTM!Connect now proactively validates provider authentication before marking logged in, with proper bridge state updates for auth failures and graceful degradation for transient issues.
910-912: LGTM!Properly stops the scheduler on disconnect with a nil guard.
966-978: LGTM!Logout cleanup centralized through
purgeLoginDatafor consistent AI-owned state purging.
992-995: LGTM!Clean use of
sdk.BuildChatInfoWithFallbackwithcmp.Orfor concise fallback chain.
1259-1287: LGTM!Profile prompt supplement correctly reads from persisted login config via
loginConfigSnapshot.
1528-1532: LGTM!Clean provider type check using the
loginMetadatahelper.
1648-1682: LGTM!Model cache management correctly migrated to use
loginStateSnapshot/updateLoginStatewith proper initialization and atomic updates.
1684-1695: LGTM!Model info lookup correctly uses
loginStateSnapshotfor cache access.
1706-1747: LGTM!Generated files metadata correctly persisted to the AI-owned transcript table with proper fallback handling when the transcript message doesn't exist yet.
1749-1754: LGTM!Added
limitfield tohistoryLoadResultstruct for tracking history load limits.
2046-2048: LGTM!Logging helper correctly uses
sdk.LoggerFromContextfor consistent context-based logger resolution.
2124-2143: LGTM!Debounced message handling correctly uses SDK helpers for ID/timestamp generation and the updated
saveUserMessagefor dual persistence.
Add reusable DB and JSON helpers and tidy desktop/OpenClaw integrations. Introduces pkg/aidb.BlobScope with scoped Load/Save/Delete helpers and unmarshalJSONField/unmarshalMapJSONField helpers in ai/bridge_db.go. Replace many per-bridge DB scope types with BlobScope usage in codex and openclaw, simplify JSON blob handling, and remove legacy metadata parsing. Add desktopAccountNetwork stub and centralize desktop-network handling, plus tryResolveDesktopTarget to deduplicate desktop message target resolution. Change execDelete to return errors and aggregate failures during purgeLoginData. Reorder savePortal to persist before AI-specific state save. Update OpenClaw login flow to use NewLogin then LoadConnectAndCompleteLogin and improve rollback handling. Bump github.com/beeper/desktop-api-go to v0.5.0 and apply related test adjustments and small fixes across AI and SDK tests.
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
bridges/ai/subagent_spawn.go (1)
333-349:⚠️ Potential issue | 🟠 MajorFail the spawn when the seed prompt can't be persisted.
After this refactor, the initial subagent task only gets stored through
persistInternalPrompt. If that write fails, the run still starts and the child session loses the persisted seed state, so follow-up history/context can diverge after reloads or retries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/subagent_spawn.go` around lines 333 - 349, The code currently continues to create and start a subagent run even if persistInternalPrompt fails; change the flow in the block using persistInternalPrompt so that if persistInternalPrompt(ctx, childPortal, eventID, promptContext, false, "subagent", time.Now()) returns an error you abort the spawn: log the error (oc.loggerForContext(ctx).Warn/Err already present) and return the error (or otherwise propagate failure) instead of proceeding to create runID, constructing subagentRun, calling oc.registerSubagentRun(run), and oc.startSubagentRun(...); ensure no registration or start occurs when persistInternalPrompt fails so the child session isn’t started without persisted seed state.bridges/ai/handlematrix.go (1)
383-413:⚠️ Potential issue | 🟠 MajorDon't continue the edit flow after a transcript upsert failure.
History and regeneration now read from
aiTranscriptTable, so warning and continuing here can leave the bridge row updated while the transcript row stays stale. Subsequent history reads/regeneration will still use the old body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handlematrix.go` around lines 383 - 413, The persistAITranscriptMessage call's error is only logged and the edit flow continues, which can leave bridge rows updated while aiTranscriptTable remains stale; change the block that calls persistAITranscriptMessage(ctx, oc, portal, transcriptMsg) so that on error you log the failure and immediately return the error (or wrap it) to stop further processing (prevent oc.notifySessionMutation, any metadata Update, and oc.regenerateFromEdit from running). Locate the persistAITranscriptMessage call and its surrounding code (including references to oc.loggerForContext, edit.EditTarget, oc.notifySessionMutation, and oc.regenerateFromEdit) and implement an early return on error instead of merely warning. Ensure the caller receives the error so upstream behavior can handle it.bridges/ai/sessions_tools.go (1)
572-580:⚠️ Potential issue | 🟠 MajorDon't treat transcript read failures as "no activity".
Returning
0on any error makessessions_listsilently drop active chats whenactiveMinutesis used, because callers can't distinguish "no messages" from "transcript query failed". Bubble the error up, or at least log it and skip the activity filter on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/sessions_tools.go` around lines 572 - 580, lastMessageTimestamp currently swallows any getAIHistoryMessages error by returning 0 which makes callers (e.g. sessions_list) treat transcript read failures as "no activity"; change the signature of lastMessageTimestamp(ctx context.Context, portal *bridgev2.Portal) to return (int64, error), propagate and return the error from oc.getAIHistoryMessages, and update callers (such as sessions_list) to handle the error by logging it and skipping the activeMinutes filter (or otherwise treating an error as "unknown activity") instead of treating it as timestamp 0; keep the final successful path to return messages[len(messages)-1].Timestamp.UnixMilli() with a nil error.bridges/codex/directory_manager.go (1)
147-160:⚠️ Potential issue | 🟠 MajorRoll back persisted welcome state if portal creation fails.
saveCodexPortalStatehappens beforesdk.EnsurePortalLifecycle. If lifecycle creation errors, theAwaitingCwdSetuprecord is left behind, and the nextwelcomeCodexPortalspass can treat that half-created portal as the existing welcome room. Please defer the save or clear the state on the error path.Minimal fix
created, err := sdk.EnsurePortalLifecycle(ctx, sdk.PortalLifecycleOptions{ Login: cc.UserLogin, Portal: portal, ChatInfo: info, SaveBeforeCreate: true, AIRoomKind: sdk.AIRoomKindAgent, ForceCapabilities: true, }) if err != nil { + _ = clearCodexPortalState(ctx, portal) return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/codex/directory_manager.go` around lines 147 - 160, The code currently calls saveCodexPortalState before sdk.EnsurePortalLifecycle, leaving an AwaitingCwdSetup record if EnsurePortalLifecycle fails; change this by deferring the persistent save until after EnsurePortalLifecycle returns successfully (i.e., call saveCodexPortalState only when sdk.EnsurePortalLifecycle succeeds), or, if you must persist before creation, ensure you roll back on error by calling the state-clearing function (e.g., clearCodexPortalState or equivalent) in the error path after sdk.EnsurePortalLifecycle returns an error; update the block around composeCodexChatInfo, saveCodexPortalState, and sdk.EnsurePortalLifecycle accordingly.bridges/ai/chat.go (1)
745-764:⚠️ Potential issue | 🟠 MajorSave the AI sidecar only after the portal save succeeds.
listAllChatPortals()now readsaichats_portal_statedirectly. If this upsert succeeds andsdk.ConfigureDMPortal(... Save: true)fails, the failed chat still leaves a live sidecar row behind and can show up later as a stale session portal.Suggested fix
portal.Metadata = pmeta - if err := saveAIPortalState(ctx, portal, pmeta); err != nil { - return nil, nil, fmt.Errorf("failed to save portal state: %w", err) - } if err := sdk.ConfigureDMPortal(ctx, sdk.ConfigureDMPortalParams{ Portal: portal, Title: title, @@ }); err != nil { return nil, nil, fmt.Errorf("failed to save portal: %w", err) } + if err := saveAIPortalState(ctx, portal, pmeta); err != nil { + return nil, nil, fmt.Errorf("failed to save portal state: %w", err) + } oc.ensureGhostDisplayName(ctx, modelID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/chat.go` around lines 745 - 764, The current flow saves the AI portal sidecar via saveAIPortalState before calling sdk.ConfigureDMPortal, which can leave a stale aichats_portal_state row if ConfigureDMPortal fails; change the order so that sdk.ConfigureDMPortal(...) with Save: true and its MutatePortal closure is executed first and only after it returns successfully call saveAIPortalState(ctx, portal, pmeta) (keep portal.Metadata = pmeta where appropriate) so the sidecar upsert happens only when the portal save has succeeded.
♻️ Duplicate comments (3)
bridges/openclaw/manager.go (1)
594-605:⚠️ Potential issue | 🟠 MajorKeep the post-resolution resync after resolving a synthetic DM key.
After Line 604 rewrites
state.OpenClawSessionKeyto the real key, the guard at Line 621 can no longer match the synthetic-DM case, sosyncSessionsnever runs. That leaves the portal keyed to the synthetic session until some later full sync, and early events on the real session can still missresolvePortal().Suggested fix
sessionKey := strings.TrimSpace(state.OpenClawSessionKey) + resolvedSyntheticDM := false if state.OpenClawDMCreatedFromContact && state.OpenClawSessionID == "" && isOpenClawSyntheticDMSessionKey(state.OpenClawSessionKey) { if resolvedKey, err := gateway.ResolveSessionKey(ctx, state.OpenClawSessionKey); err == nil { resolvedKey = strings.TrimSpace(resolvedKey) if resolvedKey != "" { updated := *state updated.OpenClawSessionKey = resolvedKey if err := saveOpenClawPortalState(ctx, msg.Portal, m.client.UserLogin, &updated); err != nil { return nil, err } state.OpenClawSessionKey = resolvedKey sessionKey = resolvedKey + resolvedSyntheticDM = true } } } @@ - if state.OpenClawDMCreatedFromContact && state.OpenClawSessionID == "" && isOpenClawSyntheticDMSessionKey(state.OpenClawSessionKey) { + if resolvedSyntheticDM { go func() { if err := m.syncSessions(m.client.BackgroundContext(ctx)); err != nil { m.client.Log().Debug().Err(err).Str("session_key", sessionKey).Msg("Failed to refresh OpenClaw sessions after synthetic DM message") } }() }Also applies to: 621-627
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/manager.go` around lines 594 - 605, The code updates state.OpenClawSessionKey with a resolved real key (via gateway.ResolveSessionKey) and persists it (saveOpenClawPortalState) but then returns without performing the synthetic-DM specific resync, so syncSessions/resolvePortal() never runs for the newly-resolved real session; after successfully saving the updated state (and before returning), explicitly trigger the same resync path you would have taken for a synthetic DM key: call the session synchronization routine (syncSessions) and/or resolvePortal() for the portal using the updated OpenClawSessionKey and m.client.UserLogin so early events on the real session are processed immediately even though isOpenClawSyntheticDMSessionKey no longer matches. Ensure you use the updated state variable when invoking these functions and handle any errors returned from syncSessions/resolvePortal() before returning.bridges/ai/logout_cleanup.go (1)
20-20:⚠️ Potential issue | 🟠 MajorCleanup failures are still non-fatal here.
purgeLoginDatanow aggregates delete errors, but it still only logs them and then clears in-memory state. Logout can therefore continue with partially retained rows, which makes retry/recovery much harder.Also applies to: 98-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/logout_cleanup.go` at line 20, The purgeLoginData function currently swallows delete errors and proceeds to clear in-memory state; change its signature to return an error (e.g., func purgeLoginData(ctx context.Context, login *bridgev2.UserLogin) error), aggregate any deletion failures into a single error and return it instead of only logging, and update its callers (the logout flow that invokes purgeLoginData) to check the returned error and abort/propagate the logout when purgeLoginData fails so rows are not cleared from memory on partial DB failures; keep references to purgeLoginData and the logout caller when making the changes.bridges/ai/scheduler_rooms.go (1)
24-24:⚠️ Potential issue | 🟠 MajorPropagate scheduler portal-save failures instead of swallowing them.
Both paths still use
savePortalQuiet, so the scheduler can return success afterOtherUserIDor metadata/name updates failed to persist. Since these functions already returnerror, this is the right place to surface the failure.Suggested fix
- s.client.savePortalQuiet(ctx, portal, "scheduler room") + if err := s.client.savePortal(ctx, portal, "scheduler room"); err != nil { + return "", fmt.Errorf("failed to persist scheduler room: %w", err) + }- s.client.savePortalQuiet(ctx, portal, "scheduler metadata update") + if err := s.client.savePortal(ctx, portal, "scheduler metadata update"); err != nil { + return nil, fmt.Errorf("failed to persist scheduler metadata update: %w", err) + }Also applies to: 90-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/scheduler_rooms.go` at line 24, The scheduler currently calls s.client.savePortalQuiet(...) which swallows persistence errors; replace those calls in scheduler_rooms.go (the call at the top and the similar calls around lines 90-91) with the non-quiet saver (e.g. s.client.savePortal(ctx, portal, "scheduler room")) and propagate any returned error up (return the error from the scheduler function) instead of ignoring it so failures to persist OtherUserID/metadata/name surface to callers.
🧹 Nitpick comments (1)
bridges/codex/portal_state_db.go (1)
127-140: Please lock this bespoke portal-key format down with a round-trip test.This parser is intentionally the inverse of the key format emitted by this bridge, so the main risk here is future format drift. A focused test for
PortalKey.String()↔parseCodexPortalKeyonid,id/receiver, and empty-input cases would make this much safer to change later.Based on learnings:
parseCodexPortalKeyis intentionally a bespoke inverse of the portal-key format emitted by this bridge, and the mautrixbridgev2/networkidversion used here does not provide a canonicalParsePortalKeyAPI; prefer documenting the invariant and covering it with round-trip tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/codex/portal_state_db.go` around lines 127 - 140, Add a focused round-trip unit test that asserts PortalKey.String() and parseCodexPortalKey are inverses: create PortalKey values for (1) only ID, (2) ID with Receiver, and (3) the zero/empty case, call String() on each, parse the result with parseCodexPortalKey, and assert equality (including trimmed whitespace and empty-input handling). Name the test to reference parseCodexPortalKey and PortalKey.String and include assertions for the boolean ok return value to ensure the parser handles id, id/receiver, and empty-input cases exactly as intended.
🤖 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/ai/account_hints.go`:
- Around line 15-19: The desktopAccountNetwork function currently returns a
constant "unknown" which collapses all desktop accounts into one synthetic
network; replace this by extracting a stable network identifier from the
available beeperdesktopapi.Account metadata (e.g., Service/Provider/API host,
environment, or any other unique field on Account) inside desktopAccountNetwork,
or if those fields aren’t present compute a deterministic fallback (e.g., a
"desktop:<stable-hash-of-account-unique-fields>") so networks are distinct;
update any callers like formatDesktopAccountID to use the new network value (or
stop using network-derived IDs) so accounts from different desktop networks are
not deduplicated.
In `@bridges/ai/agentstore.go`:
- Around line 528-533: Normalize room.Name (trim whitespace and treat empty-only
names as unset) before assigning it to resp.PortalInfo.Name so it matches what
client.applyPortalRoomName(ctx, portal, room.Name) uses; call the same
normalization/trimming used by applyPortalRoomName on room.Name, and only set
resp.PortalInfo.Name = &normalizedName when normalizedName is non-empty
(otherwise leave resp.PortalInfo.Name nil) to avoid creating blank or mismatched
persisted room names.
- Around line 86-98: saveAgent and deleteAgent dereference s.client.UserLogin
unguarded and can panic for a nil/half-initialized AgentStoreAdapter; mirror the
safe-guarding used by loadCustomAgents/loadCustomAgent: first check that s !=
nil, s.client != nil, and s.client.UserLogin is non-empty (or whatever the read
helpers check) and return nil (or the same safe response) if any are missing,
then proceed to acquire s.mu and call
saveCustomAgentForLogin/deleteCustomAgentForLogin; update the functions
saveAgent and deleteAgent to perform that pre-check before locking to avoid
panics.
In `@bridges/ai/chat.go`:
- Line 630: The call to oc.savePortalQuiet(ctx, portal, "agent config") (and the
similar call at the other site) ignores the save result; change these to capture
the returned error and abort/return immediately on failure so
provisioning/materialization does not proceed with only in-memory identity
changes. Specifically, update the savePortalQuiet invocation(s) to: err :=
oc.savePortalQuiet(ctx, portal, "agent config") and if err != nil return err (or
otherwise stop the surrounding operation and propagate/log the error), ensuring
the surrounding function (the provisioning/materialization flow) does not
continue when persistence fails.
In `@bridges/ai/client.go`:
- Around line 623-644: The saveUserMessage flow currently swallows DB/transcript
persistence failures which causes the user turn to be dropped from history;
change AIClient.saveUserMessage to return an error (propagate errors from
oc.UserLogin.Bridge.DB.Message.Insert and persistAITranscriptMessage and from
GetPortalByKey when persistence is required) instead of just logging, and update
callers dispatchOrQueueCore and handleDebouncedMessages to check the returned
error and bail out (stop processing/queueing) when saveUserMessage returns an
error so transcript persistence is treated as part of message acceptance;
reference AIClient.saveUserMessage, persistAITranscriptMessage,
dispatchOrQueueCore, and handleDebouncedMessages when making these changes.
In `@bridges/ai/custom_agents_db.go`:
- Around line 63-78: The stored JSON still contains the original untrimmed
agent.ID while the DB row key uses the trimmed agentID; before marshaling (prior
to json.Marshal(agent)) assign the normalized agentID back to agent.ID so the
content_json matches the row key (i.e., set agent.ID = agentID), then proceed to
marshal and execute the upsert (referencing agentID, agent, json.Marshal(agent),
and scope.db.Exec against aiCustomAgentsTable).
In `@bridges/ai/desktop_api_native_test.go`:
- Around line 12-19: desktopAccountNetwork() is a stub returning "unknown",
breaking network-dependent logic in matchDesktopChatsByLabelAliases,
filterDesktopChatsByResolveOptions and canonical account ID handling; replace
the TODO stub in bridges/ai/account_hints.go with implementation that derives
network from Account.AccountID by parsing known encodings (e.g., if AccountID
contains '_' use the substring before the first '_' as network; otherwise, try
splitting by '-' and map common prefixes like "local", "wa", "ig", "telegram",
"whatsapp" to logical network names) and return a sensible default only if no
pattern matches, and update the test fixtures in
bridges/ai/desktop_api_native_test.go to use realistic AccountIDs that encode
network (e.g., "local-telegram_123..." or "wa_...") so
matchDesktopChatsByLabelAliases and filterDesktopChatsByResolveOptions can
operate correctly.
In `@bridges/ai/internal_prompt_db.go`:
- Around line 148-159: The helper hasInternalPromptHistory should not mask DB
errors; change its signature to return (bool, error) and make it propagate the
query error instead of returning false on failure: in hasInternalPromptHistory
(and where portalScopeForPortal and aiInternalMessagesTable are referenced)
return (true/false, err) where err is non-nil when
scope.db.QueryRow(...).Scan(&count) fails; update all callers to handle the
error (or, if immediate caller needs a conservative fallback, treat a non-nil
error as "true" only after logging the error). Ensure the new behavior logs DB
errors and avoids silently treating errors as "no history."
- Around line 119-122: The json.Unmarshal call in loadInternalPromptHistory
currently returns nil, err for invalid rawTurnData which aborts the entire
replay; change it to treat malformed JSON as a per-row corruption: catch the
unmarshal error, log or record the failure (including rawTurnData and the error)
and continue to the next row instead of returning. Locate the
json.Unmarshal([...], &raw) in loadInternalPromptHistory and replace the return
with a continue (and optional debug log); keep DecodeTurnData's existing per-row
handling intact so only the bad row is skipped.
In `@bridges/ai/login_loaders.go`:
- Around line 35-38: The code calls loginMetadata(existing.UserLogin) and
immediately dereferences .Provider which can panic if loginMetadata returns nil
for a cached login; update the logic around existing.UserLogin and loginMetadata
to nil-check the returned metadata before using .Provider (e.g., call m :=
loginMetadata(existing.UserLogin); if m != nil { existingProvider =
strings.TrimSpace(m.Provider) } else { existingProvider = "" } ), and ensure
similar defensive checks are applied when calling
loginCredentialBaseURL(existing.loginConfigSnapshot(context.Background())) if
that helper can return nil/invalid data so the loader can safely decide rebuild
vs reuse without panicking.
- Around line 96-103: The code assumes meta is non-nil after calling
loginMetadata(login) but loginMetadata can return nil; update the block around
loadAILoginConfig and the call to oc.resolveProviderAPIKeyForConfig so you first
ensure meta is non-nil (e.g., after "if meta == nil { meta =
loginMetadata(login) }" check again), and if still nil use a safe default
provider value (empty string) or return a clear error; then call
oc.resolveProviderAPIKeyForConfig with that guarded provider value (reference
symbols: meta, loginMetadata, loadAILoginConfig,
oc.resolveProviderAPIKeyForConfig) so the function no longer dereferences
meta.Provider when meta can be nil.
In `@bridges/ai/login.go`:
- Around line 256-261: The rollback call to login.Delete is using the original
request context (ctx) which may be canceled or timed out; change the cleanup to
create and use a fresh bounded background context (e.g., ctxCleanup :=
context.WithTimeout(context.Background(), <shortDuration>) and defer cancel())
when invoking login.Delete(ctxCleanup, status.BridgeState{},
bridgev2.DeleteOpts{...}) so the rollback runs reliably even if the original ctx
is expired; keep the same Delete call and options but ensure you cancel the
cleanup context after use.
In `@bridges/ai/tools_message_actions.go`:
- Around line 26-31: Trim whitespace from the emoji input before validation and
removal: call strings.TrimSpace on args["emoji"] when assigning to emoji so the
empty check and the call to btc.Client.removeReaction(ctx, btc.Portal,
targetEventID, emoji) use the normalized value; update the emoji variable in
this block (the one that currently does emoji, _ := args["emoji"].(string)) and
ensure the trimmed emoji is used in the error message and the removeReaction
call.
In `@bridges/codex/directory_manager.go`:
- Around line 251-256: The loop is currently ignoring errors from
clearCodexPortalState which can leave DB portal rows behind; change the logic in
the loop that iterates portals so you capture and handle the error returned by
clearCodexPortalState(ctx, portal) (e.g., if err != nil return or propagate the
error after logging) before calling cc.deletePortalOnly; ensure
clearCodexPortalState failures are not swallowed so listCodexPortalStateRecords
cannot return stale records—use the existing symbols loadCodexPortalState,
clearCodexPortalState, cc.cleanupImportedPortalState, and cc.deletePortalOnly to
locate and update the behavior.
- Around line 234-240: The code is incorrectly skipping DB-backed Codex rooms by
checking in-memory portal metadata; remove the portalMeta(portal) /
meta.IsCodexRoom check in the loop that calls
cc.UserLogin.Bridge.GetExistingPortalByKey so persisted Codex state records
(e.g., the variable record selected earlier) are treated as the source of
truth—delete the conditional that uses portalMeta(portal) and !meta.IsCodexRoom
so unbridge/forget logic relies only on the persisted record and existence of
portal from GetExistingPortalByKey.
In `@bridges/codex/portal_state_db.go`:
- Around line 108-110: When parseCodexPortalKey(portalKeyRaw) returns ok==false
in bridges/codex/portal_state_db.go, log the raw portalKeyRaw (and possibly an
identifying message) before the continue so malformed or drifted portal_key
values are recorded like the malformed-JSON branch; add a
processLogger/Logger.Errorf or equivalent call immediately before the continue
referencing portalKeyRaw and context (e.g., "invalid portal_key, raw value: %s")
to ensure bad rows leave a breadcrumb for backfill/discovery.
In `@bridges/openclaw/manager.go`:
- Around line 1752-1756: The code currently returns early when
loadOpenClawPortalState fails, dropping approval handling; instead, catch and
log the error, set state to a safe empty/default value (the same structure used
in handleApprovalResolved as a fallback), and continue to call
resolveOpenClawAgentID with that empty state so the approval is still registered
and prompted locally; update the loadOpenClawPortalState error path to log the
failure and assign the empty state before calling resolveOpenClawAgentID (refer
to loadOpenClawPortalState, resolveOpenClawAgentID, and handleApprovalResolved
to mirror the safe fallback behavior).
---
Outside diff comments:
In `@bridges/ai/chat.go`:
- Around line 745-764: The current flow saves the AI portal sidecar via
saveAIPortalState before calling sdk.ConfigureDMPortal, which can leave a stale
aichats_portal_state row if ConfigureDMPortal fails; change the order so that
sdk.ConfigureDMPortal(...) with Save: true and its MutatePortal closure is
executed first and only after it returns successfully call
saveAIPortalState(ctx, portal, pmeta) (keep portal.Metadata = pmeta where
appropriate) so the sidecar upsert happens only when the portal save has
succeeded.
In `@bridges/ai/handlematrix.go`:
- Around line 383-413: The persistAITranscriptMessage call's error is only
logged and the edit flow continues, which can leave bridge rows updated while
aiTranscriptTable remains stale; change the block that calls
persistAITranscriptMessage(ctx, oc, portal, transcriptMsg) so that on error you
log the failure and immediately return the error (or wrap it) to stop further
processing (prevent oc.notifySessionMutation, any metadata Update, and
oc.regenerateFromEdit from running). Locate the persistAITranscriptMessage call
and its surrounding code (including references to oc.loggerForContext,
edit.EditTarget, oc.notifySessionMutation, and oc.regenerateFromEdit) and
implement an early return on error instead of merely warning. Ensure the caller
receives the error so upstream behavior can handle it.
In `@bridges/ai/sessions_tools.go`:
- Around line 572-580: lastMessageTimestamp currently swallows any
getAIHistoryMessages error by returning 0 which makes callers (e.g.
sessions_list) treat transcript read failures as "no activity"; change the
signature of lastMessageTimestamp(ctx context.Context, portal *bridgev2.Portal)
to return (int64, error), propagate and return the error from
oc.getAIHistoryMessages, and update callers (such as sessions_list) to handle
the error by logging it and skipping the activeMinutes filter (or otherwise
treating an error as "unknown activity") instead of treating it as timestamp 0;
keep the final successful path to return
messages[len(messages)-1].Timestamp.UnixMilli() with a nil error.
In `@bridges/ai/subagent_spawn.go`:
- Around line 333-349: The code currently continues to create and start a
subagent run even if persistInternalPrompt fails; change the flow in the block
using persistInternalPrompt so that if persistInternalPrompt(ctx, childPortal,
eventID, promptContext, false, "subagent", time.Now()) returns an error you
abort the spawn: log the error (oc.loggerForContext(ctx).Warn/Err already
present) and return the error (or otherwise propagate failure) instead of
proceeding to create runID, constructing subagentRun, calling
oc.registerSubagentRun(run), and oc.startSubagentRun(...); ensure no
registration or start occurs when persistInternalPrompt fails so the child
session isn’t started without persisted seed state.
In `@bridges/codex/directory_manager.go`:
- Around line 147-160: The code currently calls saveCodexPortalState before
sdk.EnsurePortalLifecycle, leaving an AwaitingCwdSetup record if
EnsurePortalLifecycle fails; change this by deferring the persistent save until
after EnsurePortalLifecycle returns successfully (i.e., call
saveCodexPortalState only when sdk.EnsurePortalLifecycle succeeds), or, if you
must persist before creation, ensure you roll back on error by calling the
state-clearing function (e.g., clearCodexPortalState or equivalent) in the error
path after sdk.EnsurePortalLifecycle returns an error; update the block around
composeCodexChatInfo, saveCodexPortalState, and sdk.EnsurePortalLifecycle
accordingly.
---
Duplicate comments:
In `@bridges/ai/logout_cleanup.go`:
- Line 20: The purgeLoginData function currently swallows delete errors and
proceeds to clear in-memory state; change its signature to return an error
(e.g., func purgeLoginData(ctx context.Context, login *bridgev2.UserLogin)
error), aggregate any deletion failures into a single error and return it
instead of only logging, and update its callers (the logout flow that invokes
purgeLoginData) to check the returned error and abort/propagate the logout when
purgeLoginData fails so rows are not cleared from memory on partial DB failures;
keep references to purgeLoginData and the logout caller when making the changes.
In `@bridges/ai/scheduler_rooms.go`:
- Line 24: The scheduler currently calls s.client.savePortalQuiet(...) which
swallows persistence errors; replace those calls in scheduler_rooms.go (the call
at the top and the similar calls around lines 90-91) with the non-quiet saver
(e.g. s.client.savePortal(ctx, portal, "scheduler room")) and propagate any
returned error up (return the error from the scheduler function) instead of
ignoring it so failures to persist OtherUserID/metadata/name surface to callers.
In `@bridges/openclaw/manager.go`:
- Around line 594-605: The code updates state.OpenClawSessionKey with a resolved
real key (via gateway.ResolveSessionKey) and persists it
(saveOpenClawPortalState) but then returns without performing the synthetic-DM
specific resync, so syncSessions/resolvePortal() never runs for the
newly-resolved real session; after successfully saving the updated state (and
before returning), explicitly trigger the same resync path you would have taken
for a synthetic DM key: call the session synchronization routine (syncSessions)
and/or resolvePortal() for the portal using the updated OpenClawSessionKey and
m.client.UserLogin so early events on the real session are processed immediately
even though isOpenClawSyntheticDMSessionKey no longer matches. Ensure you use
the updated state variable when invoking these functions and handle any errors
returned from syncSessions/resolvePortal() before returning.
---
Nitpick comments:
In `@bridges/codex/portal_state_db.go`:
- Around line 127-140: Add a focused round-trip unit test that asserts
PortalKey.String() and parseCodexPortalKey are inverses: create PortalKey values
for (1) only ID, (2) ID with Receiver, and (3) the zero/empty case, call
String() on each, parse the result with parseCodexPortalKey, and assert equality
(including trimmed whitespace and empty-input handling). Name the test to
reference parseCodexPortalKey and PortalKey.String and include assertions for
the boolean ok return value to ensure the parser handles id, id/receiver, and
empty-input cases exactly as intended.
🪄 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: 5f44ccfc-3c44-4a02-a291-eaba4776a81f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (100)
bridges/ai/account_hints.gobridges/ai/agentstore.gobridges/ai/bridge_db.gobridges/ai/chat.gobridges/ai/chat_fork_test.gobridges/ai/client.gobridges/ai/command_registry.gobridges/ai/custom_agents_db.gobridges/ai/delete_chat.gobridges/ai/desktop_api_native_test.gobridges/ai/desktop_api_sessions.gobridges/ai/desktop_api_sessions_test.gobridges/ai/error_logging.gobridges/ai/events.gobridges/ai/events_test.gobridges/ai/gravatar.gobridges/ai/handleai.gobridges/ai/handlematrix.gobridges/ai/handler_interfaces.gobridges/ai/integration_host.gobridges/ai/internal_prompt_db.gobridges/ai/login.gobridges/ai/login_loaders.gobridges/ai/login_loaders_test.gobridges/ai/login_state_db.gobridges/ai/logout_cleanup.gobridges/ai/logout_cleanup_test.gobridges/ai/media_understanding_runner.gobridges/ai/media_understanding_runner_openai_test.gobridges/ai/message_parts.gobridges/ai/metadata.gobridges/ai/metadata_test.gobridges/ai/persistence_boundaries_test.gobridges/ai/portal_materialize.gobridges/ai/portal_state_db.gobridges/ai/prompt_builder.gobridges/ai/reaction_handling.gobridges/ai/reaction_handling_test.gobridges/ai/room_info.gobridges/ai/room_meta_bridgev2_test.gobridges/ai/scheduler_heartbeat_test.gobridges/ai/scheduler_rooms.gobridges/ai/sessions_tools.gobridges/ai/sessions_visibility_test.gobridges/ai/status_text.gobridges/ai/streaming_output_handlers.gobridges/ai/streaming_persistence.gobridges/ai/subagent_spawn.gobridges/ai/tool_approvals.gobridges/ai/tool_approvals_rules.gobridges/ai/tool_configured.gobridges/ai/tools.gobridges/ai/tools_message_actions.gobridges/ai/tools_message_desktop.gobridges/ai/transcript_db.gobridges/codex/constructors.gobridges/codex/directory_manager.gobridges/codex/portal_state_db.gobridges/openclaw/catalog.gobridges/openclaw/identifiers.gobridges/openclaw/login.gobridges/openclaw/manager.gobridges/openclaw/metadata.gobridges/openclaw/provisioning.gobridges/opencode/client.gobridges/opencode/connector.gobridges/opencode/opencode_instance_state.gocmd/agentremote/main.gocmd/agentremote/profile.gocmd/agentremote/profile_test.gocmd/agentremote/run_bridge.gocmd/internal/bridgeentry/bridgeentry.gogo.modpkg/agents/tools/params.gopkg/aidb/001-init.sqlpkg/aidb/json_blob_table.gopkg/integrations/memory/integration.gopkg/integrations/memory/sessions.gopkg/matrixevents/matrixevents.gopkg/shared/toolspec/toolspec.gosdk/approval_flow.gosdk/approval_flow_test.gosdk/approval_reaction_helpers.gosdk/approval_reaction_helpers_test.gosdk/client.gosdk/commands.gosdk/connector_builder_test.gosdk/connector_hooks_test.gosdk/conversation.gosdk/conversation_state.gosdk/conversation_state_test.gosdk/conversation_test.gosdk/helpers.gosdk/load_user_login.gosdk/login_handle.gosdk/login_helpers.gosdk/matrix_actions.gosdk/portal_lifecycle.gosdk/room_features.gosdk/runtime.go
💤 Files with no reviewable changes (5)
- bridges/ai/chat_fork_test.go
- bridges/ai/error_logging.go
- bridges/ai/events_test.go
- bridges/ai/events.go
- bridges/ai/command_registry.go
✅ Files skipped from review due to trivial changes (5)
- bridges/ai/sessions_visibility_test.go
- bridges/ai/message_parts.go
- bridges/ai/scheduler_heartbeat_test.go
- bridges/ai/metadata_test.go
- bridges/ai/portal_state_db.go
🚧 Files skipped from review as they are similar to previous changes (13)
- bridges/ai/portal_materialize.go
- bridges/ai/gravatar.go
- bridges/ai/tool_configured.go
- bridges/ai/desktop_api_sessions_test.go
- bridges/ai/media_understanding_runner.go
- bridges/ai/streaming_output_handlers.go
- bridges/ai/reaction_handling.go
- bridges/codex/constructors.go
- bridges/openclaw/catalog.go
- bridges/ai/prompt_builder.go
- bridges/ai/delete_chat.go
- bridges/opencode/client.go
- bridges/ai/bridge_db.go
📜 Review details
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/codex/portal_state_db.go:127-140
Timestamp: 2026-04-12T18:51:08.539Z
Learning: Repo beeper/agentremote PR 103: In bridges/codex/portal_state_db.go, parseCodexPortalKey is intentionally a bespoke inverse of the portal-key format emitted by this bridge, and the mautrix bridgev2/networkid version used here does not provide a canonical ParsePortalKey API. Do not recommend switching to a non-existent parser; prefer documenting the invariant and covering it with round-trip tests.
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/ai/metadata.go:95-100
Timestamp: 2026-04-12T17:47:00.858Z
Learning: Repo beeper/agentremote: For unreleased AI bridges (package ai and related), do not add or propose migration, legacy-compat, or dual-read/backfill code paths (e.g., metadata/portal-state fallbacks). Keep only the new sidecar/state approach with no legacy compatibility.
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/codex/portal_state_db.go:78-92
Timestamp: 2026-04-12T17:55:54.825Z
Learning: Repo beeper/agentremote PR 103: In bridges/codex/portal_state_db.go, loadCodexPortalState(ctx, portal) intentionally implements a “LoadScopedOrNew” contract and must always return a non-nil *codexPortalState (empty zero state) when scope is missing or no row exists; callers already rely on state != nil.
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/ai/login_state_db.go:0-0
Timestamp: 2026-04-12T17:47:17.903Z
Learning: For the unreleased AI bridge (package bridges/ai), do not include migration or legacy-compat behavior. Specifically, do not carry over legacy UserLoginMetadata fields (e.g., NextChatIndex, LastHeartbeatEvent) into the new login runtime state; treat prior metadata migration suggestions as not applicable.
📚 Learning: 2026-04-12T17:47:17.903Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/ai/login_state_db.go:0-0
Timestamp: 2026-04-12T17:47:17.903Z
Learning: In the unreleased AI bridge implementation under bridges/ai, do not add migration logic or legacy-compat behavior. When building the new login runtime state, do not copy/carry over legacy UserLoginMetadata fields (e.g., NextChatIndex, LastHeartbeatEvent) from older schemas into the new state. Treat any prior migration/compat suggestions for UserLoginMetadata in this area as not applicable for the new login runtime state.
Applied to files:
bridges/ai/status_text.gobridges/ai/media_understanding_runner_openai_test.gobridges/ai/account_hints.gobridges/ai/scheduler_rooms.gobridges/ai/desktop_api_native_test.gobridges/ai/subagent_spawn.gobridges/ai/login_loaders_test.gobridges/ai/logout_cleanup_test.gobridges/ai/reaction_handling_test.gobridges/ai/room_info.gobridges/ai/sessions_tools.gobridges/ai/persistence_boundaries_test.gobridges/ai/streaming_persistence.gobridges/ai/login_state_db.gobridges/ai/tools_message_desktop.gobridges/ai/tool_approvals.gobridges/ai/tools_message_actions.gobridges/ai/tool_approvals_rules.gobridges/ai/metadata.gobridges/ai/login_loaders.gobridges/ai/agentstore.gobridges/ai/desktop_api_sessions.gobridges/ai/room_meta_bridgev2_test.gobridges/ai/handleai.gobridges/ai/login.gobridges/ai/handlematrix.gobridges/ai/internal_prompt_db.gobridges/ai/transcript_db.gobridges/ai/handler_interfaces.gobridges/ai/logout_cleanup.gobridges/ai/tools.gobridges/ai/custom_agents_db.gobridges/ai/integration_host.gobridges/ai/client.gobridges/ai/chat.go
📚 Learning: 2026-04-12T18:51:08.539Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/codex/portal_state_db.go:127-140
Timestamp: 2026-04-12T18:51:08.539Z
Learning: Repo beeper/agentremote PR 103: In bridges/codex/portal_state_db.go, parseCodexPortalKey is intentionally a bespoke inverse of the portal-key format emitted by this bridge, and the mautrix bridgev2/networkid version used here does not provide a canonical ParsePortalKey API. Do not recommend switching to a non-existent parser; prefer documenting the invariant and covering it with round-trip tests.
Applied to files:
bridges/ai/status_text.gobridges/ai/scheduler_rooms.gobridges/ai/subagent_spawn.gobridges/ai/login_loaders_test.gobridges/ai/reaction_handling_test.gobridges/ai/room_info.gobridges/ai/sessions_tools.gobridges/ai/persistence_boundaries_test.gobridges/ai/streaming_persistence.gobridges/ai/login_state_db.gobridges/ai/tool_approvals.gobridges/ai/metadata.gobridges/ai/agentstore.gobridges/ai/room_meta_bridgev2_test.gobridges/ai/handleai.gobridges/codex/portal_state_db.gobridges/ai/login.gobridges/openclaw/login.gobridges/ai/handlematrix.gobridges/openclaw/metadata.gobridges/ai/internal_prompt_db.gobridges/ai/transcript_db.gobridges/ai/handler_interfaces.gobridges/ai/logout_cleanup.gobridges/ai/tools.gobridges/openclaw/provisioning.gobridges/ai/custom_agents_db.gobridges/ai/integration_host.gobridges/codex/directory_manager.gobridges/ai/client.gobridges/ai/chat.gobridges/openclaw/manager.gobridges/openclaw/identifiers.go
📚 Learning: 2026-04-12T17:55:54.825Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/codex/portal_state_db.go:78-92
Timestamp: 2026-04-12T17:55:54.825Z
Learning: Repo beeper/agentremote PR 103: In bridges/codex/portal_state_db.go, loadCodexPortalState(ctx, portal) intentionally implements a “LoadScopedOrNew” contract and must always return a non-nil *codexPortalState (empty zero state) when scope is missing or no row exists; callers already rely on state != nil.
Applied to files:
bridges/ai/status_text.gobridges/ai/scheduler_rooms.gobridges/ai/subagent_spawn.gobridges/ai/login_loaders_test.gobridges/ai/reaction_handling_test.gobridges/ai/room_info.gobridges/ai/sessions_tools.gobridges/ai/persistence_boundaries_test.gobridges/ai/streaming_persistence.gobridges/ai/login_state_db.gobridges/ai/tool_approvals.gobridges/ai/metadata.gobridges/ai/agentstore.gobridges/ai/desktop_api_sessions.gobridges/ai/room_meta_bridgev2_test.gobridges/ai/handleai.gobridges/codex/portal_state_db.gobridges/ai/login.gobridges/openclaw/login.gobridges/ai/handlematrix.gobridges/openclaw/metadata.gobridges/ai/internal_prompt_db.gobridges/ai/transcript_db.gobridges/ai/logout_cleanup.gobridges/ai/tools.gobridges/openclaw/provisioning.gobridges/ai/custom_agents_db.gobridges/ai/integration_host.gobridges/codex/directory_manager.gobridges/ai/client.gobridges/ai/chat.gobridges/openclaw/manager.gobridges/openclaw/identifiers.go
📚 Learning: 2026-04-12T17:47:00.858Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/ai/metadata.go:95-100
Timestamp: 2026-04-12T17:47:00.858Z
Learning: For the unreleased AI bridge code under bridges/ai (including the ai package and related files), do not add or propose migration paths, legacy-compatibility fallbacks, or dual-read/backfill logic (e.g., metadata/portal-state fallbacks). Review changes to ensure the implementation uses only the new sidecar/state approach and does not include any compatibility layers intended to support older data/behavior.
Applied to files:
bridges/ai/status_text.gobridges/ai/media_understanding_runner_openai_test.gobridges/ai/account_hints.gobridges/ai/scheduler_rooms.gobridges/ai/desktop_api_native_test.gobridges/ai/subagent_spawn.gobridges/ai/login_loaders_test.gobridges/ai/logout_cleanup_test.gobridges/ai/reaction_handling_test.gobridges/ai/room_info.gobridges/ai/sessions_tools.gobridges/ai/persistence_boundaries_test.gobridges/ai/streaming_persistence.gobridges/ai/login_state_db.gobridges/ai/tools_message_desktop.gobridges/ai/tool_approvals.gobridges/ai/tools_message_actions.gobridges/ai/tool_approvals_rules.gobridges/ai/metadata.gobridges/ai/login_loaders.gobridges/ai/agentstore.gobridges/ai/desktop_api_sessions.gobridges/ai/room_meta_bridgev2_test.gobridges/ai/handleai.gobridges/ai/login.gobridges/ai/handlematrix.gobridges/ai/internal_prompt_db.gobridges/ai/transcript_db.gobridges/ai/handler_interfaces.gobridges/ai/logout_cleanup.gobridges/ai/tools.gobridges/ai/custom_agents_db.gobridges/ai/integration_host.gobridges/ai/client.gobridges/ai/chat.go
📚 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/ai/reaction_handling_test.gobridges/ai/room_info.gobridges/ai/tool_approvals.gobridges/ai/metadata.gobridges/ai/login_loaders.gobridges/ai/handlematrix.gobridges/ai/client.go
📚 Learning: 2026-04-12T17:46:44.941Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/openclaw/metadata.go:310-337
Timestamp: 2026-04-12T17:46:44.941Z
Learning: For the OpenClaw bridge (bridges/openclaw/*), the project intentionally does not include migration or legacy‑compatibility code (e.g., no seeding from legacy login metadata in loadOpenClawLoginState). Avoid suggesting migration fallbacks for this unreleased bridge.
Applied to files:
bridges/ai/login_state_db.gobridges/ai/metadata.gobridges/codex/portal_state_db.gobridges/ai/logout_cleanup.go
📚 Learning: 2026-04-12T17:47:00.858Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/ai/metadata.go:95-100
Timestamp: 2026-04-12T17:47:00.858Z
Learning: Repo beeper/agentremote: For unreleased AI bridges (package ai and related), do not add or propose migration, legacy-compat, or dual-read/backfill code paths (e.g., metadata/portal-state fallbacks). Keep only the new sidecar/state approach with no legacy compatibility.
Applied to files:
bridges/codex/portal_state_db.gobridges/openclaw/login.gobridges/openclaw/metadata.gobridges/openclaw/provisioning.gobridges/codex/directory_manager.gobridges/openclaw/manager.gobridges/openclaw/identifiers.go
📚 Learning: 2026-04-12T17:47:17.903Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/ai/login_state_db.go:0-0
Timestamp: 2026-04-12T17:47:17.903Z
Learning: For the unreleased AI bridge (package bridges/ai), do not include migration or legacy-compat behavior. Specifically, do not carry over legacy UserLoginMetadata fields (e.g., NextChatIndex, LastHeartbeatEvent) into the new login runtime state; treat prior metadata migration suggestions as not applicable.
Applied to files:
bridges/codex/portal_state_db.gobridges/openclaw/login.gobridges/openclaw/metadata.gobridges/openclaw/manager.gobridges/openclaw/identifiers.go
📚 Learning: 2026-04-12T17:46:44.941Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/openclaw/metadata.go:310-337
Timestamp: 2026-04-12T17:46:44.941Z
Learning: For the unreleased OpenClaw bridge under bridges/openclaw/, the codebase intentionally omits migration/legacy-compatibility logic. When reviewing files in this directory (e.g., loading/saving OpenClaw login state), do not recommend adding migration fallbacks or legacy seeding logic (such as copying legacy login metadata) unless there is an explicit existing compatibility requirement elsewhere (e.g., via documented specs/CI gates).
Applied to files:
bridges/openclaw/login.gobridges/openclaw/metadata.gobridges/openclaw/provisioning.gobridges/openclaw/manager.gobridges/openclaw/identifiers.go
🔇 Additional comments (9)
bridges/ai/media_understanding_runner_openai_test.go (3)
12-25: Nice helper consolidation for test client wiring.This keeps provider/config setup in one place and makes the test fixtures more consistent.
31-37: Good call-site cleanup for MagicProxy test setup.Passing provider and login config directly into the helper removes repetitive inline login construction and keeps test intent clearer.
Also applies to: 44-50
59-63: Provider-explicit test setup looks correct.Using the same helper path for OpenRouter config tests improves consistency across media-understanding test cases.
Also applies to: 108-109
bridges/ai/reaction_handling_test.go (1)
12-32: Good coverage for room-name precedence and fallback behavior.This test cleanly validates both the preferred bridge-owned
portal.Namepath and the slug fallback path, which is exactly the risky behavior boundary.bridges/openclaw/metadata.go (1)
209-221: No issue here. PostgreSQL explicitly supports casting booleans to/from integers (true→1, false→0) via implicit coercion, and both Go'sdatabase/sqland pgx explicitly handle scanning INTEGER columns intoboolvalues. The code is correct as-is.(Optional: Using
BOOLEANin the schema instead ofINTEGERwould make intent clearer, but this is a style preference, not a functional requirement.)bridges/ai/login_state_db.go (1)
201-222: Nice fix on cache/DB consistency.Cloning the loaded state, persisting the clone, and only then swapping
oc.loginStateavoids leaving the in-memory snapshot ahead of the database after a failed write.bridges/ai/handlematrix.go (1)
956-969: Saving the portal row before AI state is the right fix.This removes the earlier "AI state written, portal save failed" partial-commit case.
bridges/ai/persistence_boundaries_test.go (1)
16-118: Good persistence-boundary coverage.These tests pin the new contract well: bridge rows stay transport-only, AI state stays in transcript/portal-state storage, and bridge-owned room names do not get overwritten.
bridges/ai/room_meta_bridgev2_test.go (1)
14-142: Good bridgev2 room-metadata coverage.Exercising the handlers plus
portal.Saveis the right boundary here and should catch regressions in bridge-owned name/topic/avatar persistence.
| // desktopAccountNetwork returns the network identifier for a desktop API account. | ||
| // TODO: add Network field to desktop-api-go Account and remove this stub. | ||
| func desktopAccountNetwork(_ beeperdesktopapi.Account) string { | ||
| return "unknown" | ||
| } |
There was a problem hiding this comment.
Don't collapse every desktop account into the same synthetic network.
Returning a constant "unknown" here makes all downstream bridge/account IDs look like the same network. That breaks network-based filtering and can silently dedupe distinct accounts when two networks reuse the same raw AccountID, because formatDesktopAccountID(...) now produces the same unknown_* key for both. Please derive a real network value here or avoid using network-derived IDs/filters until the value is actually available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/account_hints.go` around lines 15 - 19, The desktopAccountNetwork
function currently returns a constant "unknown" which collapses all desktop
accounts into one synthetic network; replace this by extracting a stable network
identifier from the available beeperdesktopapi.Account metadata (e.g.,
Service/Provider/API host, environment, or any other unique field on Account)
inside desktopAccountNetwork, or if those fields aren’t present compute a
deterministic fallback (e.g., a
"desktop:<stable-hash-of-account-unique-fields>") so networks are distinct;
update any callers like formatDesktopAccountID to use the new network value (or
stop using network-derived IDs) so accounts from different desktop networks are
not deduplicated.
| func (s *AgentStoreAdapter) saveAgent(ctx context.Context, agent *AgentDefinitionContent) error { | ||
| if agent == nil { | ||
| return nil | ||
| } | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
|
|
||
| meta := loginMetadata(s.client.UserLogin) | ||
| if meta.CustomAgents == nil { | ||
| meta.CustomAgents = map[string]*AgentDefinitionContent{} | ||
| } | ||
| meta.CustomAgents[agent.ID] = agent | ||
| return s.client.UserLogin.Save(ctx) | ||
| return saveCustomAgentForLogin(ctx, s.client.UserLogin, agent) | ||
| } | ||
|
|
||
| func (s *AgentStoreAdapter) deleteAgentFromMetadata(ctx context.Context, agentID string) error { | ||
| func (s *AgentStoreAdapter) deleteAgent(ctx context.Context, agentID string) error { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
|
|
||
| meta := loginMetadata(s.client.UserLogin) | ||
| if meta.CustomAgents == nil { | ||
| return nil | ||
| } | ||
| if _, ok := meta.CustomAgents[agentID]; !ok { | ||
| return nil | ||
| } | ||
| delete(meta.CustomAgents, agentID) | ||
| return s.client.UserLogin.Save(ctx) | ||
| return deleteCustomAgentForLogin(ctx, s.client.UserLogin, agentID) |
There was a problem hiding this comment.
Guard the write helpers the same way as the read helpers.
loadCustomAgents and loadCustomAgent return safely when the adapter or login is missing, but saveAgent and deleteAgent dereference s.client.UserLogin unconditionally. A nil or half-initialized adapter will panic here instead of returning an error.
Suggested fix
func (s *AgentStoreAdapter) saveAgent(ctx context.Context, agent *AgentDefinitionContent) error {
if agent == nil {
return nil
}
+ if s == nil || s.client == nil || s.client.UserLogin == nil {
+ return errors.New("login state is not available")
+ }
s.mu.Lock()
defer s.mu.Unlock()
return saveCustomAgentForLogin(ctx, s.client.UserLogin, agent)
}
func (s *AgentStoreAdapter) deleteAgent(ctx context.Context, agentID string) error {
+ if s == nil || s.client == nil || s.client.UserLogin == nil {
+ return errors.New("login state is not available")
+ }
s.mu.Lock()
defer s.mu.Unlock()
return deleteCustomAgentForLogin(ctx, s.client.UserLogin, agentID)
}📝 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 (s *AgentStoreAdapter) saveAgent(ctx context.Context, agent *AgentDefinitionContent) error { | |
| if agent == nil { | |
| return nil | |
| } | |
| s.mu.Lock() | |
| defer s.mu.Unlock() | |
| meta := loginMetadata(s.client.UserLogin) | |
| if meta.CustomAgents == nil { | |
| meta.CustomAgents = map[string]*AgentDefinitionContent{} | |
| } | |
| meta.CustomAgents[agent.ID] = agent | |
| return s.client.UserLogin.Save(ctx) | |
| return saveCustomAgentForLogin(ctx, s.client.UserLogin, agent) | |
| } | |
| func (s *AgentStoreAdapter) deleteAgentFromMetadata(ctx context.Context, agentID string) error { | |
| func (s *AgentStoreAdapter) deleteAgent(ctx context.Context, agentID string) error { | |
| s.mu.Lock() | |
| defer s.mu.Unlock() | |
| meta := loginMetadata(s.client.UserLogin) | |
| if meta.CustomAgents == nil { | |
| return nil | |
| } | |
| if _, ok := meta.CustomAgents[agentID]; !ok { | |
| return nil | |
| } | |
| delete(meta.CustomAgents, agentID) | |
| return s.client.UserLogin.Save(ctx) | |
| return deleteCustomAgentForLogin(ctx, s.client.UserLogin, agentID) | |
| func (s *AgentStoreAdapter) saveAgent(ctx context.Context, agent *AgentDefinitionContent) error { | |
| if agent == nil { | |
| return nil | |
| } | |
| if s == nil || s.client == nil || s.client.UserLogin == nil { | |
| return errors.New("login state is not available") | |
| } | |
| s.mu.Lock() | |
| defer s.mu.Unlock() | |
| return saveCustomAgentForLogin(ctx, s.client.UserLogin, agent) | |
| } | |
| func (s *AgentStoreAdapter) deleteAgent(ctx context.Context, agentID string) error { | |
| if s == nil || s.client == nil || s.client.UserLogin == nil { | |
| return errors.New("login state is not available") | |
| } | |
| s.mu.Lock() | |
| defer s.mu.Unlock() | |
| return deleteCustomAgentForLogin(ctx, s.client.UserLogin, agentID) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/agentstore.go` around lines 86 - 98, saveAgent and deleteAgent
dereference s.client.UserLogin unguarded and can panic for a
nil/half-initialized AgentStoreAdapter; mirror the safe-guarding used by
loadCustomAgents/loadCustomAgent: first check that s != nil, s.client != nil,
and s.client.UserLogin is non-empty (or whatever the read helpers check) and
return nil (or the same safe response) if any are missing, then proceed to
acquire s.mu and call saveCustomAgentForLogin/deleteCustomAgentForLogin; update
the functions saveAgent and deleteAgent to perform that pre-check before locking
to avoid panics.
| // Apply custom room name if provided. | ||
| pm := portalMeta(portal) | ||
| originalName := portal.Name | ||
| originalNameSet := portal.NameSet | ||
| originalTitle := pm.Title | ||
| originalTitleGenerated := pm.TitleGenerated | ||
|
|
||
| if room.Name != "" { | ||
| pm.Title = room.Name | ||
| portal.Name = room.Name | ||
| portal.NameSet = true | ||
| b.client.applyPortalRoomName(ctx, portal, room.Name) | ||
| if resp.PortalInfo != nil { | ||
| resp.PortalInfo.Name = &room.Name | ||
| } |
There was a problem hiding this comment.
Normalize room.Name before copying it into PortalInfo.
applyPortalRoomName trims and ignores whitespace-only names, but resp.PortalInfo.Name still receives the raw input. " " can therefore create a blank Matrix room name while the portal state stays unset, and " foo " leaves the created room and persisted name out of sync.
Suggested fix
- if room.Name != "" {
- b.client.applyPortalRoomName(ctx, portal, room.Name)
- if resp.PortalInfo != nil {
- resp.PortalInfo.Name = &room.Name
- }
- }
+ if trimmedName := strings.TrimSpace(room.Name); trimmedName != "" {
+ b.client.applyPortalRoomName(ctx, portal, trimmedName)
+ if resp.PortalInfo != nil {
+ resp.PortalInfo.Name = &trimmedName
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/agentstore.go` around lines 528 - 533, Normalize room.Name (trim
whitespace and treat empty-only names as unset) before assigning it to
resp.PortalInfo.Name so it matches what client.applyPortalRoomName(ctx, portal,
room.Name) uses; call the same normalization/trimming used by
applyPortalRoomName on room.Name, and only set resp.PortalInfo.Name =
&normalizedName when normalizedName is non-empty (otherwise leave
resp.PortalInfo.Name nil) to avoid creating blank or mismatched persisted room
names.
| if err := portal.Save(ctx); err != nil { | ||
| return nil, fmt.Errorf("failed to save portal with agent config: %w", err) | ||
| } | ||
| oc.savePortalQuiet(ctx, portal, "agent config") |
There was a problem hiding this comment.
Don't continue when agent portal persistence fails.
Both sites mutate persisted portal identity fields and then ignore the save result. If that write is dropped, provisioning/materialization still continues with only in-memory OtherUserID / ResolvedTarget / avatar state, so a reload can reopen the room as the wrong target.
Suggested fix
- oc.savePortalQuiet(ctx, portal, "agent config")
+ if err := oc.savePortal(ctx, portal, "agent config"); err != nil {
+ return nil, fmt.Errorf("failed to persist agent config: %w", err)
+ }- oc.savePortalQuiet(ctx, portal, "default chat agent config")
+ if err := oc.savePortal(ctx, portal, "default chat agent config"); err != nil {
+ oc.loggerForContext(ctx).Err(err).Msg("Failed to persist default chat agent config")
+ return err
+ }Also applies to: 1149-1149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/chat.go` at line 630, The call to oc.savePortalQuiet(ctx, portal,
"agent config") (and the similar call at the other site) ignores the save
result; change these to capture the returned error and abort/return immediately
on failure so provisioning/materialization does not proceed with only in-memory
identity changes. Specifically, update the savePortalQuiet invocation(s) to: err
:= oc.savePortalQuiet(ctx, portal, "agent config") and if err != nil return err
(or otherwise stop the surrounding operation and propagate/log the error),
ensuring the surrounding function (the provisioning/materialization flow) does
not continue when persistence fails.
| func (oc *AIClient) saveUserMessage(ctx context.Context, evt *event.Event, msg *database.Message) { | ||
| if evt != nil { | ||
| msg.MXID = evt.ID | ||
| } | ||
| if _, err := oc.UserLogin.Bridge.GetGhostByID(ctx, msg.SenderID); err != nil { | ||
| oc.loggerForContext(ctx).Warn().Err(err).Msg("Failed to ensure user ghost before saving message") | ||
| } | ||
| if err := oc.UserLogin.Bridge.DB.Message.Insert(ctx, msg); err != nil { | ||
| transportMsg := *msg | ||
| transportMsg.Metadata = &MessageMetadata{} | ||
| if err := oc.UserLogin.Bridge.DB.Message.Insert(ctx, &transportMsg); err != nil { | ||
| oc.loggerForContext(ctx).Err(err).Msg("Failed to save message to database") | ||
| } | ||
| portal, err := oc.UserLogin.Bridge.GetPortalByKey(ctx, msg.Room) | ||
| if err != nil || portal == nil { | ||
| if err != nil { | ||
| oc.loggerForContext(ctx).Warn().Err(err).Msg("Failed to resolve portal for AI transcript persistence") | ||
| } | ||
| return | ||
| } | ||
| if err := persistAITranscriptMessage(ctx, oc, portal, msg); err != nil { | ||
| oc.loggerForContext(ctx).Warn().Err(err).Msg("Failed to persist AI transcript message") | ||
| } |
There was a problem hiding this comment.
Treat transcript persistence as part of message acceptance.
getAIHistoryMessages() now drives later context/session reads, so logging and continuing here drops the user turn from future history while the current request still runs. A transient DB error will desync the next prompt.
Suggested fix
-func (oc *AIClient) saveUserMessage(ctx context.Context, evt *event.Event, msg *database.Message) {
+func (oc *AIClient) saveUserMessage(ctx context.Context, evt *event.Event, msg *database.Message) error {
if evt != nil {
msg.MXID = evt.ID
}
@@
if err := persistAITranscriptMessage(ctx, oc, portal, msg); err != nil {
- oc.loggerForContext(ctx).Warn().Err(err).Msg("Failed to persist AI transcript message")
+ return fmt.Errorf("persist AI transcript message: %w", err)
}
+ return nil
}Update the callers in dispatchOrQueueCore and handleDebouncedMessages to bail out on the returned error instead of continuing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/client.go` around lines 623 - 644, The saveUserMessage flow
currently swallows DB/transcript persistence failures which causes the user turn
to be dropped from history; change AIClient.saveUserMessage to return an error
(propagate errors from oc.UserLogin.Bridge.DB.Message.Insert and
persistAITranscriptMessage and from GetPortalByKey when persistence is required)
instead of just logging, and update callers dispatchOrQueueCore and
handleDebouncedMessages to check the returned error and bail out (stop
processing/queueing) when saveUserMessage returns an error so transcript
persistence is treated as part of message acceptance; reference
AIClient.saveUserMessage, persistAITranscriptMessage, dispatchOrQueueCore, and
handleDebouncedMessages when making these changes.
| emoji, _ := args["emoji"].(string) | ||
|
|
||
| removed, err := removeMatrixReactions(ctx, btc, targetEventID, emoji) | ||
| if err != nil { | ||
| if emoji == "" { | ||
| return "", errors.New("action=react with remove requires an explicit emoji") | ||
| } | ||
| if err := btc.Client.removeReaction(ctx, btc.Portal, targetEventID, emoji); err != nil { | ||
| return "", fmt.Errorf("failed to remove reactions: %w", err) |
There was a problem hiding this comment.
Trim the emoji before attempting removal.
The add path normalizes whitespace, but the remove path now sends the raw string through. Inputs like " 👍 " pass validation here and then fail to match the stored reaction key.
Suggested fix
- emoji, _ := args["emoji"].(string)
+ emoji, _ := args["emoji"].(string)
+ emoji = strings.TrimSpace(emoji)
if emoji == "" {
return "", errors.New("action=react with remove requires an explicit emoji")
}📝 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.
| emoji, _ := args["emoji"].(string) | |
| removed, err := removeMatrixReactions(ctx, btc, targetEventID, emoji) | |
| if err != nil { | |
| if emoji == "" { | |
| return "", errors.New("action=react with remove requires an explicit emoji") | |
| } | |
| if err := btc.Client.removeReaction(ctx, btc.Portal, targetEventID, emoji); err != nil { | |
| return "", fmt.Errorf("failed to remove reactions: %w", err) | |
| emoji, _ := args["emoji"].(string) | |
| emoji = strings.TrimSpace(emoji) | |
| if emoji == "" { | |
| return "", errors.New("action=react with remove requires an explicit emoji") | |
| } | |
| if err := btc.Client.removeReaction(ctx, btc.Portal, targetEventID, emoji); err != nil { | |
| return "", fmt.Errorf("failed to remove reactions: %w", err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/tools_message_actions.go` around lines 26 - 31, Trim whitespace
from the emoji input before validation and removal: call strings.TrimSpace on
args["emoji"] when assigning to emoji so the empty check and the call to
btc.Client.removeReaction(ctx, btc.Portal, targetEventID, emoji) use the
normalized value; update the emoji variable in this block (the one that
currently does emoji, _ := args["emoji"].(string)) and ensure the trimmed emoji
is used in the error message and the removeReaction call.
| portal, err := cc.UserLogin.Bridge.GetExistingPortalByKey(ctx, record.PortalKey) | ||
| if err != nil || portal == nil { | ||
| continue | ||
| } | ||
| meta := portalMeta(portal) | ||
| if meta == nil || !meta.IsCodexRoom || !meta.ManagedImport || strings.TrimSpace(meta.CodexCwd) != path { | ||
| if meta := portalMeta(portal); meta == nil || !meta.IsCodexRoom { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Don't gate DB-backed Codex rooms on in-memory PortalMetadata.
This function already selects persisted Codex state records, but Line 238 re-checks portalMeta(portal).IsCodexRoom. That flag is only in-memory (bridges/codex/metadata.go:62-64), so after a restart imported rooms loaded by GetExistingPortalByKey can fail this check and !codex forget stops unbridging them. The persisted state record should be the source of truth here.
Suggested change
portal, err := cc.UserLogin.Bridge.GetExistingPortalByKey(ctx, record.PortalKey)
if err != nil || portal == nil {
continue
}
- if meta := portalMeta(portal); meta == nil || !meta.IsCodexRoom {
- continue
- }
out = append(out, portal)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/codex/directory_manager.go` around lines 234 - 240, The code is
incorrectly skipping DB-backed Codex rooms by checking in-memory portal
metadata; remove the portalMeta(portal) / meta.IsCodexRoom check in the loop
that calls cc.UserLogin.Bridge.GetExistingPortalByKey so persisted Codex state
records (e.g., the variable record selected earlier) are treated as the source
of truth—delete the conditional that uses portalMeta(portal) and
!meta.IsCodexRoom so unbridge/forget logic relies only on the persisted record
and existence of portal from GetExistingPortalByKey.
| for _, portal := range portals { | ||
| meta := portalMeta(portal) | ||
| if meta != nil { | ||
| cc.cleanupImportedPortalState(meta.CodexThreadID) | ||
| if state, err := loadCodexPortalState(ctx, portal); err == nil && state != nil { | ||
| cc.cleanupImportedPortalState(state.CodexThreadID) | ||
| } | ||
| _ = clearCodexPortalState(ctx, portal) | ||
| cc.deletePortalOnly(ctx, portal, "codex directory forgotten") |
There was a problem hiding this comment.
Please don't ignore clearCodexPortalState failures during forget.
deletePortalOnly no longer removes the DB portal row, so this delete is now the only cleanup of persisted Codex tracking. Swallowing the error can leave forgotten rooms discoverable by listCodexPortalStateRecords while the command reports success.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/codex/directory_manager.go` around lines 251 - 256, The loop is
currently ignoring errors from clearCodexPortalState which can leave DB portal
rows behind; change the logic in the loop that iterates portals so you capture
and handle the error returned by clearCodexPortalState(ctx, portal) (e.g., if
err != nil return or propagate the error after logging) before calling
cc.deletePortalOnly; ensure clearCodexPortalState failures are not swallowed so
listCodexPortalStateRecords cannot return stale records—use the existing symbols
loadCodexPortalState, clearCodexPortalState, cc.cleanupImportedPortalState, and
cc.deletePortalOnly to locate and update the behavior.
| key, ok := parseCodexPortalKey(portalKeyRaw) | ||
| if !ok { | ||
| continue |
There was a problem hiding this comment.
Warn before dropping rows with a bad portal_key.
Line 109 silently skips records when key parsing fails, so format drift or corrupted rows disappear from directory discovery/backfill with no breadcrumb. Please log the raw key before continue, like the malformed-JSON branch does.
Suggested change
key, ok := parseCodexPortalKey(portalKeyRaw)
if !ok {
+ zerolog.Ctx(ctx).Warn().Str("portal_key", portalKeyRaw).Msg("skipping malformed codex portal key")
continue
}📝 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.
| key, ok := parseCodexPortalKey(portalKeyRaw) | |
| if !ok { | |
| continue | |
| key, ok := parseCodexPortalKey(portalKeyRaw) | |
| if !ok { | |
| zerolog.Ctx(ctx).Warn().Str("portal_key", portalKeyRaw).Msg("skipping malformed codex portal key") | |
| continue |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/codex/portal_state_db.go` around lines 108 - 110, When
parseCodexPortalKey(portalKeyRaw) returns ok==false in
bridges/codex/portal_state_db.go, log the raw portalKeyRaw (and possibly an
identifying message) before the continue so malformed or drifted portal_key
values are recorded like the malformed-JSON branch; add a
processLogger/Logger.Errorf or equivalent call immediately before the continue
referencing portalKeyRaw and context (e.g., "invalid portal_key, raw value: %s")
to ensure bad rows leave a breadcrumb for backfill/discovery.
| state, err := loadOpenClawPortalState(ctx, portal, m.client.UserLogin) | ||
| if err != nil { | ||
| return | ||
| } | ||
| agentID := resolveOpenClawAgentID(state, sessionKey, payload.Request) |
There was a problem hiding this comment.
Don't drop approval requests when portal state fails to load.
state is only used here to improve agent resolution. Returning early means the approval never gets registered or prompted locally, so it stays pending upstream until reconnect/rehydration. Falling back to an empty state is safer, like handleApprovalResolved already does.
Suggested fix
state, err := loadOpenClawPortalState(ctx, portal, m.client.UserLogin)
if err != nil {
- return
+ m.client.Log().Warn().Err(err).Str("session_key", sessionKey).Msg("Failed to load OpenClaw portal state for approval request")
+ state = &openClawPortalState{}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/openclaw/manager.go` around lines 1752 - 1756, The code currently
returns early when loadOpenClawPortalState fails, dropping approval handling;
instead, catch and log the error, set state to a safe empty/default value (the
same structure used in handleApprovalResolved as a fallback), and continue to
call resolveOpenClawAgentID with that empty state so the approval is still
registered and prompted locally; update the loadOpenClawPortalState error path
to log the failure and assign the empty state before calling
resolveOpenClawAgentID (refer to loadOpenClawPortalState,
resolveOpenClawAgentID, and handleApprovalResolved to mirror the safe fallback
behavior).
1 participant
No description provided.