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 performs a major architectural refactoring of the AgentRemote SDK integration and login/portal state management. It unifies SDK usage by migrating from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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.
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.
bridges/ai/client.go
Outdated
| 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.
| 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.
bridges/openclaw/manager.go
Outdated
| 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).
Refactor agent enablement checks and command handling, ensure login initialization after saving, improve tool parameter utilities, and update message tool schema. - Extracted agentsEnabledForLoginConfig and simplified shouldEnsureDefaultChat logic; default agents are disabled unless explicitly enabled. - Added applyAgentsEnabledChange to centralize login config updates and return whether enabling requires creating the default welcome chat; fnAgents now uses it and only creates the welcome chat on explicit enable. - Persisted login initialization: OpenAILogin.finishLogin now calls Connector.loadAIUserLogin after saving config and rolls back on failure. - Tests added/updated: verify default-disabled behavior, applyAgentsEnabledChange semantics, and Magic Proxy login client loading. - params.go: overhauled parameter readers (ReadString, ReadStringDefault, ReadIntDefault, ReadBool, ReadStringSlice, ReadStringArray, ReadMap) to tighten validation and provide defaults and helpers. - toolspec: expanded Message tool actions/fields and updated description; added tests ensuring sanitized schemas strip top-level composition keywords. These changes fix correctness around agent enablement defaults, consolidate update logic, ensure persisted logins initialize clients properly, and provide more robust parameter parsing and tool schemas.
Replace the old AI transcript/internal-prompt persistence with a canonical "turn" store and tighten portal scoping. New ai_turns/ai_turn_refs semantics are introduced (replacing aiTranscript/aiInternalMessages usage), and portal-scoped DB rows now use portal_receiver (and strict canonical bridge_id) instead of login_id. Added message_helpers.go and a new turn_store implementation (turns & refs), removed the legacy transcript/internal_prompt DB files, and implemented advanceAIPortalContextEpoch with context_epoch/next_turn_sequence fields on persisted portal records. Updated callers and tests to use persistAIConversationMessage/loadAIConversationMessage/loadAIPromptHistoryTurns/persistAIInternalPromptTurn/deleteAITurnsForPortal/deleteAITurnByExternalRef and adjusted logging/messages from "transcript" to "turn/conversation". Also updated portal state loading/saving to return a portal record and updated DB queries to match the new schema and scoping rules; tests adapted to verify epoch/reset behavior.
Delete canonical_history.go and remove several helper functions used for assembling history messages and image downloads. This removes isImageMimeType and shouldIncludeInHistory, deletes loadAITurnByID, and updates upsertAITurn to set contextEpoch to 0 instead of using record.ContextEpoch. These changes simplify history handling by dropping previously generated-image injection and related filtering/lookup logic.
Add context-aware canonicalization helpers and switch AI DB code to use them. Introduces canonicalPortalForAIDB and portalScopeForAIDB (with context) in bridge_db.go, updates multiple DB/turn/state functions to call portalScopeForAIDB and propagate errors, and fixes related error handling. Adds a test (persistence_boundaries_test.go) that ensures history is replayed for transient portals by canonicalizing portal lookup. Also imports context where needed and tweaks an SQL err assignment. Additionally, add root-level pre-commit hooks for go vet and staticcheck in .pre-commit-config.yaml.
Introduce canonicalLoginBridgeID/canonicalLoginID helpers and replace ad-hoc string conversions throughout the AI bridge to ensure consistent, trimmed bridge/login identifiers and stricter validation. Add assistantTurnCheckpoint and related logic to track assistant turns by canonical sequence/epoch instead of relying solely on timestamps; update API names and usages (LastAssistantTurnCheckpoint, waitForAssistantTurnAfter, latestAssistantTurnRecord, etc.) and adapt scheduler, sessions, integrations, and message-waiting logic to use the new checkpoint semantics. Persist/load login config changes: finishLogin now passes a LoadUserLogin hook to initialize login state before persisting, and OpenAI connector supports loadAIUserLoginWithConfig. Add tests and a DB-backed login harness for these flows. Improve error handling and logging for AI history/turn scope access and textFS store creation. Also switch RemoteEdit stream-order fallback to ResolveEventTiming and add corresponding tests.
Allow scope resolution to fall back to the runtime bridge database ID when portal or login wrappers lack an embedded BridgeID. Added canonicalBridgeDBID and client-scoped helpers (canonicalPortalForClientAIDB, portalScopeForClientAIDB) and refactored history/turn-loading functions to accept/produce portalScope variants. Updated callers across the AI bridge to canonicalize portals via the client and to rely on login-scoped contexts (loginScope/portalScope) instead of requiring embedded bridge IDs everywhere. Added tests to cover saving/loading login config with empty persisted BridgeID and replaying history for transient/missing portal wrapper cases.
Introduce client-scoped portal DB resolution and scope-based turn operations, and centralize portal message sending. Changes: - Add portalScopeForClientAIDB usage and logic to resolve DB, bridgeID and portal receiver from canonical portal when client-scoped context is needed (bridge_db.go). - Replace direct portalScopeForAIDB calls with oc.portalScopeForClientAIDB across integration and persistence code paths to support client-scoped DB access (integration_host.go, turn_store.go, load/persist functions). - Add scope-aware persistence helpers: upsertAITurnByScope, deleteAITurnByExternalRefByScope, loadAIConversationMessageByScope, hasInternalPromptHistoryByScope and related AIClient wrapper methods to operate using resolved portalScope (turn_store.go). - Refactor message sending: introduce buildConvertedPortalTextMessage and sendPortalMessage; route system notices through the same pipeline and use converted messages when sending (chat.go). Update tests to validate converted message structure (portal_send_test.go). - Make sendWelcomeMessage return errors and use sendPortalMessage; persist/rollback welcome state and log failures rather than silent returns. Ensure callers handle errors when sending welcome messages (handleai.go, portal_materialize.go). - Update persistence flows to canonicalize the portal early, extract turn data, and upsert via scope-aware functions (turn_store.go). Added tests for client-scoped turn persistence and transient portal loading (persistence_boundaries_test.go). Why: These changes allow AI client logic to correctly operate when the portal is detached from a portal-specific bridge/database, unify sending paths for AI/system messages, and make persistence operations robust to client-scoped DB contexts and transient portals. Logging and error propagation were improved to avoid silent failures.
Extract queue/room-run runtime and locking into a new bridges/ai/queue_runtime.go, replacing the old activeRooms-based logic with roomLocks and consolidating inflight checks. Replace multiple direct ConfigureDMPortal/EnsurePortalLifecycle calls with a new SDK helper SDK.BootstrapDMPortal (added sdk/portal_bootstrap.go) and adapt callers in codex, openclaw, opencode and ai chat initialization to use the new DMPortalBootstrapSpec/Result flow. Update tests and remove obsolete files (active_room_state.go, room_activity.go), plus adjust related naming and behavior to centralize bootstrap/save/mutate semantics and improve per-room concurrency handling.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
bridges/ai/integrations.go (1)
411-421:⚠️ Potential issue | 🟡 MinorNormalize
loginIDbefore dispatching lifecycle stop.Line 412 validates
strings.TrimSpace(loginID), but Line 420 forwards the untrimmedloginID. If callers pass whitespace-padded IDs,StopForLogincan miss the intended login.Suggested fix
func (oc *AIClient) stopLoginLifecycleIntegrations(bridgeID, loginID string) { - if oc == nil || strings.TrimSpace(loginID) == "" { + normalizedLoginID := strings.TrimSpace(loginID) + if oc == nil || normalizedLoginID == "" { return } oc.eachIntegrationModule(func(_ string, module integrationruntime.ModuleHooks) { loginLifecycle, ok := module.(integrationruntime.LoginLifecycleIntegration) if !ok { return } - loginLifecycle.StopForLogin(bridgeID, loginID) + loginLifecycle.StopForLogin(bridgeID, normalizedLoginID) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/integrations.go` around lines 411 - 421, stopLoginLifecycleIntegrations checks strings.TrimSpace(loginID) but passes the original untrimmed loginID to each module; normalize the ID by assigning a trimmed value (e.g., loginID = strings.TrimSpace(loginID)) before calling module methods so LoginLifecycleIntegration.StopForLogin always receives the canonical ID and won’t miss matches due to surrounding whitespace.bridges/ai/handleai.go (1)
321-328:⚠️ Potential issue | 🟠 MajorRoll back
AutoGreetingSentwhen dispatch fails.Once Line 322 persists the flag, a transient failure at Line 326 leaves the portal marked as greeted forever, so the loop never retries. The welcome-message path already handles this correctly.
Proposed fix
currentMeta.AutoGreetingSent = true if err := oc.savePortal(bgCtx, current, "auto greeting state"); err != nil { oc.loggerForContext(ctx).Warn().Err(err).Msg("Failed to persist auto greeting state") return } if _, _, err := oc.dispatchInternalMessage(bgCtx, current, currentMeta, autoGreetingPrompt, "auto-greeting", true); err != nil { + currentMeta.AutoGreetingSent = false + if saveErr := oc.savePortal(bgCtx, current, "auto greeting rollback"); saveErr != nil { + oc.loggerForContext(ctx).Warn().Err(saveErr).Msg("Failed to roll back auto greeting state") + } oc.loggerForContext(ctx).Warn().Err(err).Msg("Failed to dispatch auto greeting") + return } return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handleai.go` around lines 321 - 328, The code sets currentMeta.AutoGreetingSent and persists it via oc.savePortal before calling oc.dispatchInternalMessage, so if dispatch fails the portal remains marked greeted; modify the flow in the AutoGreeting block to only set and persist currentMeta.AutoGreetingSent = true after a successful oc.dispatchInternalMessage call (using the same bgCtx/current/currentMeta/autoGreetingPrompt/"auto-greeting" call), and if dispatch succeeds then call oc.savePortal to persist the flag; if dispatch fails, leave currentMeta unchanged and log the error as before so the retry loop can attempt sending again.bridges/ai/metadata.go (1)
361-367:⚠️ Potential issue | 🟠 MajorKeep AI-specific fields in
MessageMetadata.CopyFrom.
database.MetaMergerwill call this during merge/update paths, but the new implementation only copies the embedded SDK metadata.MediaUnderstanding,MediaUnderstandingDecisions,MediaURL, andMimeTypewill be dropped on merge.Proposed fix
func (mm *MessageMetadata) CopyFrom(other any) { src, ok := other.(*MessageMetadata) if !ok || src == nil { return } sdk.CopyFromBaseAndAssistant(&mm.BaseMessageMetadata, &src.BaseMessageMetadata, &mm.AssistantMessageMetadata, &src.AssistantMessageMetadata) + mm.MediaUnderstanding = append([]MediaUnderstandingOutput(nil), src.MediaUnderstanding...) + mm.MediaUnderstandingDecisions = append([]MediaUnderstandingDecision(nil), src.MediaUnderstandingDecisions...) + mm.MediaURL = src.MediaURL + mm.MimeType = src.MimeType }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/metadata.go` around lines 361 - 367, The current MessageMetadata.CopyFrom function only delegates to sdk.CopyFromBaseAndAssistant and thus drops AI-specific fields; update MessageMetadata.CopyFrom to, after calling sdk.CopyFromBaseAndAssistant(&mm.BaseMessageMetadata, &src.BaseMessageMetadata, &mm.AssistantMessageMetadata, &src.AssistantMessageMetadata), explicitly copy the AI-specific fields from src to mm (e.g. mm.MediaUnderstanding = src.MediaUnderstanding, mm.MediaUnderstandingDecisions = src.MediaUnderstandingDecisions, mm.MediaURL = src.MediaURL, mm.MimeType = src.MimeType), preserving any pointer/slice/map semantics as appropriate so these fields are not lost during merge.bridges/ai/delete_chat.go (1)
67-70:⚠️ Potential issue | 🟠 MajorPortal-scoped cleanup is incorrectly gated on
sessionKey.This function now deletes portal rows via
portal.PortalKey, but Lines 67-70 return before that work runs when the deleted portal has noMXID. That leavesaiPortalStateTableand AI turns behind for detached/orphaned portals. Only gate the session-key deletes onsessionKey; always run the portal-scoped cleanup whenportalis available.Suggested fix
func (oc *AIClient) deletePersistedSessionArtifacts(ctx context.Context, portal *bridgev2.Portal, sessionKey string) { - if oc == nil { + if oc == nil || portal == nil { return } sessionKey = strings.TrimSpace(sessionKey) - if sessionKey == "" { - return - } db, bridgeID, loginID := loginDBContext(oc) - if db != nil && loginID != "" { + if db != nil && loginID != "" && sessionKey != "" { execDelete(ctx, db, oc.Log(), `DELETE FROM `+aiSessionsTable+` WHERE bridge_id=$1 AND login_id=$2 AND session_key=$3`, bridgeID, loginID, sessionKey, @@ } deleteAITurnsForPortal(ctx, portal) - clearSystemEventsForSession(systemEventsOwnerKey(oc), sessionKey) + if sessionKey != "" { + clearSystemEventsForSession(systemEventsOwnerKey(oc), sessionKey) + } }Also applies to: 99-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/delete_chat.go` around lines 67 - 70, The portal-scoped cleanup is currently skipped whenever sessionKey is empty because the code returns early after trimming sessionKey; change the logic in delete_chat.go so that only the sessionKey-related deletion is gated on sessionKey (i.e., only skip the session-scoped deletes when sessionKey == ""), but always perform portal-scoped cleanup using portal.PortalKey (delete from aiPortalStateTable and related AI turns) whenever the portal object is present; apply the same fix for the other occurrence referenced around the second block (the section covering the logic at the 99-106 area) so portal cleanup is never bypassed for orphaned/detached portals even if sessionKey is empty.bridges/ai/client.go (1)
997-1013:⚠️ Potential issue | 🔴 CriticalGuard the nil login-config path.
loginConfigSnapshot()is treated as nullable elsewhere in this change, butprofilePromptSupplement()dereferencesloginCfg.Profileunconditionally. A login with no persisted config will panic here the first time prompt construction hits this path.Suggested fix
loginCfg := oc.loginConfigSnapshot(context.Background()) var lines []string - if profile := loginCfg.Profile; profile != nil { - if v := strings.TrimSpace(profile.Name); v != "" { - lines = append(lines, "Name: "+v) - } - if v := strings.TrimSpace(profile.Occupation); v != "" { - lines = append(lines, "Occupation: "+v) - } - if v := strings.TrimSpace(profile.AboutUser); v != "" { - lines = append(lines, "About the user: "+v) - } - if v := strings.TrimSpace(profile.CustomInstructions); v != "" { - lines = append(lines, "Custom instructions: "+v) - } + if loginCfg != nil { + if profile := loginCfg.Profile; profile != nil { + if v := strings.TrimSpace(profile.Name); v != "" { + lines = append(lines, "Name: "+v) + } + if v := strings.TrimSpace(profile.Occupation); v != "" { + lines = append(lines, "Occupation: "+v) + } + if v := strings.TrimSpace(profile.AboutUser); v != "" { + lines = append(lines, "About the user: "+v) + } + if v := strings.TrimSpace(profile.CustomInstructions); v != "" { + lines = append(lines, "Custom instructions: "+v) + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/client.go` around lines 997 - 1013, profilePromptSupplement() currently calls loginCfg := oc.loginConfigSnapshot(...) and then dereferences loginCfg.Profile without checking if loginCfg itself is nil; add a nil check for loginCfg before accessing loginCfg.Profile (e.g., return or skip building profile lines when loginCfg == nil) so the function safely handles absent persisted login configs; ensure the check surrounds the block that reads loginCfg.Profile and references profile.Name, profile.Occupation, profile.AboutUser, and profile.CustomInstructions to avoid a panic.
♻️ Duplicate comments (5)
bridges/ai/login_loaders.go (2)
35-38:⚠️ Potential issue | 🔴 CriticalGuard missing cached metadata in the rebuild check.
loginMetadata(existing.UserLogin)can still benilfor cached/broken logins, so Line 36 panics before the loader can decide whether to rebuild. Treat missing metadata as an empty provider here.Suggested fix
existingProvider := "" existingBaseURL := "" if existing.UserLogin != nil { - existingProvider = strings.TrimSpace(loginMetadata(existing.UserLogin).Provider) + if meta := loginMetadata(existing.UserLogin); meta != nil { + existingProvider = strings.TrimSpace(meta.Provider) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/login_loaders.go` around lines 35 - 38, The code assumes loginMetadata(existing.UserLogin) is non-nil and can panic; change the rebuild-check to guard the cached metadata: call loginMetadata(existing.UserLogin) into a local (e.g., meta), check meta != nil before reading Provider and set existingProvider to an empty string when meta is nil, then proceed to compute existingBaseURL using loginCredentialBaseURL(existing.loginConfigSnapshot(context.Background())); this ensures cached/broken logins don’t cause a panic.
110-116:⚠️ Potential issue | 🔴 CriticalHandle
nillogin metadata before resolving the provider config.After the fallback in Lines 110-112,
metacan still benil. Line 116 then dereferencesmeta.Providerand turns a recoverable broken login into a panic instead of failing closed.Suggested fix
if meta == nil { meta = loginMetadata(login) } + if meta == nil { + oc.evictCachedClient(login.ID, nil) + login.Client = newBrokenLoginClient(login, initLoginClientError) + return nil + } if cfg == nil { cfg = &aiLoginConfig{} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/login_loaders.go` around lines 110 - 116, The code dereferences meta.Provider when calling oc.resolveProviderAPIKeyForConfig even though meta can still be nil after the fallback; modify the logic around loginMetadata(login), cfg (aiLoginConfig) and the call to oc.resolveProviderAPIKeyForConfig so that you explicitly handle meta == nil (either return/fail closed or use a safe default provider) before calling oc.resolveProviderAPIKeyForConfig; ensure the code only invokes oc.resolveProviderAPIKeyForConfig(meta.Provider, cfg) when meta is non-nil and otherwise takes a safe error path.bridges/ai/logout_cleanup.go (1)
19-19:⚠️ Potential issue | 🟠 MajorDon't keep logout “successful” after partial purge failures.
The joined error is only logged. Callers still proceed as if cleanup succeeded, and Lines 100-104 then wipe the in-memory login state/config anyway, which makes retrying the failed purge harder. Please return the aggregated error so logout can abort or retry instead of silently leaving data behind.
Also applies to: 97-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 19, The purgeLoginData function currently only logs aggregated errors and returns nothing, letting callers proceed as if cleanup succeeded; change purgeLoginData(ctx context.Context, login *bridgev2.UserLogin) to return error, return the joined/aggregated error when any sub-purge fails, and update its callers so they check the returned error and abort/allow retry instead of continuing to wipe in-memory login state/config; ensure callers that currently clear state (the logout path that clears login state/config) only do so on nil error from purgeLoginData.bridges/ai/client.go (1)
540-583:⚠️ Potential issue | 🟠 MajorTreat user-message persistence as part of acceptance.
saveUserMessage()still only logs failures fromupsertTransportPortalMessage()/persistAIConversationMessage(), and callers keep going. That lets the live run succeed while the canonical AI turn store drops the user turn, which desyncs later history-based prompts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/client.go` around lines 540 - 583, saveUserMessage currently swallows failures from upsertTransportPortalMessage and persistAIConversationMessage so user-turn persistence can silently fail; change saveUserMessage (and its callers) to return an error instead of only logging: in function saveUserMessage(ctx, evt, msg) return an error when GetPortalByKey fails (already returns) or when upsertTransportPortalMessage or persistAIConversationMessage return errors, propagating those errors to callers (update any callers to check the returned error and abort/handle the AI turn accordingly) so persistence failures stop the live run from proceeding and callers can retry or surface the failure.bridges/ai/chat.go (1)
615-630:⚠️ Potential issue | 🟠 MajorPersist post-bootstrap agent retargeting to the AI sidecar, and don't continue on save failure.
These blocks change
portal.OtherUserID/pm.ResolvedTargetafterinitPortalForChat()has already written sidecar state inBeforeSave, then only callsavePortalQuiet(). Even when the portal row save succeeds,aiPortalStateTablestill contains the pre-agent state; if it fails, provisioning/materialization still continues with in-memory-only identity changes.Based on learnings: For the unreleased AI bridge code under
bridges/ai, the implementation should use only the new sidecar/state approach with no metadata/portal-state fallbacks.Also applies to: 1151-1156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/chat.go` around lines 615 - 630, The post-bootstrap retargeting updates (portal.OtherUserID, pm.ResolvedTarget, portal.AvatarID/AvatarMXC and RuntimeModelOverride) must be persisted to the AI sidecar/state instead of relying on the pre-existing portal metadata and must abort on save failures; after calling initPortalForChat() replace the in-memory-only oc.savePortalQuiet(...) call with a transactional sidecar/state persist to aiPortalStateTable (or the designated sidecar writer used elsewhere in this package) and return an error if that persist fails so provisioning/materialization does not continue with only in-memory changes; remove any fallback logic that expects portal DB metadata to carry the new agent identity and ensure the same persistence is applied for the identical block at lines ~1151-1156.
🧹 Nitpick comments (1)
bridges/ai/tool_schema_sanitize_test.go (1)
122-133: Strengthen this test to avoid vacuous pass cases.At Line 123, if
cleanedisnil/empty, the composition-key checks still pass. Add one positive structural assertion (e.g., root type or properties presence) so the test proves sanitization produced a meaningful schema.Suggested test hardening
func TestSanitizeToolSchema_MessageSchemaHasNoTopLevelComposition(t *testing.T) { cleaned, _ := sanitizeToolSchemaWithReport(toolspec.MessageSchema()) + if cleaned == nil || len(cleaned) == 0 { + t.Fatalf("expected sanitized schema to be non-empty") + } + if typ, _ := cleaned["type"].(string); typ != "object" { + t.Fatalf("expected top-level type=object, got %v", cleaned["type"]) + } if _, ok := cleaned["allOf"]; ok { t.Fatalf("expected allOf to be absent") } if _, ok := cleaned["anyOf"]; ok { t.Fatalf("expected anyOf to be absent") } if _, ok := cleaned["oneOf"]; ok { t.Fatalf("expected oneOf to be absent") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/tool_schema_sanitize_test.go` around lines 122 - 133, The test TestSanitizeToolSchema_MessageSchemaHasNoTopLevelComposition currently can pass vacuously if cleaned is nil/empty; update it to assert that sanitizeToolSchemaWithReport(toolspec.MessageSchema()) returns a non-nil, non-empty map and that the root schema has an expected structural field (for example assert cleaned["type"] == "object" or that cleaned["properties"] is present and non-empty) before checking that "allOf", "anyOf", and "oneOf" are absent; reference sanitizeToolSchemaWithReport and toolspec.MessageSchema() to locate the call and add the positive structural assertion(s) to guarantee the sanitizer produced a meaningful schema.
🤖 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/canonical_history.go`:
- Around line 36-37: Replace the case-sensitive MIME check with the standardized
helper: call stringutil.NormalizeMimeType on f.MimeType and use
strings.HasPrefix on the normalized value (instead of
strings.HasPrefix(strings.TrimSpace(f.MimeType), "image/")) while keeping the
existing f.URL empty check; update the condition in canonical_history.go (the
block referencing f.MimeType and f.URL) so mixed-case MIME values like
"Image/PNG" are accepted.
In `@bridges/ai/commands_parity.go`:
- Around line 46-50: The code sets meta.SessionResetAt before calling
advanceAIPortalContextEpoch, so if advanceAIPortalContextEpoch(ce.Ctx,
ce.Portal) fails the in-memory meta.SessionResetAt is mutated; move the
assignment of meta.SessionResetAt = time.Now().UnixMilli() to after
advanceAIPortalContextEpoch returns successfully (i.e., only set SessionResetAt
once advanceAIPortalContextEpoch(ce.Ctx, ce.Portal) completes without error) and
keep the existing error handling path (client.log.Warn... and ce.Reply...)
unchanged so failed advances do not leave a stale reset timestamp in memory or
persisted state.
In `@bridges/ai/login_config_db.go`:
- Around line 166-182: updateLoginConfig currently holds loginConfigMu while
calling saveAILoginConfig, causing a deadlock because saveAILoginConfig can
re-enter via login.Client.(*AIClient); change the flow so the mutex is not held
during DB writes: call loadAILoginConfig and run fn under loginConfigMu to
determine if the config changed, capture the modified aiLoginConfig (or a dirty
flag), then release loginConfigMu before calling the persistence helper (or a
new DB-only saveAILoginConfigDB helper); after the DB write, re-acquire
loginConfigMu to update oc.loginConfig if needed. Ensure references to
oc.loginConfig, oc.loginConfigMu, updateLoginConfig, saveAILoginConfig,
loadAILoginConfig, UserLogin and aiLoginConfig are used to locate and apply the
change.
In `@bridges/ai/queue_runtime.go`:
- Around line 61-66: releaseRoom currently holds roomLocksMu while calling
clearRoomRun (which does Matrix I/O and can block), so move the ack cleanup
outside the lock: acquire roomLocksMu, delete oc.roomLocks[roomID] (and capture
any needed local state if required), then unlock roomLocksMu before calling
oc.clearRoomRun(roomID); ensure you do not access oc.roomLocks after releasing
the lock. This change keeps the critical section minimal (only the delete) and
prevents a slow clearRoomRun from blocking acquireRoom/releaseRoom for other
rooms.
- Around line 144-170: The queue decision can misclassify a room as idle because
roomBusy is computed only from oc.roomHasActiveRun and
oc.roomHasPendingQueueWork but not the real exclusivity guard (roomLocks),
allowing a race between queueDecision and acquireRoom/attachRoomRun; fix by
including the room lock state in the busy check (e.g., consult oc.roomLocks or
expose a helper like oc.roomIsLocked(roomID)) or alternatively re-check lock
after acquireRoom and before acting on queueDecision — update the logic around
roomHasActiveRun/roomHasPendingQueueWork, queueDecision, acquireRoom, and
attachRoomRun so the exclusivity guard (roomLocks) is considered before deciding
InterruptAndRun or Drop (referenced symbols: roomHasActiveRun,
roomHasPendingQueueWork, roomLocks/roomIsLocked, DecideQueueAction, acquireRoom,
attachRoomRun, cancelRoomRun, clearPendingQueue).
In `@bridges/ai/tool_approvals.go`:
- Around line 108-115: The function normalizeMcpRuleToolName currently calls
normalizeApprovalToken which lowercases tool names; change it to preserve casing
by trimming whitespace only and removing a leading "mcp." prefix without
lowercasing. Concretely, update normalizeMcpRuleToolName to perform
strings.TrimSpace(name) and then strip the "mcp." prefix (e.g., via
strings.HasPrefix + strings.TrimPrefix) so the returned tool name keeps its
original case while still removing the synthetic prefix; do not call
normalizeApprovalToken from normalizeMcpRuleToolName.
In `@bridges/ai/turn_store.go`:
- Around line 96-108: The current allocateAITurnSequence reads NextTurnSequence
in Go then writes back, causing race conditions; change it to perform a single
atomic SQL update that increments and returns the new sequence (e.g. UPDATE
<aiPortalStateTable> SET next_turn_sequence = next_turn_sequence + 1,
updated_at_ms = $X RETURNING next_turn_sequence, context_epoch) and use that
returned value as sequence/contextEpoch instead of reading then writing; ensure
you still handle the "row missing" case by calling ensurePortalTurnStateByScope
or inserting the portal-state row and retrying the single-statement UPDATE if
needed, and replace the scope.db.Exec usage with a query that reads the
RETURNING values so allocateAITurnSequence (and references to scope.db,
aiPortalStateTable, ensurePortalTurnStateByScope, and the sequence/contextEpoch
locals) atomically obtains the incremented sequence.
---
Outside diff comments:
In `@bridges/ai/client.go`:
- Around line 997-1013: profilePromptSupplement() currently calls loginCfg :=
oc.loginConfigSnapshot(...) and then dereferences loginCfg.Profile without
checking if loginCfg itself is nil; add a nil check for loginCfg before
accessing loginCfg.Profile (e.g., return or skip building profile lines when
loginCfg == nil) so the function safely handles absent persisted login configs;
ensure the check surrounds the block that reads loginCfg.Profile and references
profile.Name, profile.Occupation, profile.AboutUser, and
profile.CustomInstructions to avoid a panic.
In `@bridges/ai/delete_chat.go`:
- Around line 67-70: The portal-scoped cleanup is currently skipped whenever
sessionKey is empty because the code returns early after trimming sessionKey;
change the logic in delete_chat.go so that only the sessionKey-related deletion
is gated on sessionKey (i.e., only skip the session-scoped deletes when
sessionKey == ""), but always perform portal-scoped cleanup using
portal.PortalKey (delete from aiPortalStateTable and related AI turns) whenever
the portal object is present; apply the same fix for the other occurrence
referenced around the second block (the section covering the logic at the 99-106
area) so portal cleanup is never bypassed for orphaned/detached portals even if
sessionKey is empty.
In `@bridges/ai/handleai.go`:
- Around line 321-328: The code sets currentMeta.AutoGreetingSent and persists
it via oc.savePortal before calling oc.dispatchInternalMessage, so if dispatch
fails the portal remains marked greeted; modify the flow in the AutoGreeting
block to only set and persist currentMeta.AutoGreetingSent = true after a
successful oc.dispatchInternalMessage call (using the same
bgCtx/current/currentMeta/autoGreetingPrompt/"auto-greeting" call), and if
dispatch succeeds then call oc.savePortal to persist the flag; if dispatch
fails, leave currentMeta unchanged and log the error as before so the retry loop
can attempt sending again.
In `@bridges/ai/integrations.go`:
- Around line 411-421: stopLoginLifecycleIntegrations checks
strings.TrimSpace(loginID) but passes the original untrimmed loginID to each
module; normalize the ID by assigning a trimmed value (e.g., loginID =
strings.TrimSpace(loginID)) before calling module methods so
LoginLifecycleIntegration.StopForLogin always receives the canonical ID and
won’t miss matches due to surrounding whitespace.
In `@bridges/ai/metadata.go`:
- Around line 361-367: The current MessageMetadata.CopyFrom function only
delegates to sdk.CopyFromBaseAndAssistant and thus drops AI-specific fields;
update MessageMetadata.CopyFrom to, after calling
sdk.CopyFromBaseAndAssistant(&mm.BaseMessageMetadata, &src.BaseMessageMetadata,
&mm.AssistantMessageMetadata, &src.AssistantMessageMetadata), explicitly copy
the AI-specific fields from src to mm (e.g. mm.MediaUnderstanding =
src.MediaUnderstanding, mm.MediaUnderstandingDecisions =
src.MediaUnderstandingDecisions, mm.MediaURL = src.MediaURL, mm.MimeType =
src.MimeType), preserving any pointer/slice/map semantics as appropriate so
these fields are not lost during merge.
---
Duplicate comments:
In `@bridges/ai/chat.go`:
- Around line 615-630: The post-bootstrap retargeting updates
(portal.OtherUserID, pm.ResolvedTarget, portal.AvatarID/AvatarMXC and
RuntimeModelOverride) must be persisted to the AI sidecar/state instead of
relying on the pre-existing portal metadata and must abort on save failures;
after calling initPortalForChat() replace the in-memory-only
oc.savePortalQuiet(...) call with a transactional sidecar/state persist to
aiPortalStateTable (or the designated sidecar writer used elsewhere in this
package) and return an error if that persist fails so
provisioning/materialization does not continue with only in-memory changes;
remove any fallback logic that expects portal DB metadata to carry the new agent
identity and ensure the same persistence is applied for the identical block at
lines ~1151-1156.
In `@bridges/ai/client.go`:
- Around line 540-583: saveUserMessage currently swallows failures from
upsertTransportPortalMessage and persistAIConversationMessage so user-turn
persistence can silently fail; change saveUserMessage (and its callers) to
return an error instead of only logging: in function saveUserMessage(ctx, evt,
msg) return an error when GetPortalByKey fails (already returns) or when
upsertTransportPortalMessage or persistAIConversationMessage return errors,
propagating those errors to callers (update any callers to check the returned
error and abort/handle the AI turn accordingly) so persistence failures stop the
live run from proceeding and callers can retry or surface the failure.
In `@bridges/ai/login_loaders.go`:
- Around line 35-38: The code assumes loginMetadata(existing.UserLogin) is
non-nil and can panic; change the rebuild-check to guard the cached metadata:
call loginMetadata(existing.UserLogin) into a local (e.g., meta), check meta !=
nil before reading Provider and set existingProvider to an empty string when
meta is nil, then proceed to compute existingBaseURL using
loginCredentialBaseURL(existing.loginConfigSnapshot(context.Background())); this
ensures cached/broken logins don’t cause a panic.
- Around line 110-116: The code dereferences meta.Provider when calling
oc.resolveProviderAPIKeyForConfig even though meta can still be nil after the
fallback; modify the logic around loginMetadata(login), cfg (aiLoginConfig) and
the call to oc.resolveProviderAPIKeyForConfig so that you explicitly handle meta
== nil (either return/fail closed or use a safe default provider) before calling
oc.resolveProviderAPIKeyForConfig; ensure the code only invokes
oc.resolveProviderAPIKeyForConfig(meta.Provider, cfg) when meta is non-nil and
otherwise takes a safe error path.
In `@bridges/ai/logout_cleanup.go`:
- Line 19: The purgeLoginData function currently only logs aggregated errors and
returns nothing, letting callers proceed as if cleanup succeeded; change
purgeLoginData(ctx context.Context, login *bridgev2.UserLogin) to return error,
return the joined/aggregated error when any sub-purge fails, and update its
callers so they check the returned error and abort/allow retry instead of
continuing to wipe in-memory login state/config; ensure callers that currently
clear state (the logout path that clears login state/config) only do so on nil
error from purgeLoginData.
---
Nitpick comments:
In `@bridges/ai/tool_schema_sanitize_test.go`:
- Around line 122-133: The test
TestSanitizeToolSchema_MessageSchemaHasNoTopLevelComposition currently can pass
vacuously if cleaned is nil/empty; update it to assert that
sanitizeToolSchemaWithReport(toolspec.MessageSchema()) returns a non-nil,
non-empty map and that the root schema has an expected structural field (for
example assert cleaned["type"] == "object" or that cleaned["properties"] is
present and non-empty) before checking that "allOf", "anyOf", and "oneOf" are
absent; reference sanitizeToolSchemaWithReport and toolspec.MessageSchema() to
locate the call and add the positive structural assertion(s) to guarantee the
sanitizer produced a meaningful schema.
🪄 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: 668fba79-01cd-4e73-9c9f-35a83b48c246
📒 Files selected for processing (135)
.pre-commit-config.yamlbridges/ai/active_room_state.gobridges/ai/agent_display.gobridges/ai/approval_prompt_presentation.gobridges/ai/bootstrap_context.gobridges/ai/bootstrap_context_test.gobridges/ai/bridge_db.gobridges/ai/canonical_history.gobridges/ai/chat.gobridges/ai/chat_bootstrap_test.gobridges/ai/client.gobridges/ai/client_init_test.gobridges/ai/commands.gobridges/ai/commands_parity.gobridges/ai/commands_test.gobridges/ai/delete_chat.gobridges/ai/handleai.gobridges/ai/handlematrix.gobridges/ai/heartbeat_events.gobridges/ai/heartbeat_execute.gobridges/ai/identifiers.gobridges/ai/integration_host.gobridges/ai/integrations.gobridges/ai/internal_dispatch.gobridges/ai/login.gobridges/ai/login_config_db.gobridges/ai/login_loaders.gobridges/ai/login_loaders_test.gobridges/ai/login_test.gobridges/ai/logout_cleanup.gobridges/ai/logout_cleanup_test.gobridges/ai/message_helpers.gobridges/ai/metadata.gobridges/ai/persistence_boundaries_test.gobridges/ai/portal_cleanup.gobridges/ai/portal_materialize.gobridges/ai/portal_send_test.gobridges/ai/portal_state_db.gobridges/ai/prompt_builder.gobridges/ai/queue_policy_runtime_test.gobridges/ai/queue_runtime.gobridges/ai/queue_status_test.gobridges/ai/room_activity.gobridges/ai/scheduler_cron.gobridges/ai/sessions_tools.gobridges/ai/streaming_error_handling.gobridges/ai/streaming_finish_reason_test.gobridges/ai/streaming_persistence.gobridges/ai/streaming_success.gobridges/ai/subagent_spawn.gobridges/ai/test_login_helpers_test.gobridges/ai/tool_approvals.gobridges/ai/tool_approvals_helpers_test.gobridges/ai/tool_approvals_policy.gobridges/ai/tool_approvals_rules.gobridges/ai/tool_configured.gobridges/ai/tool_schema_sanitize_test.gobridges/ai/tools.gobridges/ai/tools_beeper_docs.gobridges/ai/tools_search_fetch.gobridges/ai/tools_search_fetch_test.gobridges/ai/turn_store.gobridges/codex/approval_rpc.gobridges/codex/approval_runtime.gobridges/codex/backfill.gobridges/codex/client.gobridges/codex/compat_helpers.gobridges/codex/constructors.gobridges/codex/directory_manager.gobridges/codex/dispatch_test.gobridges/codex/identifiers.gobridges/codex/login.gobridges/codex/metadata.gobridges/codex/portal_keys.gobridges/codex/portal_send.gobridges/codex/runtime_helpers.gobridges/codex/sdk_agent.gobridges/dummybridge/connector_session.gobridges/dummybridge/runtime.gobridges/dummybridge/runtime_commands.gobridges/dummybridge/runtime_runner.gobridges/dummybridge/runtime_text.gobridges/dummybridge/runtime_types.gobridges/openclaw/approval.gobridges/openclaw/client.gobridges/openclaw/connector.gobridges/openclaw/discovery.gobridges/openclaw/discovery_provisioning.gobridges/openclaw/events.gobridges/openclaw/identifiers.gobridges/openclaw/login.gobridges/openclaw/login_prefill.gobridges/openclaw/manager.gobridges/openclaw/media_test.gobridges/openclaw/metadata.gobridges/openclaw/provisioning.gobridges/openclaw/provisioning_test.gobridges/openclaw/sdk_agent.gobridges/openclaw/status.gobridges/openclaw/stream.gobridges/opencode/backfill_canonical.gobridges/opencode/client.gobridges/opencode/message_metadata.gobridges/opencode/opencode_canonical_stream.gobridges/opencode/opencode_delete.gobridges/opencode/opencode_helpers.gobridges/opencode/opencode_identifiers.gobridges/opencode/opencode_messages.gobridges/opencode/opencode_portal.gobridges/opencode/opencode_text_stream.gobridges/opencode/opencode_tool_stream.gobridges/opencode/opencode_turn_stream.gobridges/opencode/sdk_agent.gobridges/opencode/stream_canonical.gobridges/opencode/stream_canonical_test.gobridges/opencode/stream_metadata.gocmd/agentremote/run_bridge.gocmd/internal/bridgeentry/bridgeentry.godocs/duplication-audit.mddocs/rewrite-plan.mdpkg/agents/tools/params.gopkg/agents/tools/websearch.gopkg/aidb/001-init.sqlpkg/aidb/db.gopkg/aidb/db_test.gopkg/fetch/env.gopkg/fetch/router_test.gopkg/fetch/types.gopkg/matrixevents/matrixevents.gopkg/matrixevents/matrixevents_test.gopkg/retrieval/config.gopkg/retrieval/env.gopkg/retrieval/fetch.gopkg/retrieval/fetch_test.gopkg/retrieval/provider_direct.go
💤 Files with no reviewable changes (7)
- bridges/ai/portal_send_test.go
- bridges/ai/active_room_state.go
- bridges/ai/client_init_test.go
- bridges/ai/room_activity.go
- bridges/ai/approval_prompt_presentation.go
- bridges/ai/tool_approvals_rules.go
- bridges/ai/tool_approvals_policy.go
✅ Files skipped from review due to trivial changes (3)
- bridges/ai/streaming_finish_reason_test.go
- bridges/ai/portal_state_db.go
- bridges/ai/tools.go
🚧 Files skipped from review as they are similar to previous changes (10)
- bridges/ai/portal_cleanup.go
- bridges/ai/tool_approvals_helpers_test.go
- bridges/ai/tool_configured.go
- bridges/ai/tools_search_fetch_test.go
- bridges/ai/logout_cleanup_test.go
- bridges/ai/sessions_tools.go
- bridges/ai/scheduler_cron.go
- bridges/ai/login.go
- bridges/ai/integration_host.go
- bridges/ai/handlematrix.go
👮 Files not reviewed due to content moderation or server errors (8)
- bridges/ai/queue_status_test.go
- bridges/ai/identifiers.go
- bridges/ai/subagent_spawn.go
- bridges/ai/commands.go
- bridges/ai/prompt_builder.go
- bridges/ai/commands_test.go
- bridges/ai/login_test.go
- bridges/ai/heartbeat_execute.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-docker
- GitHub Check: Lint
- GitHub Check: build-agentremote-cli-docker (amd64)
- GitHub Check: build-docker
- GitHub Check: Lint
🧰 Additional context used
🧠 Learnings (7)
📓 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-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/streaming_error_handling.gobridges/ai/commands_parity.gobridges/ai/bootstrap_context.gobridges/ai/agent_display.gobridges/ai/tools_beeper_docs.gobridges/ai/portal_materialize.gobridges/ai/streaming_success.gobridges/ai/integrations.gobridges/ai/identifiers.gobridges/ai/bootstrap_context_test.gobridges/ai/subagent_spawn.gobridges/ai/login_test.gobridges/ai/tool_approvals.gobridges/ai/chat_bootstrap_test.gobridges/ai/logout_cleanup.gobridges/ai/internal_dispatch.gobridges/ai/delete_chat.gobridges/ai/test_login_helpers_test.gobridges/ai/login_config_db.gobridges/ai/prompt_builder.gobridges/ai/metadata.gobridges/ai/message_helpers.gobridges/ai/persistence_boundaries_test.gobridges/ai/heartbeat_events.gobridges/codex/approval_rpc.gobridges/ai/login_loaders_test.gobridges/ai/commands.gobridges/ai/turn_store.gobridges/ai/streaming_persistence.gobridges/ai/bridge_db.gobridges/ai/handleai.gobridges/ai/heartbeat_execute.gobridges/ai/chat.gobridges/ai/tools_search_fetch.gobridges/ai/client.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/streaming_error_handling.gobridges/ai/commands_parity.gobridges/ai/bootstrap_context.gobridges/ai/agent_display.gobridges/ai/tools_beeper_docs.gobridges/ai/tool_schema_sanitize_test.gobridges/ai/portal_materialize.gobridges/ai/streaming_success.gobridges/ai/integrations.gobridges/ai/queue_policy_runtime_test.gobridges/ai/identifiers.gobridges/ai/bootstrap_context_test.gobridges/ai/subagent_spawn.gobridges/ai/login_test.gobridges/ai/tool_approvals.gobridges/ai/chat_bootstrap_test.gobridges/ai/canonical_history.gobridges/ai/commands_test.gobridges/ai/logout_cleanup.gobridges/ai/internal_dispatch.gobridges/ai/delete_chat.gobridges/ai/test_login_helpers_test.gobridges/ai/login_config_db.gobridges/ai/queue_status_test.gobridges/ai/prompt_builder.gobridges/ai/metadata.gobridges/ai/message_helpers.gobridges/ai/login_loaders.gobridges/ai/persistence_boundaries_test.gobridges/ai/heartbeat_events.gobridges/ai/queue_runtime.gobridges/ai/login_loaders_test.gobridges/ai/commands.gobridges/ai/turn_store.gobridges/ai/streaming_persistence.gobridges/ai/bridge_db.gobridges/ai/handleai.gobridges/ai/heartbeat_execute.gobridges/ai/chat.gobridges/ai/tools_search_fetch.gobridges/ai/client.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: 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/streaming_error_handling.gobridges/ai/commands_parity.gobridges/ai/bootstrap_context.gobridges/ai/agent_display.gobridges/ai/tools_beeper_docs.gobridges/ai/tool_schema_sanitize_test.gobridges/ai/portal_materialize.gobridges/ai/streaming_success.gobridges/ai/integrations.gobridges/ai/queue_policy_runtime_test.gobridges/ai/identifiers.gobridges/ai/bootstrap_context_test.gobridges/ai/subagent_spawn.gobridges/ai/login_test.gobridges/ai/tool_approvals.gobridges/ai/chat_bootstrap_test.gobridges/ai/canonical_history.gobridges/ai/commands_test.gobridges/ai/logout_cleanup.gobridges/ai/internal_dispatch.gobridges/ai/delete_chat.gobridges/ai/test_login_helpers_test.gobridges/ai/login_config_db.gobridges/ai/queue_status_test.gobridges/ai/prompt_builder.gobridges/ai/metadata.gobridges/ai/message_helpers.gobridges/ai/login_loaders.gobridges/ai/persistence_boundaries_test.gobridges/ai/heartbeat_events.gobridges/ai/queue_runtime.gobridges/ai/login_loaders_test.gobridges/ai/commands.gobridges/ai/turn_store.gobridges/ai/streaming_persistence.gobridges/ai/bridge_db.gobridges/ai/handleai.gobridges/ai/heartbeat_execute.gobridges/ai/chat.gobridges/ai/tools_search_fetch.gobridges/ai/client.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/commands_parity.gobridges/ai/bootstrap_context.gobridges/ai/agent_display.gobridges/ai/tools_beeper_docs.gobridges/ai/portal_materialize.gobridges/ai/integrations.gobridges/ai/identifiers.gobridges/ai/bootstrap_context_test.gobridges/ai/subagent_spawn.gobridges/ai/tool_approvals.gobridges/ai/chat_bootstrap_test.gobridges/ai/logout_cleanup.gobridges/codex/approval_runtime.gobridges/ai/internal_dispatch.gobridges/ai/delete_chat.gobridges/ai/login_config_db.gobridges/ai/queue_status_test.gobridges/ai/prompt_builder.gobridges/ai/metadata.gobridges/ai/message_helpers.gobridges/ai/login_loaders.gobridges/ai/persistence_boundaries_test.gobridges/codex/approval_rpc.gobridges/ai/login_loaders_test.gobridges/ai/commands.gobridges/ai/turn_store.gobridges/ai/streaming_persistence.gobridges/ai/bridge_db.gobridges/ai/handleai.gobridges/ai/heartbeat_execute.gobridges/ai/chat.gobridges/ai/client.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/commands_parity.gobridges/ai/portal_materialize.gobridges/ai/integrations.gobridges/ai/tool_approvals.gobridges/ai/chat_bootstrap_test.gobridges/ai/commands_test.gobridges/ai/login_config_db.gobridges/ai/metadata.gobridges/ai/login_loaders.gobridges/ai/login_loaders_test.gobridges/ai/commands.gobridges/ai/handleai.gobridges/ai/heartbeat_execute.gobridges/ai/chat.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/logout_cleanup.gobridges/ai/metadata.go
🔇 Additional comments (30)
.pre-commit-config.yaml (1)
21-32: Good move to root-scoped Go analyzers.Using local hooks with
pass_filenames: falseand./...avoids partial-package analysis and makesgo vet/staticcheckbehavior consistent for repo-wide checks.bridges/ai/streaming_error_handling.go (1)
11-11: Finish-reason mapping migration looks correct.Switching to
sdk.MapFinishReason(...)here is consistent with the SDK consolidation and preserves the existing"stop"terminal-turn behavior.Also applies to: 67-67
bridges/ai/streaming_success.go (1)
10-10:completeStreamingSuccessmapping update is clean.Using
sdk.MapFinishReason(state.finishReason)here aligns with the migration and keeps success finalization semantics intact.Also applies to: 37-37
bridges/ai/tool_schema_sanitize_test.go (1)
3-7: Import update looks correct.
toolspecis required for the new schema-based test and the import block is clean.bridges/ai/integrations.go (2)
16-17: SDK import migration looks correct.Using
github.com/beeper/agentremote/sdkhere is consistent with the room-kind constant migration in this file.
549-557: Fallback room kind update is consistent.Returning
sdk.AIRoomKindAgentinintegrationPortalAIKindmatches the SDK migration and keeps fallback behavior clear.bridges/ai/tools_beeper_docs.go (1)
10-32: Retrieval config migration looks consistent.This keeps
effectiveSearchConfig(ctx)as the single source of Exa settings while moving the tool onto the shared retrieval config type, so the refactor stays nicely contained.bridges/ai/tools_search_fetch.go (3)
22-27: Search/fetch call-site migration is clean.Both tool entrypoints still resolve the effective config from
BridgeToolContextand hand off typed requests directly topkg/retrieval, which keeps the package swap low-risk.Also applies to: 59-70
105-208: Good consolidation of login-token injection.Centralizing the provider/login-config mutation in
applyLoginTokensToRetrievalConfigremoves the old metadata coupling without reintroducing compatibility branches. Based on learnings: For the unreleased AI bridge code under bridges/ai, do not add or propose migration paths, legacy-compatibility fallbacks, or dual-read/backfill logic.
223-268: Config mapping stays structurally aligned.Returning
retrieval.SearchConfig/retrieval.FetchConfigwith value-copied nestedExaandDirectsections keeps the boundary between local config models and retrieval models clear.bridges/codex/approval_runtime.go (8)
1-13: LGTM!Package declaration and imports are appropriate for the approval runtime implementation.
15-30: LGTM!The struct definitions are well-organized with appropriate fields for carrying approval metadata through the pending-data storage.
32-44: LGTM!Nil-safe accessor methods are correctly implemented.
46-67: LGTM!The
Waitmethod correctly combines the wait status with the approval decision, and properly propagates the result back to the turn's approval and tool writer systems.
114-141: LGTM!The
requestSDKApprovalmethod properly handles nil guards, resolves the approval request, registers tracking, emits the request to the appropriate turn context, and returns a well-formed approval handle.
143-157: LGTM!The registration method properly sanitizes string inputs and delegates to the approval flow.
159-172: LGTM!The
waitToolApprovalmethod correctly handles both successful and failed wait scenarios, ensuringFinishResolvedis called in both paths for proper cleanup.
89-106: Clarify expiry computation approach differencesBoth paths compute expiry as
time.Time, so type consistency is not an issue. However, line 93 directly addsttlwithtime.Now().Add(ttl), while line 106 converts to seconds and usessdk.ComputeApprovalExpiry(int(ttl / time.Second)). The utility function includes fallback handling forttl <= 0that the direct approach lacks. Consider whether both code paths should use consistent expiry logic, especially when the turn path doesn't account for invalid/zero TTL values.> Likely an incorrect or invalid review comment.bridges/codex/approval_rpc.go (7)
1-13: LGTM!Package declaration and imports are appropriate for the RPC approval handlers.
15-67: LGTM!The structs and helper functions are well-designed. The approval ID derivation correctly handles JSON-encoded IDs, and the response value mapping covers all expected outcomes.
69-122: LGTM!The
resolveApprovalForActiveTurnmethod has proper mutex handling, defensive validation of turn IDs, and appropriate auto-approval logic for elevated sessions. The ignored unmarshal error is acceptable here since malformed params will safely fail at the active turn lookup.
124-138: LGTM!The
handleApprovalRequestmethod cleanly separates input extraction via callback and properly maps error conditions to appropriate response decisions.
140-184: LGTM!The command approval handler comprehensively parses all relevant parameters and builds appropriate approval presentation with session-level approval support.
186-204: LGTM!File change approval handler is clean and focused with appropriate session approval support.
206-238: LGTM!The permissions approval handler correctly returns the requested permissions only when approved, with appropriate scope escalation to "session" when the user grants persistent approval.
bridges/ai/internal_dispatch.go (1)
42-76: Nice consolidation of internal turn persistence and queue dispatch.Routing this through
persistAIInternalPromptTurnanddispatchOrQueueremoves duplicate orchestration while keeping the prompt-building flow intact.bridges/ai/chat_bootstrap_test.go (1)
53-107: Good coverage for the config-driven default-chat behavior.These cases pin both the disabled-by-default agents gate and the “reuse an existing visible chat” path, which are the easiest regressions to miss in this refactor.
bridges/ai/login_loaders_test.go (1)
73-161: Good coverage for persisted login-config loading.The new cases lock down the rebuild decision, persisted-config reload, and runtime bridge-ID fallback that
loadAIUserLoginnow depends on.bridges/ai/streaming_persistence.go (1)
84-139: Nice move to the shared assistant-turn persistence path.Building the SDK metadata bundle and persisting through
persistAIConversationMessagekeeps the streaming path aligned with the rest of the AI turn store.bridges/ai/heartbeat_events.go (1)
61-71: The canonical login keying here is a solid cleanup.Scoping the cache and persister by canonical bridge/login identifiers matches the new runtime-state persistence model and avoids coupling heartbeat state to mutable metadata.
Also applies to: 147-209
| if !strings.HasPrefix(strings.TrimSpace(f.MimeType), "image/") || strings.TrimSpace(f.URL) == "" { | ||
| continue |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the file and surrounding context
cat -n bridges/ai/canonical_history.go | head -60Repository: beeper/agentremote
Length of output: 2228
🏁 Script executed:
# Search for other MIME type handling patterns in the codebase
rg "MimeType" -B 2 -A 2Repository: beeper/agentremote
Length of output: 44759
🏁 Script executed:
# Check for any existing MIME type validation helpers
rg -i "mime" --type go | head -30Repository: beeper/agentremote
Length of output: 2603
Use the standard MIME type helper for case-insensitive matching.
strings.HasPrefix(strings.TrimSpace(f.MimeType), "image/") is case-sensitive, so mixed-case values like Image/PNG will be skipped. The codebase provides stringutil.NormalizeMimeType() which lowercases and trims MIME types according to RFC standards. This helper is consistently used throughout the bridges for MIME comparisons.
Suggested fix
+ mimeType := stringutil.NormalizeMimeType(f.MimeType)
- if !strings.HasPrefix(strings.TrimSpace(f.MimeType), "image/") || strings.TrimSpace(f.URL) == "" {
+ if !strings.HasPrefix(mimeType, "image/") || strings.TrimSpace(f.URL) == "" {📝 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.
| if !strings.HasPrefix(strings.TrimSpace(f.MimeType), "image/") || strings.TrimSpace(f.URL) == "" { | |
| continue | |
| mimeType := stringutil.NormalizeMimeType(f.MimeType) | |
| if !strings.HasPrefix(mimeType, "image/") || strings.TrimSpace(f.URL) == "" { | |
| continue |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/canonical_history.go` around lines 36 - 37, Replace the
case-sensitive MIME check with the standardized helper: call
stringutil.NormalizeMimeType on f.MimeType and use strings.HasPrefix on the
normalized value (instead of strings.HasPrefix(strings.TrimSpace(f.MimeType),
"image/")) while keeping the existing f.URL empty check; update the condition in
canonical_history.go (the block referencing f.MimeType and f.URL) so mixed-case
MIME values like "Image/PNG" are accepted.
| var pmeta *PortalMetadata | ||
| if opts.CopyFrom != nil { | ||
| pmeta = cloneForkPortalMetadata(opts.CopyFrom, slug, title) | ||
| } else { | ||
| pmeta = &PortalMetadata{ | ||
| Slug: slug, | ||
| Title: title, | ||
| Slug: slug, | ||
| } | ||
| } | ||
| portal.Metadata = pmeta | ||
|
|
||
| if err := agentremote.ConfigureDMPortal(ctx, agentremote.ConfigureDMPortalParams{ | ||
| Portal: portal, | ||
| chatInfo := oc.composeChatInfo(ctx, title, modelID) | ||
| result, err := sdk.BootstrapDMPortal(ctx, sdk.DMPortalBootstrapSpec{ | ||
| Login: oc.UserLogin, | ||
| PortalKey: portalKey, | ||
| Title: title, | ||
| OtherUserID: modelUserID(modelID), | ||
| Save: true, | ||
| MutatePortal: func(portal *bridgev2.Portal) { | ||
| PortalMutate: func(portal *bridgev2.Portal) { | ||
| portal.Metadata = pmeta | ||
| defaultAvatar := strings.TrimSpace(agents.DefaultAgentAvatarMXC) | ||
| if defaultAvatar != "" { | ||
| portal.AvatarID = networkid.AvatarID(defaultAvatar) | ||
| portal.AvatarMXC = id.ContentURIString(defaultAvatar) | ||
| } | ||
| }, | ||
| }); err != nil { | ||
| return nil, nil, fmt.Errorf("failed to save portal: %w", err) | ||
| BeforeSave: func(ctx context.Context, portal *bridgev2.Portal) error { | ||
| return saveAIPortalState(ctx, portal, pmeta) | ||
| }, |
There was a problem hiding this comment.
Persist the initial resolved target before BeforeSave runs.
For brand-new chats, pmeta only carries Slug, but saveAIPortalState() is invoked from BeforeSave. That means the canonical AI portal-state row is created without the model target, so a reload can no longer recover the room's model from sidecar state.
Based on learnings: For the unreleased AI bridge code under bridges/ai, the implementation should use only the new sidecar/state approach with no metadata/portal-state fallbacks.
| meta.SessionResetAt = time.Now().UnixMilli() | ||
| if err := advanceAIPortalContextEpoch(ce.Ctx, ce.Portal); err != nil { | ||
| client.log.Warn().Err(err).Stringer("portal", ce.Portal.PortalKey).Msg("Failed to advance AI context epoch during reset") | ||
| ce.Reply("%s", formatSystemAck("Failed to reset session.")) | ||
| return |
There was a problem hiding this comment.
Only stamp SessionResetAt after the epoch bump succeeds.
If advanceAIPortalContextEpoch fails, this path returns “Failed to reset session” but leaves meta.SessionResetAt mutated in memory. A later save of the same PortalMetadata can then persist a reset timestamp without the matching context-epoch advance, leaving reset markers and visible history out of sync.
Suggested fix
- meta.SessionResetAt = time.Now().UnixMilli()
if err := advanceAIPortalContextEpoch(ce.Ctx, ce.Portal); err != nil {
client.log.Warn().Err(err).Stringer("portal", ce.Portal.PortalKey).Msg("Failed to advance AI context epoch during reset")
ce.Reply("%s", formatSystemAck("Failed to reset session."))
return
}
+ meta.SessionResetAt = time.Now().UnixMilli()
client.savePortalQuiet(ce.Ctx, ce.Portal, "session reset")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/commands_parity.go` around lines 46 - 50, The code sets
meta.SessionResetAt before calling advanceAIPortalContextEpoch, so if
advanceAIPortalContextEpoch(ce.Ctx, ce.Portal) fails the in-memory
meta.SessionResetAt is mutated; move the assignment of meta.SessionResetAt =
time.Now().UnixMilli() to after advanceAIPortalContextEpoch returns successfully
(i.e., only set SessionResetAt once advanceAIPortalContextEpoch(ce.Ctx,
ce.Portal) completes without error) and keep the existing error handling path
(client.log.Warn... and ce.Reply...) unchanged so failed advances do not leave a
stale reset timestamp in memory or persisted state.
| func (oc *AIClient) updateLoginConfig(ctx context.Context, fn func(*aiLoginConfig) bool) error { | ||
| if oc == nil || oc.UserLogin == nil { | ||
| return nil | ||
| } | ||
| oc.loginConfigMu.Lock() | ||
| defer oc.loginConfigMu.Unlock() | ||
| if oc.loginConfig == nil { | ||
| cfg, err := loadAILoginConfig(ctx, oc.UserLogin) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| oc.loginConfig = cfg | ||
| } | ||
| if !fn(oc.loginConfig) { | ||
| return nil | ||
| } | ||
| return saveAILoginConfig(ctx, oc.UserLogin, oc.loginConfig) |
There was a problem hiding this comment.
updateLoginConfig deadlocks on successful writes.
This function holds loginConfigMu while calling saveAILoginConfig(). That helper re-enters the same mutex through login.Client.(*AIClient), so any real config change hangs after the DB write.
Suggested direction
func (oc *AIClient) updateLoginConfig(ctx context.Context, fn func(*aiLoginConfig) bool) error {
if oc == nil || oc.UserLogin == nil {
return nil
}
oc.loginConfigMu.Lock()
- defer oc.loginConfigMu.Unlock()
if oc.loginConfig == nil {
cfg, err := loadAILoginConfig(ctx, oc.UserLogin)
if err != nil {
+ oc.loginConfigMu.Unlock()
return err
}
oc.loginConfig = cfg
}
if !fn(oc.loginConfig) {
+ oc.loginConfigMu.Unlock()
return nil
}
- return saveAILoginConfig(ctx, oc.UserLogin, oc.loginConfig)
+ cfg := cloneAILoginConfig(oc.loginConfig)
+ oc.loginConfigMu.Unlock()
+ return saveAILoginConfig(ctx, oc.UserLogin, cfg)
}If you want stronger consistency, I'd split saveAILoginConfig into a DB-only helper and keep the cache update outside the locked section.
🤖 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 166 - 182, updateLoginConfig
currently holds loginConfigMu while calling saveAILoginConfig, causing a
deadlock because saveAILoginConfig can re-enter via login.Client.(*AIClient);
change the flow so the mutex is not held during DB writes: call
loadAILoginConfig and run fn under loginConfigMu to determine if the config
changed, capture the modified aiLoginConfig (or a dirty flag), then release
loginConfigMu before calling the persistence helper (or a new DB-only
saveAILoginConfigDB helper); after the DB write, re-acquire loginConfigMu to
update oc.loginConfig if needed. Ensure references to oc.loginConfig,
oc.loginConfigMu, updateLoginConfig, saveAILoginConfig, loadAILoginConfig,
UserLogin and aiLoginConfig are used to locate and apply the change.
| func (oc *AIClient) releaseRoom(roomID id.RoomID) { | ||
| oc.roomLocksMu.Lock() | ||
| defer oc.roomLocksMu.Unlock() | ||
| delete(oc.roomLocks, roomID) | ||
| oc.clearRoomRun(roomID) | ||
| } |
There was a problem hiding this comment.
Don't hold roomLocksMu across ack cleanup.
releaseRoom() keeps the global room-lock mutex held while clearRoomRun() removes pending ack reactions. Those removals can block on Matrix I/O, so one slow room release can stall acquireRoom() / releaseRoom() for every other room.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/queue_runtime.go` around lines 61 - 66, releaseRoom currently
holds roomLocksMu while calling clearRoomRun (which does Matrix I/O and can
block), so move the ack cleanup outside the lock: acquire roomLocksMu, delete
oc.roomLocks[roomID] (and capture any needed local state if required), then
unlock roomLocksMu before calling oc.clearRoomRun(roomID); ensure you do not
access oc.roomLocks after releasing the lock. This change keeps the critical
section minimal (only the delete) and prevents a slow clearRoomRun from blocking
acquireRoom/releaseRoom for other rooms.
| roomBusy := oc.roomHasActiveRun(roomID) || oc.roomHasPendingQueueWork(roomID) | ||
| queueDecision := airuntime.DecideQueueAction(queueSettings.Mode, roomBusy, false) | ||
| if queueDecision.Action == airuntime.QueueActionInterruptAndRun { | ||
| oc.cancelRoomRun(roomID) | ||
| oc.clearPendingQueue(ctx, roomID) | ||
| roomBusy = false | ||
| } | ||
| if !roomBusy && oc.acquireRoom(roomID) { | ||
| oc.stopQueueTyping(roomID) | ||
| if hasDBMessage { | ||
| oc.saveUserMessage(ctx, evt, userMessage) | ||
| } | ||
| if evt != nil && !queueItem.pending.PendingSent { | ||
| oc.sendPendingStatus(ctx, portal, evt, "Processing...") | ||
| queueItem.pending.PendingSent = true | ||
| } | ||
| runCtx := oc.backgroundContext(ctx) | ||
| if len(queueItem.pending.StatusEvents) > 0 { | ||
| runCtx = context.WithValue(runCtx, statusEventsKey{}, queueItem.pending.StatusEvents) | ||
| } | ||
| if queueItem.pending.InboundContext != nil { | ||
| runCtx = withInboundContext(runCtx, *queueItem.pending.InboundContext) | ||
| } | ||
| if queueItem.pending.Typing != nil { | ||
| runCtx = WithTypingContext(runCtx, queueItem.pending.Typing) | ||
| } | ||
| runCtx = oc.attachRoomRun(runCtx, roomID) |
There was a problem hiding this comment.
Queue policy can misclassify a room as idle.
roomBusy only checks activeRoomRuns and pending-queue state, but the real exclusivity guard is roomLocks. Since attachRoomRun() doesn't happen until Line 170, a concurrent message can compute the non-busy action, lose acquireRoom(), and skip the intended interrupt/drop behavior for that queue mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/queue_runtime.go` around lines 144 - 170, The queue decision can
misclassify a room as idle because roomBusy is computed only from
oc.roomHasActiveRun and oc.roomHasPendingQueueWork but not the real exclusivity
guard (roomLocks), allowing a race between queueDecision and
acquireRoom/attachRoomRun; fix by including the room lock state in the busy
check (e.g., consult oc.roomLocks or expose a helper like
oc.roomIsLocked(roomID)) or alternatively re-check lock after acquireRoom and
before acting on queueDecision — update the logic around
roomHasActiveRun/roomHasPendingQueueWork, queueDecision, acquireRoom, and
attachRoomRun so the exclusivity guard (roomLocks) is considered before deciding
InterruptAndRun or Drop (referenced symbols: roomHasActiveRun,
roomHasPendingQueueWork, roomLocks/roomIsLocked, DecideQueueAction, acquireRoom,
attachRoomRun, cancelRoomRun, clearPendingQueue).
| func normalizeApprovalToken(s string) string { | ||
| return strings.ToLower(strings.TrimSpace(s)) | ||
| } | ||
|
|
||
| func normalizeMcpRuleToolName(name string) string { | ||
| n := normalizeApprovalToken(name) | ||
| return strings.TrimPrefix(n, "mcp.") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the file context around lines 108-115 to understand usage
cat -n bridges/ai/tool_approvals.go | head -220 | tail -120Repository: beeper/agentremote
Length of output: 4273
🌐 Web query:
MCP Model Context Protocol tool names case sensitivity specification
💡 Result:
In the Model Context Protocol (MCP), tool names are case-sensitive. This is specified in SEP-986 (final/accepted status), which states: "Tool names are case-sensitive." Allowed characters include a-z, A-Z, 0-9, _, -, ., /; length 1-64 characters; unique within namespace. Official docs (e.g., 2025-11-25 spec) use "SHOULD be considered case-sensitive." Examples include mixed-case like "DATA_EXPORT_v2". AWS MCP guidelines confirm: "Tool names are case-sensitive (per MCP specification)".
Citations:
- 1: SEP-986: Specify Format for Tool Names modelcontextprotocol/modelcontextprotocol#986
- 2: https://modelcontextprotocol.io/seps/986-specify-format-for-tool-names
- 3: https://modelcontextprotocol.io/specification/2025-11-25/server/tools
- 4: https://github.com/awslabs/mcp/blob/main/DESIGN_GUIDELINES.md
Preserve MCP tool name casing in approval rules.
normalizeMcpRuleToolName incorrectly lowercases tool names before persisting or matching "always allow" rules. MCP tool names are case-sensitive per specification, so this allows an approval granted for ReadFile to auto-match a distinct READFILE tool later, broadening permissions unintentionally. Strip the synthetic mcp. prefix while preserving the original tool casing.
Suggested fix
func normalizeMcpRuleToolName(name string) string {
- n := normalizeApprovalToken(name)
- return strings.TrimPrefix(n, "mcp.")
+ n := strings.TrimSpace(name)
+ if len(n) >= 4 && strings.EqualFold(n[:4], "mcp.") {
+ n = n[4:]
+ }
+ return n
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/tool_approvals.go` around lines 108 - 115, The function
normalizeMcpRuleToolName currently calls normalizeApprovalToken which lowercases
tool names; change it to preserve casing by trimming whitespace only and
removing a leading "mcp." prefix without lowercasing. Concretely, update
normalizeMcpRuleToolName to perform strings.TrimSpace(name) and then strip the
"mcp." prefix (e.g., via strings.HasPrefix + strings.TrimPrefix) so the returned
tool name keeps its original case while still removing the synthetic prefix; do
not call normalizeApprovalToken from normalizeMcpRuleToolName.
| func allocateAITurnSequence(ctx context.Context, scope *portalScope) (contextEpoch, sequence int64, err error) { | ||
| record, err := ensurePortalTurnStateByScope(ctx, scope) | ||
| if err != nil { | ||
| return 0, 0, err | ||
| } | ||
| contextEpoch = record.ContextEpoch | ||
| sequence = record.NextTurnSequence + 1 | ||
| _, err = scope.db.Exec(ctx, ` | ||
| UPDATE `+aiPortalStateTable+` | ||
| SET next_turn_sequence=$4, updated_at_ms=$5 | ||
| WHERE bridge_id=$1 AND portal_id=$2 AND portal_receiver=$3 | ||
| `, scope.bridgeID, scope.portalID, scope.portalReceiver, sequence, time.Now().UnixMilli()) | ||
| return contextEpoch, sequence, err |
There was a problem hiding this comment.
Allocate turn sequence numbers atomically.
Two concurrent writers can both read the same NextTurnSequence, both assign the same sequence, and then persist turns with duplicate ordering. That breaks checkpointing and history replay even though the inserts succeed.
Suggested direction
Use a single SQL increment step that returns the new sequence, instead of a read/modify/write in Go. For example, update next_turn_sequence = next_turn_sequence + 1 and read back the incremented value in the same statement/transaction, retrying only if the portal-state row does not exist yet.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/turn_store.go` around lines 96 - 108, The current
allocateAITurnSequence reads NextTurnSequence in Go then writes back, causing
race conditions; change it to perform a single atomic SQL update that increments
and returns the new sequence (e.g. UPDATE <aiPortalStateTable> SET
next_turn_sequence = next_turn_sequence + 1, updated_at_ms = $X RETURNING
next_turn_sequence, context_epoch) and use that returned value as
sequence/contextEpoch instead of reading then writing; ensure you still handle
the "row missing" case by calling ensurePortalTurnStateByScope or inserting the
portal-state row and retrying the single-statement UPDATE if needed, and replace
the scope.db.Exec usage with a query that reads the RETURNING values so
allocateAITurnSequence (and references to scope.db, aiPortalStateTable,
ensurePortalTurnStateByScope, and the sequence/contextEpoch locals) atomically
obtains the incremented sequence.
1 participant
No description provided.