feat: clean up AI booboos, fix bridgev2 usage#103
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
WalkthroughLarge-scale refactor migrating bridges to github.com/beeper/agentremote/sdk, reworking AI Chats core: new DB-backed turn store, login config/state persistence, queue/runtime orchestration, portal/materialization, provisioning, approval flow, and heartbeat routing. Codex and DummyBridge updated to the sdk and to new state/persistence paths. Tests and workflows renamed and aligned. ChangesAI Chats core refactor (SDK migration, state/turn store, queue, messaging, heartbeat, provisioning)
Codex refactor (SDK migration, portal state, approvals)
DummyBridge SDK migration
AI Chats feature-level adjustments (chat/commands/desktop)
DB scoped helpers and cleanup
Housekeeping and branding
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Matrix as Matrix Room
participant Client as AIClient
participant DB as AI DB (turns/state)
participant SDK as SDK Messaging
participant Provider as Model Provider
User->>Matrix: Send message
Matrix->>Client: HandleMatrixMessage
Client->>DB: Save user DB message (optional)
Client->>Client: resolve queue settings / busy state
alt can run now
Client->>Client: buildPromptContextForPendingMessage
Client->>DB: Persist canonical turn (user)
Client->>Provider: Dispatch prompt (stream/run)
Provider-->>Client: Stream/results
Client->>DB: Upsert assistant turn
Client->>SDK: Send statuses (success)
else enqueue
Client->>DB: Enqueue pending item
Client->>SDK: Send statuses (queued)
Client->>Client: processPendingQueue (deferred)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Rename user-facing references to "AgentRemote CLI" across workflows, README, docker docs, help text, and Homebrew cask generation. Introduce defaultCodexClientInfoName/title constants, apply them in the Codex connector and add a unit test to assert defaults. Add a user-agent base constant for OpenClaw Gateway and use it when building client identity and tests. Minor prompt and help string tweaks to clarify wording and usage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
bridges/ai/command_registry.go (1)
197-210:⚠️ Potential issue | 🟠 MajorRemove dead code
buildCommandDescriptionContentand its test.The
BroadcastCommandDescriptionsmethod no longer uses this function—it now directly constructssdk.Commandobjects instead. The only remaining usage is inevents_test.go:51, which tests the unused function directly. Remove both the function definition and the test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/command_registry.go` around lines 197 - 210, Remove the dead helper buildCommandDescriptionContent and its associated unit test that directly invokes it (the test in events_test that targets buildCommandDescriptionContent); update references so BroadcastCommandDescriptions continues to construct sdk.Command objects directly and delete both the function buildCommandDescriptionContent and the test file's test case that calls it. Ensure no other code references buildCommandDescriptionContent remain and run the test suite to confirm removal is safe.sdk/conversation_state.go (2)
75-106:⚠️ Potential issue | 🟠 MajorAvoid caching conversation state under the empty key.
When
portal.Portal == nil,conversationStateKeyreturns"", butgetandsetstill read/writerooms[""]. That lets unrelated partially initialized portals share cached state.Suggested fix
func (s *conversationStateStore) get(portal *bridgev2.Portal) *sdkConversationState { if s == nil || portal == nil { return &sdkConversationState{} } key := conversationStateKey(portal) + if key == "" { + return &sdkConversationState{} + } s.mu.RLock() state := s.rooms[key] s.mu.RUnlock() if state != nil { return state.clone() @@ func (s *conversationStateStore) set(portal *bridgev2.Portal, state *sdkConversationState) { if s == nil || portal == nil { return } key := conversationStateKey(portal) + if key == "" { + return + } s.mu.Lock() s.rooms[key] = state.clone() s.mu.Unlock() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/conversation_state.go` around lines 75 - 106, The code currently allows storing/reading under an empty key when conversationStateKey(portal) returns "", causing unrelated portals to share state; update conversationStateStore.get and conversationStateStore.set to treat an empty key as invalid: after computing key := conversationStateKey(portal) return a new empty sdkConversationState (without accessing s.rooms) if key == "" in get, and return immediately without writing to s.rooms if key == "" in set; reference conversationStateKey, conversationStateStore.get, conversationStateStore.set, s.rooms and ensure the same nil checks remain.
134-176:⚠️ Potential issue | 🔴 CriticalAdd a legacy-state fallback before switching reads to DB-only storage.
loadConversationStatenow consults only the cache andsdk_conversation_statetable. Existing deployments that still have conversation state in portal metadata will silently load defaults until something rewrites the room—a regression unless a migration backfills the data or a fallback read from legacy storage is implemented.The test explicitly validates that
portal.Metadataremains untouched after saving (line 65-66), confirming the migration path doesn't write to the old storage location. No backfill or legacy read fallback is present in the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/conversation_state.go` around lines 134 - 176, loadConversationState currently only checks the cache and DB, so add a legacy fallback: when state is still nil/default after consulting the store and DB (inside loadConversationState), inspect portal.Metadata for the legacy conversation-state entry (the key used previously for storing state in portal metadata), unmarshal that JSON into an sdkConversationState, and use it as the loaded state (without mutating portal.Metadata). After loading from legacy metadata, call state.ensureDefaults() and store.set(portal, state) as currently done so the rest of the code treats it like a normal load; keep loadConversationStateFromDB unchanged.pkg/aidb/db_test.go (1)
53-68:⚠️ Potential issue | 🟡 MinorAssert the rest of the new v1 tables here too.
pkg/aidb/001-init.sqlnow createsaichats_internal_messages,aichats_login_state, andaichats_tool_approval_rulesas part of the fresh schema, but this test doesn't verify them. If upgrade stops creating any of those tables,TestUpgradeV1Freshwill still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/aidb/db_test.go` around lines 53 - 68, TestUpgradeV1Fresh is missing assertions for newly added v1 tables; update the test (TestUpgradeV1Fresh in pkg/aidb/db_test.go) to include checks for the three tables created by pkg/aidb/001-init.sql: aichats_internal_messages, aichats_login_state, and aichats_tool_approval_rules so the fresh-schema path fails if any of those tables are not created during upgrade.sdk/conversation.go (1)
47-52:⚠️ Potential issue | 🔴 CriticalRestore the nil receiver guard in
getIntent.Line 51 now dereferences
cunconditionally. A nil*Conversationwill panic here before returning an error, and that propagates intoSendMedia,SendNotice,sendMessageContent, andSetTyping.🐛 Proposed fix
func (c *Conversation) getIntent(ctx context.Context) (bridgev2.MatrixAPI, error) { + if c == nil { + return nil, fmt.Errorf("conversation unavailable") + } if c != nil && c.intentOverride != nil { return c.intentOverride(ctx) } return resolveMatrixIntent(ctx, c.login, c.portal, c.sender, bridgev2.RemoteEventMessage) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/conversation.go` around lines 47 - 52, The getIntent method on Conversation now dereferences c unconditionally which will panic for a nil *Conversation and cascade into SendMedia, SendNotice, sendMessageContent, and SetTyping; restore the nil receiver guard by checking if c == nil at the top of Conversation.getIntent and return a sensible error (e.g., fmt.Errorf or a package-level err like ErrNoConversation) instead of calling resolveMatrixIntent, and keep the existing intentOverride branch (i.e., if c != nil && c.intentOverride != nil return c.intentOverride(ctx)); ensure callers that expect an error continue to propagate it.bridges/ai/login.go (1)
239-248:⚠️ Potential issue | 🟠 MajorAvoid returning an error after a successful
NewLoginwithout rollback.
ol.User.NewLogin(...)has already created the login beforesaveAIUserLogin(...)runs. If the second write fails, the user sees a failed login while the new login still exists, which can strand state and make retries/relogins produce duplicate or confusing accounts. Please make these writes atomic, or explicitly clean up the just-created login before returning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/login.go` around lines 239 - 248, The code calls ol.User.NewLogin(...) which creates a login and then calls saveAIUserLogin(...); if saveAIUserLogin fails you must roll back the created login to avoid stranded state. Modify the error path after saveAIUserLogin to call the appropriate deletion method (e.g., ol.User.DeleteLogin(ctx, loginID) or equivalent) to remove the created login (and handle/log any deletion error), then return the original wrapped save error; ensure you reference ol.User.NewLogin, saveAIUserLogin, and loginID when implementing the rollback so creation is undone on failure.bridges/ai/tool_approvals_rules.go (1)
59-80:⚠️ Potential issue | 🟠 MajorDon't use
context.Background()for approval-rule lookups.These checks are on the tool/approval path now that they hit the DB. Using
context.Background()makes them uncancellable, so a slow or wedged DB can stall message handling indefinitely. Please thread the caller context through these methods, or wrap the queries in a short timeout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/tool_approvals_rules.go` around lines 59 - 80, The approval checks isMcpAlwaysAllowed and isBuiltinAlwaysAllowed currently call hasToolApprovalRule/hasBuiltinToolApprovalRule with context.Background(), making DB lookups uncancellable; change these functions to accept a context.Context parameter (e.g., ctx) and pass that ctx into oc.hasToolApprovalRule and oc.hasBuiltinToolApprovalRule, or alternatively create a derived context with a short timeout inside each function and use that for the DB call; update all callers of isMcpAlwaysAllowed and isBuiltinAlwaysAllowed to supply the caller context so the DB queries are cancellable.bridges/ai/chat.go (1)
1095-1132:⚠️ Potential issue | 🟠 MajorKeep a legacy-room fallback before creating a new default chat.
This path now only checks
defaultChatPortalKey(...). For existing logins whose default AI room was created under an older key/selection path, bootstrap will create a second default room instead of reusing the current one.Falling back to
listAllChatPortals(...)/chooseDefaultChatPortal(...)before creating a new portal would preserve existing rooms during upgrade.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/chat.go` around lines 1095 - 1132, Before creating a new default portal, add a legacy-room fallback: after the GetExistingPortalByKey check fails (portal == nil) call oc.listAllChatPortals(ctx, oc.UserLogin) and pass the result into oc.chooseDefaultChatPortal(...) to see if an older portal should be reused; if chooseDefaultChatPortal returns a portal, reuse it (materialize if MXID missing via oc.materializePortalRoom) and return nil instead of proceeding to create a new portal with oc.initPortalForChat; keep existing logging behavior and error handling around materializePortalRoom and initPortalForChat, and reference the existing symbols GetExistingPortalByKey, listAllChatPortals, chooseDefaultChatPortal, materializePortalRoom, and initPortalForChat when making the change.bridges/codex/directory_manager.go (1)
133-164:⚠️ Potential issue | 🟠 MajorSave the welcome-room state before creating the Matrix room.
EnsurePortalLifecycle(...)can succeed beforesaveCodexPortalState(...)runs. If the save fails afterward, the room already exists but there is no persistedcodexPortalStateto find or repair it on restart.Suggested ordering
state := &codexPortalState{ Title: "New Codex Chat", Slug: "codex-welcome", AwaitingCwdSetup: true, } portalMeta(portal).IsCodexRoom = true + if err := saveCodexPortalState(ctx, portal, state); err != nil { + return nil, err + } if err := sdk.ConfigureDMPortal(ctx, sdk.ConfigureDMPortalParams{ Portal: portal, Title: state.Title, OtherUserID: codexGhostID, Save: false, }); err != nil { return nil, err } info := cc.composeCodexChatInfo(portal, state, false) created, err := sdk.EnsurePortalLifecycle(ctx, sdk.PortalLifecycleOptions{ Login: cc.UserLogin, Portal: portal, ChatInfo: info, SaveBeforeCreate: true, AIRoomKind: sdk.AIRoomKindAgent, ForceCapabilities: true, }) if err != nil { return nil, err } - if err := saveCodexPortalState(ctx, portal, state); err != nil { - return nil, err - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/codex/directory_manager.go` around lines 133 - 164, The current flow calls sdk.EnsurePortalLifecycle (in bridges/codex/directory_manager.go) before persisting codexPortalState with saveCodexPortalState, which can create the Matrix room without saved state; move the saveCodexPortalState call to occur immediately after initializing the codexPortalState and before calling sdk.EnsurePortalLifecycle, so the state is persisted (and errors returned) prior to creating the room; keep the calls to portalMeta(portal).IsCodexRoom = true and sdk.ConfigureDMPortal/cc.composeCodexChatInfo/sendSystemNotice intact, but ensure saveCodexPortalState is executed and its error handled before invoking EnsurePortalLifecycle to avoid orphaned rooms without persisted state.
🧹 Nitpick comments (13)
cmd/internal/bridgeentry/bridgeentry.go (1)
21-50: Inconsistent description format for AI bridge.The
AIdefinition uses a different description format ("AI Chats bridge for Beeper") compared to all other bridges which follow the pattern "X bridge built with the AgentRemote SDK." If this is intentional, consider adding a comment explaining why. Otherwise, align the format for consistency.AI = Definition{ Name: "ai", - Description: "AI Chats bridge for Beeper", + Description: "AI Chats bridge built with the AgentRemote SDK.", Port: 29345, DBName: "ai.db", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/internal/bridgeentry/bridgeentry.go` around lines 21 - 50, The AI Definition's Description ("AI Chats bridge for Beeper") is inconsistent with the other bridge Definitions (Codex, OpenCode, OpenClaw, DummyBridge) which use the "X bridge built with the AgentRemote SDK." pattern; update the AI variable's Description in the Definition for AI to match that pattern (e.g., "AI bridge built with the AgentRemote SDK.") or, if the different wording is intentional, add a brief inline comment above the AI Definition explaining the special case so reviewers understand the deviation.cmd/agentremote/profile.go (1)
26-42: Comments reference hardcoded "sdk" path but code uses dynamicbinaryName.The comments on lines 26 and 35 hardcode "sdk" in the path description, but the actual code uses the
binaryNamevariable. IfbinaryNamechanges in the future, these comments will be misleading.📝 Suggested fix to make comments more accurate
-// configRoot returns ~/.config/sdk +// configRoot returns ~/.config/<binaryName> func configRoot() (string, error) { home, err := os.UserHomeDir() if err != nil { return "", err } return filepath.Join(home, ".config", binaryName), nil } -// profileRoot returns ~/.config/sdk/profiles/<profile> +// profileRoot returns ~/.config/<binaryName>/profiles/<profile> func profileRoot(profile string) (string, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/agentremote/profile.go` around lines 26 - 42, Update the doc comments for configRoot and profileRoot to reflect that the path uses the dynamic binaryName variable rather than the hardcoded "sdk" string: change the comment above configRoot to indicate it returns ~/.config/<binaryName> and the comment above profileRoot to indicate it returns ~/.config/<binaryName>/profiles/<profile>, referencing the functions configRoot and profileRoot so future readers know the path is derived from binaryName.bridges/opencode/opencode_instance_state.go (1)
100-110: Consider deterministic ordering insessionIDs().Go map iteration order is non-deterministic; sorting
outbefore return can reduce flaky ordering in callers/tests/logs.Suggested refactor
func (inst *openCodeInstance) sessionIDs() []string { if inst == nil { return nil } inst.seenMu.Lock() defer inst.seenMu.Unlock() out := make([]string, 0, len(inst.knownSessions)) for sessionID := range inst.knownSessions { out = append(out, sessionID) } + sort.Strings(out) return out }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/opencode/opencode_instance_state.go` around lines 100 - 110, The sessionIDs() method returns session IDs in non-deterministic map order; lock seenMu, collect keys from inst.knownSessions as done, then sort the slice (e.g., using sort.Strings) before returning to ensure deterministic ordering for callers, tests, and logs; update the function (sessionIDs) to import the sort package and call sort.Strings(out) right before the return.pkg/shared/toolspec/toolspec.go (1)
143-145: Encode theemojirequirement in the schema, not just the description.Line 144 now says
emojiis required whenremove:true, but the schema still accepts a reaction-removal payload without it. If anything downstream relies on schema validation, malformedreactrequests will still get through. Consider adding anif/thenoroneOfrule for thereactaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/shared/toolspec/toolspec.go` around lines 143 - 145, The schema currently documents that "emoji" is required when "remove": true for the "react" action but doesn't enforce it; update the toolspec schema construction in toolspec.go (the code building properties like StringProperty("emoji") and BooleanProperty("remove") for the "react" action) to add a conditional JSON Schema rule that enforces emoji when remove is true — for example add an if/then clause scoped to action === "react" (if: {properties:{action:{const:"react"}, remove:{const:true}}} then: {required:["emoji"]}) or express the same via oneOf alternatives; ensure the new rule is attached to the same schema object used by the validator so malformed react/remove payloads are rejected.bridges/ai/identifiers.go (1)
171-171: Thread caller context throughportalMeta()to propagate cancellation for DB-backed portal state loads.Line 171 uses
context.Background()when callingloadPortalStateIntoMetadata(), preventing request cancellation/timeouts from propagating to the database operation. SinceportalMeta()is an accessor-style helper with 50+ call sites across the codebase, refactoring it to accept a context parameter would require updating all callers, though many already have context available in their call path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/identifiers.go` at line 171, The call to loadPortalStateIntoMetadata is using context.Background() inside portalMeta(), preventing cancellation/timeouts from propagating; change portalMeta to accept a context.Context parameter and propagate that ctx into loadPortalStateIntoMetadata (i.e., call loadPortalStateIntoMetadata(ctx, portal, meta) instead of using Background()), then update portalMeta call sites to pass the available request/context where present (and only use context.Background() as an explicit fallback in rare callers that truly lack a context), ensuring all DB-backed portal state loads honor cancellation.bridges/ai/logout_cleanup.go (2)
60-71: Inconsistent table name usage.Line 45 uses the
aiSessionsTablevariable for the sessions table, but these new deletions use hardcoded table names (aichats_internal_messages,aichats_tool_approval_rules,aichats_login_state). Consider using variables consistently for all AI-related tables to centralize table name definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/logout_cleanup.go` around lines 60 - 71, The three DELETEs use hardcoded table names; replace them with the centralized table-name variables (like the existing aiSessionsTable pattern) so table names are consistent and configurable; update the bestEffortExec calls to use the corresponding variables (e.g., internalMessagesTable, toolApprovalRulesTable, loginStateTable or whatever names are defined in the same package), and if those variables don't exist add them alongside aiSessionsTable where other AI table constants are declared so all DELETEs reference the shared table-name variables rather than literal strings.
72-74: Consider consolidating duplicate type assertion.The
(*AIClient)type assertion is performed twice: once at line 36 forpurgeLoginIntegrationsand again here at line 72 forclearLoginState. Consider calling both methods within a single type-assertion block to reduce redundancy.♻️ Proposed consolidation
if client, ok := login.Client.(*AIClient); ok && client != nil { client.purgeLoginIntegrations(ctx, login, bridgeID, loginID) + defer client.clearLoginState(ctx) } var logger *zerolog.LoggerThen remove the second type assertion block at lines 72-74.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/logout_cleanup.go` around lines 72 - 74, The code performs the same type assertion on login.Client to *AIClient twice; consolidate by doing a single assertion (e.g., if client, ok := login.Client.(*AIClient); ok && client != nil { ... }) and call both client.purgeLoginIntegrations(ctx) and client.clearLoginState(ctx) inside that block, then remove the duplicate assertion and the separate clearLoginState call so both methods run under one nil-checked *AIClient branch.sdk/conversation_test.go (1)
47-49: Prefer test-failure signaling overpanicin helper.Optional: pass
*testing.TintonewTestConversationand uset.Fatalf(...)so failures are attributed cleanly to the calling test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/conversation_test.go` around lines 47 - 49, The helper currently panics on save errors which obscures test attribution; modify the test helper (e.g., newTestConversation) to accept a *testing.T (or change it to return an error) and replace the panic(err) after conv.saveState(...) with t.Fatalf("saveState failed: %v", err) so failures are reported as test failures and attributed to the calling test; update all callers of newTestConversation to pass the *testing.T (or handle the returned error) accordingly.pkg/integrations/memory/sessions.go (1)
63-67: Unnecessary state save when hash matches.When the computed hash matches
state.contentHashand!force, the code saves the session state (line 64-66) even though nothing has changed. This appears redundant sincestatehasn't been modified at this point.💡 Suggested simplification
if !force && hash == state.contentHash { - if err := m.saveSessionState(ctx, key, state); err != nil { - m.log.Warn().Err(err).Str("session", key).Msg("memory session state save failed") - } continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/integrations/memory/sessions.go` around lines 63 - 67, The code redundantly calls m.saveSessionState(ctx, key, state) when !force and hash == state.contentHash even though state wasn't modified; remove the save call from that branch (the if-block in sessions.go that checks !force && hash == state.contentHash) so it simply continues, and ensure any necessary saves remain where state is actually mutated (keep m.saveSessionState only in places like saveSessionState invocations after state changes).bridges/ai/login_config_db.go (1)
69-84: Table creation uses safe constant, but consider schema migrations.The DDL uses
CREATE TABLE IF NOT EXISTSwhich is safe for initial creation. SinceaiLoginConfigTableappears to be a constant, there's no SQL injection risk. However, for production systems, consider using formal schema migrations for better version control and rollback capabilities.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/login_config_db.go` around lines 69 - 84, The current ensureAILoginConfigTable function contains inline DDL (CREATE TABLE IF NOT EXISTS) for aiLoginConfigTable; replace this ad-hoc creation with a proper schema migration: remove the inline CREATE TABLE from ensureAILoginConfigTable and instead register a migration (e.g., a new migration entry or function like migrateCreateAILoginConfigTable) that performs the DDL via your project's migration framework or migration runner, ensure the migration is idempotent and versioned, and update any bootstrap code that called ensureAILoginConfigTable to run/verify migrations (refer to symbols ensureAILoginConfigTable and aiLoginConfigTable to locate the code to change).bridges/openclaw/metadata.go (2)
127-143: Consider caching table creation status.
ensureOpenClawPortalStateTableexecutesCREATE TABLE IF NOT EXISTSon every load/save operation. While SQLite/PostgreSQL handle this idempotently, it adds unnecessary overhead.Consider using a
sync.Onceor a package-level flag to track whether the table has been created during this process lifetime.♻️ Optional: Cache table creation
+var openClawPortalStateTableCreated sync.Once + func ensureOpenClawPortalStateTable(ctx context.Context, portal *bridgev2.Portal, login *bridgev2.UserLogin) error { scope := openClawPortalDBScopeFor(portal, login) if scope == nil { return nil } - _, err := scope.db.Exec(ctx, ` + var err error + openClawPortalStateTableCreated.Do(func() { + _, err = scope.db.Exec(ctx, ` CREATE TABLE IF NOT EXISTS openclaw_portal_state ( ... ) - `) + `) + }) return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/metadata.go` around lines 127 - 143, The ensureOpenClawPortalStateTable function runs CREATE TABLE IF NOT EXISTS on every call; add a cheap in-process cache to avoid repeated DDL: introduce a package-level sync.Once or a map[<dbIdentifier>]bool guarded by a mutex to record that the openclaw_portal_state table has been created, check that cache at the top of ensureOpenClawPortalStateTable (using the same scope derived via openClawPortalDBScopeFor/ scope.db to derive a stable key), and only call scope.db.Exec when the cache says the table hasn’t been created yet, updating the cache after a successful Exec (or using Once.Do to run the Exec exactly once).
115-116: Portal key uses null byte separator.The composite key
string(portal.PortalKey.ID) + "\x00" + string(portal.PortalKey.Receiver)works but is unconventional. Null bytes in strings can cause issues with some tools/loggers. Consider using a printable delimiter like|or storing ID and Receiver as separate columns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/metadata.go` around lines 115 - 116, The code builds a composite portalKey using a null byte separator which can cause tooling/logging issues; replace the null ("\x00") separator in the expression that assigns portalKey (currently string(portal.PortalKey.ID) + "\x00" + string(portal.PortalKey.Receiver)) with a printable delimiter (e.g., "|" or another safe character) or, better, persist ID and Receiver as separate fields instead of a single concatenated string; update any consumers of portalKey to parse the new delimiter or to use the separate fields accordingly (look for usages of portalKey and portal.PortalKey.ID / portal.PortalKey.Receiver).bridges/ai/internal_prompt_db.go (1)
102-134: Consider filtering in SQL instead of post-fetch.Lines 125-127 and 132-134 filter rows after fetching. Moving these conditions to the WHERE clause would reduce data transfer and processing:
♻️ Proposed optimization
rows, err := scope.db.Query(ctx, ` SELECT event_id, canonical_turn_data, exclude_from_history, created_at_ms FROM `+aiInternalMessagesTable+` - WHERE bridge_id=$1 AND login_id=$2 AND room_id=$3 + WHERE bridge_id=$1 AND login_id=$2 AND room_id=$3 AND exclude_from_history=0 + AND ($5 = 0 OR created_at_ms >= $5) ORDER BY created_at_ms DESC, event_id DESC LIMIT $4 - `, scope.bridgeID, scope.loginID, portal.MXID.String(), limit) + `, scope.bridgeID, scope.loginID, portal.MXID.String(), limit, resetAt)Note: The
opts.excludeMessageIDfilter may still need post-fetch handling unless added as another WHERE clause.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/internal_prompt_db.go` around lines 102 - 134, Move the post-fetch filters into the SQL query to reduce rows returned: update the scope.db.Query call that selects from aiInternalMessagesTable to include "exclude_from_history = false" and, when resetAt > 0, add a "created_at_ms >= $N" predicate (and bind resetAt) so rows with excludeFromHistory and older createdAtMs are filtered out in SQL; keep the opts.excludeMessageID check in Go unless you also want to add it as another WHERE clause (in which case bind opts.excludeMessageID and compare event_id != $M). Target the Query call and the variables excludeFromHistory, createdAtMs, resetAt, and opts.excludeMessageID when applying these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bridges/ai/agentstore.go`:
- Line 563: CreateRoom and ModifyRoom call b.client.savePortalQuiet which
swallows errors, causing those APIs to report success even when portal
persistence fails; change the call to use a persistence function that returns an
error (or modify savePortalQuiet to return an error instead of only logging) and
handle that error in CreateRoom and ModifyRoom: call
savePortal/savePortalWithError (or update savePortalQuiet to return error),
check the returned error, and if non-nil return an appropriate failure to the
caller instead of a success response.
In `@bridges/ai/command_registry.go`:
- Around line 176-194: The log call at the end of the command broadcasting block
uses len(handlers) which is incorrect after filtering; change the debug log to
report len(cmds) instead so it reflects the actual number broadcast. Locate the
block that builds cmds from handlers (variables: handlers, cmds) and the call to
sdk.BroadcastCommandDescriptions(ctx, portal, bot, cmds), then update the
log.Debug().Int("count", ...) argument to use len(cmds) and keep the same "room"
portal.MXID and message.
In `@bridges/ai/delete_chat.go`:
- Around line 73-82: The delete cleanup must also clear login-level portal
metadata so stale default-chat/last-active-room IDs don't point at the deleted
portal; add a bestEffortExec call (same pattern as the existing bestEffortExec
calls that delete from aiSessionsTable and aichats_system_events) to UPDATE the
login record(s) for the same bridgeID/loginID and set the default-chat and
last-active-room fields to NULL (or their zero values) before/after
deleteInternalPromptsForRoom; locate the cleanup block referencing
bestEffortExec, aiSessionsTable, deleteInternalPromptsForRoom, and
id.RoomID(sessionKey) and add the SQL UPDATE against the login table clearing
the relevant columns.
In `@bridges/ai/handleai.go`:
- Around line 315-317: The code currently sets currentMeta.AutoGreetingSent (and
similarly WelcomeSent) and calls oc.savePortalQuiet but ignores save failures
before calling oc.dispatchInternalMessage, which can cause sent messages without
persisted flags; change the flow so oc.savePortalQuiet returns an error (or use
an existing error-returning save variant) and check its result: after setting
currentMeta.AutoGreetingSent (and for WelcomeSent in the other block) call
oc.savePortalQuiet (or a new savePortal that returns error) and if it returns an
error immediately return/bail instead of proceeding to
oc.dispatchInternalMessage; ensure you update both the AutoGreetingSent branch
(where currentMeta.AutoGreetingSent is set) and the WelcomeSent branch (around
the 383-387 area) to persist first and only dispatch the one-shot message on
successful save.
- Around line 452-459: The code updates portal.Name and portal.NameSet then
calls savePortalQuiet which only persists to DB; to sync the generated title to
the Matrix room you must call the Matrix-sync path after saving — either invoke
materializePortalRoom(bgCtx, portal, ...) (as used elsewhere when returning
PortalInfo) or call oc.sdk.EnsurePortalLifecycle(bgCtx, portal) (as in
scheduler_rooms.go) so the room state is updated; add the appropriate call
immediately after oc.savePortalQuiet(bgCtx, portal, "room title") so the DB
change is also propagated to the Matrix room.
In `@bridges/ai/integration_host.go`:
- Around line 238-272: SessionTranscript currently calls CountMessagesInPortal
then fetches that many messages with GetLastNInPortal which can load thousands
into memory; cap the number fetched (e.g., limit := min(count, 500) or a
configurable constant) and call GetLastNInPortal with that limit instead of
count to prevent unbounded memory use. Update the logic around
CountMessagesInPortal and the call to GetLastNInPortal in
runtimeIntegrationHost.SessionTranscript (and ensure
h.client.backgroundContext(ctx) is still passed) so only the capped number of
recent messages are retrieved.
In `@bridges/ai/login_config_db.go`:
- Around line 139-143: The swap-and-save pattern around
compactAIUserLoginMetadata and login.Save can leave the system inconsistent
because the separate custom DB upsert (the AI login config table) may succeed
while login.Save fails; to fix, make both updates atomic: if both tables share
the same DB, execute the custom upsert and the login.Save inside a single
transaction (use the DB transaction API surrounding the upsert and the call to
login.Save), otherwise implement a compensating rollback for the custom upsert
(e.g., delete or restore the previous row) when login.Save returns an error;
locate and change code around compactAIUserLoginMetadata, login.Save(ctx) and
the custom upsert logic so both succeed or the earlier change is reverted.
In `@bridges/ai/portal_state_db.go`:
- Around line 165-170: The current error handling block incorrectly hides
database errors by checking strings.TrimSpace(raw) in the same OR with err ==
sql.ErrNoRows; move and split checks so real Scan() errors are returned: first
check if err != nil and return err, then handle the no-row case (err ==
sql.ErrNoRows) and separately check if strings.TrimSpace(raw) == "" to return
nil,nil; reference the variables err and raw and the sql.ErrNoRows comparison in
the error-handling block around the Scan()/query result processing.
In `@bridges/ai/prompt_builder.go`:
- Around line 152-171: The current code merges internalRows into candidates and
then applies hr.limit to the combined slice, allowing internal prompts to evict
recent chat messages and corrupt image-injection indexing; fix by treating
internal rows separately: when building candidates, create a separate
internalCandidates slice from loadInternalPromptHistory and keep chatCandidates
for user/assistant messages, then merge for ordering but when enforcing hr.limit
only count/trim chatCandidates (preserve internalCandidates regardless of
hr.limit), and update any image-injection logic that uses the loop index `i` to
instead use a counter of non-internal chat entries; reference
loadInternalPromptHistory, internalRows, candidates, replayCandidate
(id/role/ts/messages), hr.limit and the sort/trimming block when making this
change.
In `@bridges/ai/tools.go`:
- Around line 390-398: The comment and logic in executeMessageReact disagree:
empty emoji is being treated as a removal but executeMessageReactRemove requires
an explicit emoji. Update executeMessageReact to only call
executeMessageReactRemove when the remove flag is true (i.e., change the
condition from `remove || emoji == ""` to `remove`) and instead validate/return
an error when emoji is empty and remove is false; also update the top comment to
reflect that removals require remove:true (or explicit emoji in the args) and
reference executeMessageReactRemove in the comment so callers know which path
needs an explicit emoji.
In `@bridges/codex/directory_manager.go`:
- Around line 354-369: The code flips state.AwaitingCwdSetup to false and saves
the portal state before calling cc.ensureRPC and cc.ensureCodexThread, which can
leave the room out of welcome mode if those calls fail; either move the state
mutations (CodexCwd, CodexThreadID, AwaitingCwdSetup, ManagedImport, Title,
Slug) and the saveCodexPortalState call to after cc.ensureCodexThread succeeds,
or capture the prior state before mutating and, if cc.ensureRPC or
cc.ensureCodexThread returns an error, call saveCodexPortalState to restore the
previous state so the room remains in welcome mode; references:
state.AwaitingCwdSetup, saveCodexPortalState, cc.ensureRPC,
cc.ensureCodexThread, and cc.syncCodexRoomTopic.
In `@bridges/openclaw/catalog.go`:
- Around line 94-104: The persisted state fields OpenClawDefaultAgentID,
OpenClawKnownModelCount, OpenClawToolCount and OpenClawToolProfile must be reset
before re-enriching so stale values aren’t left when lookups are empty or the
default/selected agent changes; update the code around oc.agentDefaultID(),
state.OpenClawAgentID/ state.OpenClawDMTargetAgentID (used with
stringutil.TrimDefault), loadModelCatalog(ctx, false) and loadToolsCatalog(ctx,
agentID, false) to clear state.OpenClawDefaultAgentID (set to ""),
state.OpenClawKnownModelCount (set to 0), and
state.OpenClawToolCount/state.OpenClawToolProfile (set to 0 and nil/empty)
before attempting the loads, then repopulate them only when the corresponding
loads return non-empty results and use summarizeToolsCatalog only when catalog
is non-nil.
In `@bridges/openclaw/identifiers.go`:
- Around line 33-39: The current ID format built by openClawScopedGhostUserID
uses url.PathEscape but still allows colons to survive, causing ambiguous
parsing; change the encoding to an unambiguous one (e.g., hex or base64 URL-safe
encoding like base64.RawURLEncoding or hex.EncodeToString) for both the loginID
and agentID when constructing the string and add a versioned prefix (e.g.,
"v1:openclaw-agent:") to the ID; update the corresponding parser (the function
that parses/scans scoped ghost IDs) to decode the chosen encoding and honor the
version prefix so splitting on ":" is safe and unambiguous.
In `@bridges/openclaw/login.go`:
- Around line 262-269: If persisting credentials via saveOpenClawLoginState
fails after sdk.CreateAndCompleteLogin succeeded, implement a rollback or
coordination: either wrap the create+persist in a transaction/atomic operation
or, on saveOpenClawLoginState error, call the SDK cleanup to remove the newly
created login (e.g., invoke a delete/revert API for the created login using the
login ID) so you don't leave a half-created login without credentials; reference
the saveOpenClawLoginState call, the sdk.CreateAndCompleteLogin invocation, and
the openClawPersistedLoginState struct when adding the rollback/transaction
logic.
In `@bridges/openclaw/manager.go`:
- Around line 596-619: The code updates only the local sessionKey after
resolving a synthetic DM but doesn't persist it, so racey events can miss the
real session; before calling gateway.SendMessage(...) set
state.OpenClawSessionKey = strings.TrimSpace(resolvedKey) (or the trimmed
sessionKey if no resolve) and persist the portal/state using the existing
persistence method in this manager (so the saved state reflects the real session
key), ensuring the saved key is in place before SendMessage; alternatively, if
you prefer sync refresh, call m.syncSessions synchronously (using
m.client.BackgroundContext(ctx)) immediately after setting/persisting the key
and before SendMessage so the portal state is consistent.
In `@bridges/openclaw/provisioning.go`:
- Around line 282-322: The code may create the Matrix room via
sdk.EnsurePortalLifecycle before the updated OpenClaw state is durably persisted
and before portalMeta(portal).IsOpenClawRoom is set; to fix, persist the updated
state and set the in-memory marker before calling sdk.EnsurePortalLifecycle:
after oc.enrichPortalState(ctx, state) call save the state with
saveOpenClawPortalState(ctx, portal, oc.UserLogin, state) and set
portalMeta(portal).IsOpenClawRoom = true, handling and returning any error
immediately, then proceed to call sdk.EnsurePortalLifecycle; this ensures
failure to persist aborts before the room is created and the in-memory marker is
set consistently.
In `@pkg/integrations/memory/integration.go`:
- Around line 168-174: The PurgeForLogin implementation currently returns before
shutting down in-process managers when resolveStateDB() is nil; move the call to
StopManagersForLogin(scope.BridgeID, scope.LoginID) so it runs unconditionally
before the nil check, then keep the existing nil return and the subsequent
PurgeTables(ctx, db, scope.BridgeID, scope.LoginID) call when a DB exists;
update the function in Integration (PurgeForLogin) that calls resolveStateDB(),
StopManagersForLogin and PurgeTables to ensure managers are always stopped even
if MemoryStateDB() is unavailable.
---
Outside diff comments:
In `@bridges/ai/chat.go`:
- Around line 1095-1132: Before creating a new default portal, add a legacy-room
fallback: after the GetExistingPortalByKey check fails (portal == nil) call
oc.listAllChatPortals(ctx, oc.UserLogin) and pass the result into
oc.chooseDefaultChatPortal(...) to see if an older portal should be reused; if
chooseDefaultChatPortal returns a portal, reuse it (materialize if MXID missing
via oc.materializePortalRoom) and return nil instead of proceeding to create a
new portal with oc.initPortalForChat; keep existing logging behavior and error
handling around materializePortalRoom and initPortalForChat, and reference the
existing symbols GetExistingPortalByKey, listAllChatPortals,
chooseDefaultChatPortal, materializePortalRoom, and initPortalForChat when
making the change.
In `@bridges/ai/command_registry.go`:
- Around line 197-210: Remove the dead helper buildCommandDescriptionContent and
its associated unit test that directly invokes it (the test in events_test that
targets buildCommandDescriptionContent); update references so
BroadcastCommandDescriptions continues to construct sdk.Command objects directly
and delete both the function buildCommandDescriptionContent and the test file's
test case that calls it. Ensure no other code references
buildCommandDescriptionContent remain and run the test suite to confirm removal
is safe.
In `@bridges/ai/login.go`:
- Around line 239-248: The code calls ol.User.NewLogin(...) which creates a
login and then calls saveAIUserLogin(...); if saveAIUserLogin fails you must
roll back the created login to avoid stranded state. Modify the error path after
saveAIUserLogin to call the appropriate deletion method (e.g.,
ol.User.DeleteLogin(ctx, loginID) or equivalent) to remove the created login
(and handle/log any deletion error), then return the original wrapped save
error; ensure you reference ol.User.NewLogin, saveAIUserLogin, and loginID when
implementing the rollback so creation is undone on failure.
In `@bridges/ai/tool_approvals_rules.go`:
- Around line 59-80: The approval checks isMcpAlwaysAllowed and
isBuiltinAlwaysAllowed currently call
hasToolApprovalRule/hasBuiltinToolApprovalRule with context.Background(), making
DB lookups uncancellable; change these functions to accept a context.Context
parameter (e.g., ctx) and pass that ctx into oc.hasToolApprovalRule and
oc.hasBuiltinToolApprovalRule, or alternatively create a derived context with a
short timeout inside each function and use that for the DB call; update all
callers of isMcpAlwaysAllowed and isBuiltinAlwaysAllowed to supply the caller
context so the DB queries are cancellable.
In `@bridges/codex/directory_manager.go`:
- Around line 133-164: The current flow calls sdk.EnsurePortalLifecycle (in
bridges/codex/directory_manager.go) before persisting codexPortalState with
saveCodexPortalState, which can create the Matrix room without saved state; move
the saveCodexPortalState call to occur immediately after initializing the
codexPortalState and before calling sdk.EnsurePortalLifecycle, so the state is
persisted (and errors returned) prior to creating the room; keep the calls to
portalMeta(portal).IsCodexRoom = true and
sdk.ConfigureDMPortal/cc.composeCodexChatInfo/sendSystemNotice intact, but
ensure saveCodexPortalState is executed and its error handled before invoking
EnsurePortalLifecycle to avoid orphaned rooms without persisted state.
In `@pkg/aidb/db_test.go`:
- Around line 53-68: TestUpgradeV1Fresh is missing assertions for newly added v1
tables; update the test (TestUpgradeV1Fresh in pkg/aidb/db_test.go) to include
checks for the three tables created by pkg/aidb/001-init.sql:
aichats_internal_messages, aichats_login_state, and aichats_tool_approval_rules
so the fresh-schema path fails if any of those tables are not created during
upgrade.
In `@sdk/conversation_state.go`:
- Around line 75-106: The code currently allows storing/reading under an empty
key when conversationStateKey(portal) returns "", causing unrelated portals to
share state; update conversationStateStore.get and conversationStateStore.set to
treat an empty key as invalid: after computing key :=
conversationStateKey(portal) return a new empty sdkConversationState (without
accessing s.rooms) if key == "" in get, and return immediately without writing
to s.rooms if key == "" in set; reference conversationStateKey,
conversationStateStore.get, conversationStateStore.set, s.rooms and ensure the
same nil checks remain.
- Around line 134-176: loadConversationState currently only checks the cache and
DB, so add a legacy fallback: when state is still nil/default after consulting
the store and DB (inside loadConversationState), inspect portal.Metadata for the
legacy conversation-state entry (the key used previously for storing state in
portal metadata), unmarshal that JSON into an sdkConversationState, and use it
as the loaded state (without mutating portal.Metadata). After loading from
legacy metadata, call state.ensureDefaults() and store.set(portal, state) as
currently done so the rest of the code treats it like a normal load; keep
loadConversationStateFromDB unchanged.
In `@sdk/conversation.go`:
- Around line 47-52: The getIntent method on Conversation now dereferences c
unconditionally which will panic for a nil *Conversation and cascade into
SendMedia, SendNotice, sendMessageContent, and SetTyping; restore the nil
receiver guard by checking if c == nil at the top of Conversation.getIntent and
return a sensible error (e.g., fmt.Errorf or a package-level err like
ErrNoConversation) instead of calling resolveMatrixIntent, and keep the existing
intentOverride branch (i.e., if c != nil && c.intentOverride != nil return
c.intentOverride(ctx)); ensure callers that expect an error continue to
propagate it.
---
Nitpick comments:
In `@bridges/ai/identifiers.go`:
- Line 171: The call to loadPortalStateIntoMetadata is using
context.Background() inside portalMeta(), preventing cancellation/timeouts from
propagating; change portalMeta to accept a context.Context parameter and
propagate that ctx into loadPortalStateIntoMetadata (i.e., call
loadPortalStateIntoMetadata(ctx, portal, meta) instead of using Background()),
then update portalMeta call sites to pass the available request/context where
present (and only use context.Background() as an explicit fallback in rare
callers that truly lack a context), ensuring all DB-backed portal state loads
honor cancellation.
In `@bridges/ai/internal_prompt_db.go`:
- Around line 102-134: Move the post-fetch filters into the SQL query to reduce
rows returned: update the scope.db.Query call that selects from
aiInternalMessagesTable to include "exclude_from_history = false" and, when
resetAt > 0, add a "created_at_ms >= $N" predicate (and bind resetAt) so rows
with excludeFromHistory and older createdAtMs are filtered out in SQL; keep the
opts.excludeMessageID check in Go unless you also want to add it as another
WHERE clause (in which case bind opts.excludeMessageID and compare event_id !=
$M). Target the Query call and the variables excludeFromHistory, createdAtMs,
resetAt, and opts.excludeMessageID when applying these changes.
In `@bridges/ai/login_config_db.go`:
- Around line 69-84: The current ensureAILoginConfigTable function contains
inline DDL (CREATE TABLE IF NOT EXISTS) for aiLoginConfigTable; replace this
ad-hoc creation with a proper schema migration: remove the inline CREATE TABLE
from ensureAILoginConfigTable and instead register a migration (e.g., a new
migration entry or function like migrateCreateAILoginConfigTable) that performs
the DDL via your project's migration framework or migration runner, ensure the
migration is idempotent and versioned, and update any bootstrap code that called
ensureAILoginConfigTable to run/verify migrations (refer to symbols
ensureAILoginConfigTable and aiLoginConfigTable to locate the code to change).
In `@bridges/ai/logout_cleanup.go`:
- Around line 60-71: The three DELETEs use hardcoded table names; replace them
with the centralized table-name variables (like the existing aiSessionsTable
pattern) so table names are consistent and configurable; update the
bestEffortExec calls to use the corresponding variables (e.g.,
internalMessagesTable, toolApprovalRulesTable, loginStateTable or whatever names
are defined in the same package), and if those variables don't exist add them
alongside aiSessionsTable where other AI table constants are declared so all
DELETEs reference the shared table-name variables rather than literal strings.
- Around line 72-74: The code performs the same type assertion on login.Client
to *AIClient twice; consolidate by doing a single assertion (e.g., if client, ok
:= login.Client.(*AIClient); ok && client != nil { ... }) and call both
client.purgeLoginIntegrations(ctx) and client.clearLoginState(ctx) inside that
block, then remove the duplicate assertion and the separate clearLoginState call
so both methods run under one nil-checked *AIClient branch.
In `@bridges/openclaw/metadata.go`:
- Around line 127-143: The ensureOpenClawPortalStateTable function runs CREATE
TABLE IF NOT EXISTS on every call; add a cheap in-process cache to avoid
repeated DDL: introduce a package-level sync.Once or a map[<dbIdentifier>]bool
guarded by a mutex to record that the openclaw_portal_state table has been
created, check that cache at the top of ensureOpenClawPortalStateTable (using
the same scope derived via openClawPortalDBScopeFor/ scope.db to derive a stable
key), and only call scope.db.Exec when the cache says the table hasn’t been
created yet, updating the cache after a successful Exec (or using Once.Do to run
the Exec exactly once).
- Around line 115-116: The code builds a composite portalKey using a null byte
separator which can cause tooling/logging issues; replace the null ("\x00")
separator in the expression that assigns portalKey (currently
string(portal.PortalKey.ID) + "\x00" + string(portal.PortalKey.Receiver)) with a
printable delimiter (e.g., "|" or another safe character) or, better, persist ID
and Receiver as separate fields instead of a single concatenated string; update
any consumers of portalKey to parse the new delimiter or to use the separate
fields accordingly (look for usages of portalKey and portal.PortalKey.ID /
portal.PortalKey.Receiver).
In `@bridges/opencode/opencode_instance_state.go`:
- Around line 100-110: The sessionIDs() method returns session IDs in
non-deterministic map order; lock seenMu, collect keys from inst.knownSessions
as done, then sort the slice (e.g., using sort.Strings) before returning to
ensure deterministic ordering for callers, tests, and logs; update the function
(sessionIDs) to import the sort package and call sort.Strings(out) right before
the return.
In `@cmd/agentremote/profile.go`:
- Around line 26-42: Update the doc comments for configRoot and profileRoot to
reflect that the path uses the dynamic binaryName variable rather than the
hardcoded "sdk" string: change the comment above configRoot to indicate it
returns ~/.config/<binaryName> and the comment above profileRoot to indicate it
returns ~/.config/<binaryName>/profiles/<profile>, referencing the functions
configRoot and profileRoot so future readers know the path is derived from
binaryName.
In `@cmd/internal/bridgeentry/bridgeentry.go`:
- Around line 21-50: The AI Definition's Description ("AI Chats bridge for
Beeper") is inconsistent with the other bridge Definitions (Codex, OpenCode,
OpenClaw, DummyBridge) which use the "X bridge built with the AgentRemote SDK."
pattern; update the AI variable's Description in the Definition for AI to match
that pattern (e.g., "AI bridge built with the AgentRemote SDK.") or, if the
different wording is intentional, add a brief inline comment above the AI
Definition explaining the special case so reviewers understand the deviation.
In `@pkg/integrations/memory/sessions.go`:
- Around line 63-67: The code redundantly calls m.saveSessionState(ctx, key,
state) when !force and hash == state.contentHash even though state wasn't
modified; remove the save call from that branch (the if-block in sessions.go
that checks !force && hash == state.contentHash) so it simply continues, and
ensure any necessary saves remain where state is actually mutated (keep
m.saveSessionState only in places like saveSessionState invocations after state
changes).
In `@pkg/shared/toolspec/toolspec.go`:
- Around line 143-145: The schema currently documents that "emoji" is required
when "remove": true for the "react" action but doesn't enforce it; update the
toolspec schema construction in toolspec.go (the code building properties like
StringProperty("emoji") and BooleanProperty("remove") for the "react" action) to
add a conditional JSON Schema rule that enforces emoji when remove is true — for
example add an if/then clause scoped to action === "react" (if:
{properties:{action:{const:"react"}, remove:{const:true}}} then:
{required:["emoji"]}) or express the same via oneOf alternatives; ensure the new
rule is attached to the same schema object used by the validator so malformed
react/remove payloads are rejected.
In `@sdk/conversation_test.go`:
- Around line 47-49: The helper currently panics on save errors which obscures
test attribution; modify the test helper (e.g., newTestConversation) to accept a
*testing.T (or change it to return an error) and replace the panic(err) after
conv.saveState(...) with t.Fatalf("saveState failed: %v", err) so failures are
reported as test failures and attributed to the calling test; update all callers
of newTestConversation to pass the *testing.T (or handle the returned error)
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-agentremote-docker (amd64)
- GitHub Check: build-docker
- GitHub Check: Lint
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-16T09:01:24.464Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 71
File: bridges/ai/connector.go:53-63
Timestamp: 2026-03-16T09:01:24.464Z
Learning: In package ai, the AI connector’s configuration type (Config) defines Bridge as a value field of type BridgeConfig (not a pointer). Therefore, accessing oc.Config.Bridge.CommandPrefix in OpenAIConnector.applyRuntimeDefaults (bridges/ai/connector.go) is safe and does not require a nil-check.
Applied to files:
bridges/ai/integrations.gobridges/ai/abort_helpers_test.gobridges/ai/bridge_info.gobridges/ai/client_capabilities_test.gobridges/ai/command_registry.gobridges/ai/constructors_test.gocmd/agentremote/run_bridge.gobridges/ai/agentstore.gobridges/dummybridge/runtime_test.gosdk/connector_builder_test.gobridges/ai/bridge_db.gobridges/ai/login_loaders.gobridges/codex/connector.gobridges/codex/constructors.gobridges/ai/commands.gobridges/codex/runtime_helpers.gobridges/ai/connector.gosdk/connector_builder.gobridges/ai/constructors.gobridges/ai/sdk_agent_catalog.gobridges/dummybridge/connector.gobridges/ai/handleai.gobridges/opencode/connector.gobridges/openclaw/connector.gocmd/internal/bridgeentry/bridgeentry.gosdk/connector.gosdk/types.gobridges/dummybridge/runtime.gobridges/ai/client.go
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
bridges/codex/constructors.go (1)
42-55:⚠️ Potential issue | 🟠 MajorGuard
aidb.EnsureSchemawhen the bridge DB is unavailable.
InitConnectorconditionally initializescc.dbonly whenbridge,bridge.DB, andbridge.DB.Databaseare all non-nil. However,StartConnectorunconditionally passes the result ofcc.bridgeDB()toaidb.EnsureSchema, which explicitly errors on nil input rather than short-circuiting. This causes startup failures in no-DB scenarios.Suggested fix
StartConnector: func(ctx context.Context, _ *bridgev2.Bridge) error { db := cc.bridgeDB() - if err := aidb.EnsureSchema(ctx, db); err != nil { - return err + if db != nil { + if err := aidb.EnsureSchema(ctx, db); err != nil { + return err + } } cc.applyRuntimeDefaults() sdk.PrimeUserLoginCache(ctx, cc.br) return nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/codex/constructors.go` around lines 42 - 55, StartConnector calls aidb.EnsureSchema with cc.bridgeDB() even when InitConnector left cc.db nil; wrap the EnsureSchema call in a nil-check so it only runs when cc.bridgeDB() != nil (e.g. assign db := cc.bridgeDB(); if db == nil { return nil } else if err := aidb.EnsureSchema(ctx, db); err != nil { return err }), ensuring StartConnector short-circuits cleanly in no-DB scenarios and avoids passing nil to aidb.EnsureSchema.bridges/ai/chat.go (2)
46-52:⚠️ Potential issue | 🟠 MajorTreat an unset
Agentsflag as enabled.This helper returns
falsewhencfg.Agentsis nil, butshouldEnsureDefaultChatstill treats nil as “enabled”. That leaves default-config logins in a split state where bootstrap creates the agent chat while search/create-chat flows reject agents.Suggested fix
func (oc *AIClient) agentsEnabledForLogin() bool { if oc == nil || oc.UserLogin == nil { return false } cfg := oc.loginConfigSnapshot(context.Background()) - return cfg.Agents != nil && *cfg.Agents + return cfg.Agents == nil || *cfg.Agents }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/chat.go` around lines 46 - 52, agentsEnabledForLogin currently returns false when cfg.Agents is nil, causing a mismatch with shouldEnsureDefaultChat which treats nil as enabled; update agentsEnabledForLogin (in AIClient) to treat a nil Agents pointer as enabled by returning true when cfg.Agents == nil, otherwise return the boolean value (*cfg.Agents) from the loginConfigSnapshot result so default-config logins consistently enable agents.
239-268:⚠️ Potential issue | 🟠 MajorReturn the bridge ghost ID for agent contacts.
agentContactResponsenow exposesnetworkid.UserID(agent.ID), but the rest of the bridge consistently usesoc.agentUserID(agentID)for agent ghosts. Search/contact results can therefore hand back an ID that doesn't hydrate to the actual ghost or open the right DM.Suggested fix
func (oc *AIClient) agentContactResponse(ctx context.Context, agent *sdk.Agent) *bridgev2.ResolveIdentifierResponse { if agent == nil || !oc.agentsEnabledForLogin() { return nil } + userID := networkid.UserID(agent.ID) + if agentID := catalogAgentID(agent); agentID != "" { + userID = oc.agentUserID(agentID) + } resp := &bridgev2.ResolveIdentifierResponse{ - UserID: networkid.UserID(agent.ID), + UserID: userID, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/chat.go` around lines 239 - 268, The response currently sets resp.UserID to networkid.UserID(agent.ID) which doesn't match how agent ghosts are stored; update agentContactResponse so the UserID returned for agent contacts uses the bridge ghost ID via oc.agentUserID(agentID) (using the catalogAgentID result) instead of networkid.UserID(agent.ID); if catalogAgentID(agent) is empty, keep the existing fallback behavior, and ensure this change happens before calling UserLogin.Bridge.GetGhostByID so the ghost hydrate uses the correct ID (refer to functions agentContactResponse, catalogAgentID, oc.ResolveResponderForAgent, and oc.agentUserID).bridges/ai/metadata.go (1)
313-325:⚠️ Potential issue | 🟠 Major
cloneUserLoginMetadatanow strips all transient login state.After the tag changes above, this JSON round-trip preserves essentially only
Provider. Any caller that clones before mutating or persisting login state will silently lose credentials, caches, profile data, custom agents, and error counters. This needs an explicit clone path for the non-serialized fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/metadata.go` around lines 313 - 325, cloneUserLoginMetadata performs a JSON round-trip which strips non-serialized/transient fields (credentials, caches, profile data, custom agents, error counters), so update cloneUserLoginMetadata to perform the JSON marshal/unmarshal for the serializable fields and then explicitly copy the transient fields from src into the resulting clone (e.g., copy Credentials, Cache, Profile, CustomAgents, ErrorCounters or their pointer/deep copies as appropriate) so callers that clone before mutating/persisting don't lose runtime state; locate and modify cloneUserLoginMetadata to perform the manual transfer after unmarshal and ensure pointer safety (deep-copy if needed) before returning the clone.
♻️ Duplicate comments (5)
bridges/ai/handleai.go (3)
387-390:⚠️ Potential issue | 🟠 MajorPersist
WelcomeSentbefore sending the notice.This has the same one-shot persistence gap as the auto-greeting path: if the save doesn't stick, the welcome notice can be emitted again later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handleai.go` around lines 387 - 390, Set meta.WelcomeSent = true and persist that change with oc.savePortalQuiet using a cancellable context (bgCtx from context.WithTimeout(oc.backgroundContext(ctx), 10*time.Second)) before any code that emits the welcome notice; ensure you call oc.savePortalQuiet immediately after setting meta.WelcomeSent and check/handle its error (log/stop the notice emission) so the notice is only sent if the persistence succeeds.
319-321:⚠️ Potential issue | 🟠 MajorPersist
AutoGreetingSentbefore dispatching the greeting.This still sends the one-shot greeting even if the flag wasn't durably saved. A restart or retry can resend it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handleai.go` around lines 319 - 321, The AutoGreetingSent flag is being set in-memory but not guaranteed persisted before sending the greeting; change the order and add error handling so persistence happens first: set currentMeta.AutoGreetingSent = true, call oc.savePortalQuiet(bgCtx, current, "auto greeting state") and check its error (do not call oc.dispatchInternalMessage if save failed), logging or returning the save error; only after a successful save call oc.dispatchInternalMessage(..., "auto-greeting", true). This uses the existing symbols currentMeta.AutoGreetingSent, oc.savePortalQuiet, and oc.dispatchInternalMessage.
456-463:⚠️ Potential issue | 🟠 MajorSync the generated title to Matrix after saving it.
Lines 461-463 only update
portal.Name/NameSetand persist the portal record. Without a lifecycle/materialization sync, clients won't see the generated room title update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handleai.go` around lines 456 - 463, The portal title is only persisted locally (portal.Name/portal.NameSet and oc.savePortalQuiet) but not pushed to Matrix; after oc.savePortalQuiet(bgCtx, portal, "room title") call the existing lifecycle/materialization sync that pushes portal metadata to Matrix (i.e., the oc method responsible for materializing/syncing portals) so the updated portal.Title/portal.Name is sent to clients; if no such helper exists, add and call a method like oc.MaterializePortal(ctx, portal) or oc.SyncPortalTitleToMatrix(ctx, portal) immediately after saving to trigger the external update.bridges/ai/tools.go (1)
390-397:⚠️ Potential issue | 🟡 MinorRequire
remove:truebefore routing to reaction removal.Line 396 still treats an empty
emojias a removal request. That sends malformed add-reaction calls into the remove handler instead of rejecting them up front, and it still doesn't match the comment about “explicit emoji” removal.Suggested fix
-// Supports adding reactions (with emoji) and removing reactions (with remove:true or explicit emoji). +// Supports adding reactions with `emoji` and removing reactions with `remove:true`. func executeMessageReact(ctx context.Context, args map[string]any, btc *BridgeToolContext) (string, error) { emoji, _ := args["emoji"].(string) remove, _ := args["remove"].(bool) // Check if this is a removal request. - if remove || emoji == "" { + if remove { return executeMessageReactRemove(ctx, args, btc) } + if strings.TrimSpace(emoji) == "" { + return "", errors.New("action=react requires 'emoji' unless remove=true") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/tools.go` around lines 390 - 397, The routing in executeMessageReact incorrectly treats an empty emoji as a removal request; change the conditional so only remove == true forwards to executeMessageReactRemove (i.e., if remove { return executeMessageReactRemove(...) }), and add explicit validation before attempting an add reaction that rejects empty or missing emoji (return an error) so malformed add-reaction calls are rejected instead of sent to executeMessageReactRemove; reference executeMessageReact and executeMessageReactRemove when making the change.bridges/ai/integration_host.go (1)
238-250:⚠️ Potential issue | 🟠 MajorCap
SessionTranscripthistory fetches.Line 242 counts the full portal history, and Line 250 immediately asks
getAIHistoryMessagesfor that full count. Long-lived chats can turn this into an unbounded in-memory load.Suggested fix
func (h *runtimeIntegrationHost) SessionTranscript(ctx context.Context, portalKey networkid.PortalKey) ([]integrationruntime.MessageSummary, error) { if h == nil || h.client == nil || h.client.UserLogin == nil || h.client.UserLogin.Bridge == nil || h.client.UserLogin.Bridge.DB == nil { return nil, nil } count, err := h.client.UserLogin.Bridge.DB.Message.CountMessagesInPortal(ctx, portalKey) if err != nil || count <= 0 { return nil, err } + const maxTranscriptMessages = 500 + if count > maxTranscriptMessages { + count = maxTranscriptMessages + } portal, err := h.client.UserLogin.Bridge.GetPortalByKey(h.client.backgroundContext(ctx), portalKey)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/integration_host.go` around lines 238 - 250, SessionTranscript currently requests the entire portal message count (CountMessagesInPortal) and passes it unbounded into getAIHistoryMessages which can blow up memory for long-lived chats; cap the fetch size by defining a sensible max (e.g. maxHistoryMessages) and clamp the count before calling getAIHistoryMessages (use min(count, maxHistoryMessages)), update SessionTranscript to pass the capped value to getAIHistoryMessages and consider logging or annotating when the returned history was truncated; reference functions/values: SessionTranscript, CountMessagesInPortal, getAIHistoryMessages.
🧹 Nitpick comments (8)
bridges/ai/image_generation_tool.go (5)
255-272: Redundant nil check onbtc.Client.UserLogin.Metadata.Line 256 checks
btc.Client.UserLogin.Metadata == nil, but after migrating toeffectiveLoginMetadata, this check is no longer meaningful sinceeffectiveLoginMetadatadoesn't directly useUserLogin.Metadata. The subsequent nil check onloginMeta(line 260) is sufficient.♻️ Suggested simplification
func supportsOpenAIImageGen(btc *BridgeToolContext) bool { - if btc == nil || btc.Client == nil || btc.Client.UserLogin == nil || btc.Client.UserLogin.Metadata == nil { + if btc == nil || btc.Client == nil || btc.Client.UserLogin == nil { return false } loginMeta := btc.Client.effectiveLoginMetadata(context.Background())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/image_generation_tool.go` around lines 255 - 272, The nil check for btc.Client.UserLogin.Metadata in supportsOpenAIImageGen is redundant because effectiveLoginMetadata() is the authoritative source; remove the btc.Client.UserLogin.Metadata == nil check from the initial guard and rely on the existing nil check for loginMeta returned by btc.Client.effectiveLoginMetadata(context.Background()), keeping the rest of the logic (including Provider switch, magic proxy credential checks via loginCredentialAPIKey/loginCredentialBaseURL, and resolveOpenAIAPIKey) unchanged.
623-660: Same redundantMetadatanil check pattern inresolveOpenRouterImageGenEndpoint.The
btc.Client.UserLogin.Metadata == nilcheck at line 624 can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/image_generation_tool.go` around lines 623 - 660, In resolveOpenRouterImageGenEndpoint, remove the redundant nil check for btc.Client.UserLogin.Metadata from the initial guard (the check that includes btc.Client.UserLogin.Metadata == nil) because you call btc.Client.effectiveLoginMetadata(...) later which already handles missing Metadata; keep the other nil checks (btc, btc.Client, btc.Client.UserLogin) intact and ensure the function still returns the same false/empty values when required.
280-295: Same redundantMetadatanil check pattern.Similar to
supportsOpenAIImageGen, thebtc.Client.UserLogin.Metadata == nilcheck at line 281 is no longer necessary after the migration toeffectiveLoginMetadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/image_generation_tool.go` around lines 280 - 295, Remove the redundant btc.Client.UserLogin.Metadata nil check in supportsGeminiImageGen: rely on effectiveLoginMetadata(context.Background()) for login metadata validation (as in supportsOpenAIImageGen). Update the initial guard to only check btc, btc.Client and btc.Client.UserLogin as needed, call btc.Client.effectiveLoginMetadata(...) and return false if it is nil, then keep the existing provider switch (ProviderMagicProxy -> false, default -> false); reference supportsGeminiImageGen, BridgeToolContext, btc.Client.UserLogin.Metadata, and effectiveLoginMetadata to locate and edit the code.
480-507: Same redundantMetadatanil check pattern inbuildOpenAIImagesBaseURL.The
btc.Client.UserLogin.Metadata == nilcheck at line 481 can be removed for consistency with the new pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/image_generation_tool.go` around lines 480 - 507, Remove the redundant btc.Client.UserLogin.Metadata == nil check from the initial guard in buildOpenAIImagesBaseURL; keep the nil checks for btc, btc.Client, and btc.Client.UserLogin only, and rely on loginMeta := btc.Client.effectiveLoginMetadata(...) to handle metadata presence and return the appropriate error if nil—update the initial conditional accordingly so the function compiles and behavior is consistent with effectiveLoginMetadata.
509-533: Same redundantMetadatanil check pattern inbuildGeminiBaseURL.The
btc.Client.UserLogin.Metadata == nilcheck at line 510 can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/image_generation_tool.go` around lines 509 - 533, Remove the redundant nil check for UserLogin.Metadata in buildGeminiBaseURL: keep the existing nil guards for btc, btc.Client, and btc.Client.UserLogin but delete the btc.Client.UserLogin.Metadata == nil condition since effectiveLoginMetadata (btc.Client.effectiveLoginMetadata) is already called and validated; ensure buildGeminiBaseURL still returns the same errors when loginMeta is nil and that Provider-specific logic (ProviderMagicProxy branch, connector.resolveServiceConfig, normalizeProxyBaseURL, joinProxyPath) remains unchanged.bridges/ai/logout_cleanup.go (1)
94-109:bestEffortExecis redundant and should be removed.The function at lines 107–109 is a direct pass-through to
execDelete. Update the 6 callers acrossdelete_chat.go,internal_prompt_db.go, andlogin_state_db.goto callexecDeletedirectly instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/logout_cleanup.go` around lines 94 - 109, Remove the redundant wrapper function bestEffortExec and replace its uses with direct calls to execDelete; specifically delete the bestEffortExec declaration and update the six call sites that currently call bestEffortExec (across delete_chat.go, internal_prompt_db.go, and login_state_db.go) to call execDelete(ctx, db, logger, query, args...) directly so argument expansion and behavior remain unchanged.bridges/ai/mcp_helpers.go (1)
82-101: Type switch should include a defensive default case for consistency.The type switch at lines 95–100 lacks a default case, unlike the similar switches in
loginCredentials()andensureLoginCredentials()(which both return nil or do nothing for unsupported types). While all current callers pass*aiLoginConfigfromloginConfigSnapshot(), adding a default case maintains defensive consistency and guards against future refactoring where unexpected types might be passed.♻️ Suggested defensive handling
if loginCredentialsEmpty(creds) { switch v := owner.(type) { case *UserLoginMetadata: v.Credentials = nil case *aiLoginConfig: v.Credentials = nil + default: + // Unsupported owner type; credentials not cleared } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/mcp_helpers.go` around lines 82 - 101, The type switch in clearLoginMCPServer does not include a default branch, making it inconsistent with loginCredentials() and ensureLoginCredentials(); update the switch in clearLoginMCPServer (the one switching on owner in function clearLoginMCPServer) to add a defensive default case that does nothing (no-op) so unsupported owner types are ignored safely; keep the existing cases for *UserLoginMetadata and *aiLoginConfig and ensure the default mirrors the harmless behavior used in loginCredentials()/ensureLoginCredentials().bridges/ai/media_understanding_runner.go (1)
926-926: Consider passing the actual context instead ofcontext.Background().Using
context.Background()here loses any tracing, logging context, or cancellation signals from the caller. IfeffectiveLoginMetadataperforms any I/O or logging that should respect the request context, consider passing the actualctxparameter (or the surrounding function's context) instead.This pattern appears at multiple call sites in this file (lines 926, 942, 1032, 1038, 1054).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/media_understanding_runner.go` at line 926, The call to oc.effectiveLoginMetadata currently uses context.Background() which discards caller tracing/cancellation; update calls like oc.connector.resolveServiceConfig(oc.effectiveLoginMetadata(context.Background())) to pass the upstream context (e.g., the local ctx variable) instead so effectiveLoginMetadata and resolveServiceConfig receive the real request context; apply the same change at all similar call sites in this file (references: effectiveLoginMetadata, resolveServiceConfig).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bridges/ai/custom_agents_db.go`:
- Around line 107-135: In saveCustomAgentForLogin ensure the trimmed agent.ID is
non-empty before using it: call strings.TrimSpace(agent.ID) into a local id
variable, and if id=="" return an error (or non-nil) so we don't write an
empty-key into loginMetadata (meta.CustomAgents) or insert a DB row; update the
branches that use strings.TrimSpace(agent.ID) (the fallback map insertion and
the scope.db.Exec call) to use that validated id; this prevents saving blank IDs
while keeping references to customAgentScopeForLogin, loginMetadata,
cloneAgentDefinitionContentMap, and aiCustomAgentsTable intact.
In `@bridges/ai/desktop_api_sessions.go`:
- Line 161: The call to effectiveLoginMetadata is using context.Background(),
dropping request cancellation/deadlines; update desktopAPIInstances to accept a
context.Context parameter (rename to desktopAPIInstances(ctx)) and thread that
ctx into the call that builds creds (use
loginCredentials(effectiveLoginMetadata(ctx))). Also update all internal calls
and call sites (e.g., places in sessions_tools.go and other callers) to pass the
incoming request context; this ensures the chain through loginConfigSnapshot →
ensureLoginConfigLoaded → loadAILoginConfig(ctx) receives the proper context and
cancels I/O when requests are cancelled.
In `@bridges/ai/gravatar.go`:
- Around line 38-43: The function ensureGravatarState dereferences
state.Gravatar without checking that the pointer state is non-nil; update
ensureGravatarState to first check if state == nil and either return a new
GravatarState (or allocate and return a new loginRuntimeState with Gravatar set)
or explicitly document/require non-nil callers — modify ensureGravatarState (and
any callers if you choose to allocate) to avoid panics by handling a nil
*loginRuntimeState before accessing state.Gravatar or state.Gravatar =
&GravatarState{}.
In `@bridges/ai/handleai.go`:
- Around line 129-156: The health-transition logic currently reads state via
loginStateSnapshot() then mutates via updateLoginState(), causing racey
decisions; fix by making the decision and mutation atomic inside
updateLoginState() for both recordProviderFailure (the failure path) and
recordProviderSuccess: inside the updateLoginState callback read the current
state.ConsecutiveErrors, compute nextErrors (or wasUnhealthy→nextHealthy) based
on the change, update state.ConsecutiveErrors and state.LastErrorAt there, and
set a boolean/enum captured by the outer scope indicating whether a
health-transition event should be emitted; after updateLoginState returns,
consult that captured result and call oc.UserLogin.BridgeState.Send(...) if and
only if the atomic transition crossed the healthWarningThreshold (use the same
threshold constant in both places). Ensure you reference and change the logic in
the functions that call loginStateSnapshot(), updateLoginState(), and
oc.UserLogin.BridgeState.Send (recordProviderFailure/recordProviderSuccess
flows) so all decisioning happens inside updateLoginState.
In `@bridges/ai/login_state_db.go`:
- Around line 64-75: The fallback conversion in loginRuntimeStateFromMetadata
currently omits the legacy NextChatIndex and LastHeartbeatEvent fields so chat
numbering and heartbeat seeding are lost; update loginRuntimeStateFromMetadata
to copy meta.NextChatIndex and meta.LastHeartbeatEvent into the returned
*loginRuntimeState (and likewise update the inverse mapper(s) that convert
runtime state back to UserLoginMetadata in the same file — any functions
handling mapping around loginRuntimeState/UserLoginMetadata between lines
~103-139) so both NextChatIndex and LastHeartbeatEvent are preserved across
migrations.
- Around line 249-265: The updateLoginState function currently lets fn mutate
oc.loginState in-place before persistence, so a failed save leaves the in-memory
cache out of sync; fix this by making a deep copy of oc.loginState (type
loginRuntimeState) inside updateLoginState, call fn on that copy, and only if fn
returns true attempt saveLoginRuntimeState with the copied state; on successful
save replace oc.loginState with the saved copy, otherwise leave oc.loginState
unchanged and return the error. Keep locking via oc.loginStateMu around the
load/copy/replace steps and use loadLoginRuntimeState when oc.loginState is nil
as currently done.
In `@bridges/ai/message_state_db.go`:
- Around line 127-136: The placeholder numbering uses the loop index i which
doesn't account for skipped blank IDs, causing gaps like $4,$6; fix the loop in
the code that builds args/placeholders (the block that iterates messageIDs and
populates args and placeholders) so the placeholder is derived from the actual
arg position rather than i—either keep an explicit counter starting at 4 and
increment only when you append a non-blank ID, or append the ID to args first
and then generate the placeholder with fmt.Sprintf("$%d", len(args)) so the
placeholder number always matches the argument index.
In `@bridges/ai/metadata.go`:
- Around line 95-114: The portal-state gap: add a metadata fallback and
documentation for portal reads—update loadAIPortalState to fallback to bridgev2
metadata (implement an aiPortalStateFromMetadata helper mirroring
aiLoginConfigFromMetadata/loginRuntimeStateFromMetadata) so aichats_portal_state
missing rows will populate Portal state from UserLoginMetadata, and ensure
CustomAgents persistence by retaining fallback in aichats_custom_agents reads to
UserLoginMetadata.CustomAgents; also add a brief comment near loadAILoginConfig,
loadLoginRuntimeState, loadAIPortalState explaining the runtime fallback chain
and call sites must load portal state before use.
In `@bridges/ai/status_text.go`:
- Line 203: The loop in lastAssistantUsage that iterates over history (populated
via oc.getAIHistoryMessages/GetLastNInPortal) is iterating backwards and thus
returns the oldest assistant message with tokens; change the loop to iterate
forward (for i := 0; i < len(history); i++) so it checks newest-first order as
returned by GetLastNInPortal and returns the most recent assistant message with
token usage; update any comments accordingly to reflect newest-first semantics.
In `@bridges/ai/tool_configured.go`:
- Line 57: The call to effectiveLoginMetadata currently uses
context.Background() (via meta =
oc.effectiveLoginMetadata(context.Background())), which detaches DB/metadata
lookups from the caller's cancellation/deadlines; update function signatures and
call-sites so the request context is threaded through: change
effectiveToolConfig to accept a ctx context.Context and use that when calling
effectiveLoginMetadata(ctx), and update effectiveSearchConfig and
effectiveFetchConfig to accept and forward the same ctx into
effectiveToolConfig; finally replace the context.Background() call with the
propagated ctx so all loginConfigSnapshot/database operations use the caller's
context.
---
Outside diff comments:
In `@bridges/ai/chat.go`:
- Around line 46-52: agentsEnabledForLogin currently returns false when
cfg.Agents is nil, causing a mismatch with shouldEnsureDefaultChat which treats
nil as enabled; update agentsEnabledForLogin (in AIClient) to treat a nil Agents
pointer as enabled by returning true when cfg.Agents == nil, otherwise return
the boolean value (*cfg.Agents) from the loginConfigSnapshot result so
default-config logins consistently enable agents.
- Around line 239-268: The response currently sets resp.UserID to
networkid.UserID(agent.ID) which doesn't match how agent ghosts are stored;
update agentContactResponse so the UserID returned for agent contacts uses the
bridge ghost ID via oc.agentUserID(agentID) (using the catalogAgentID result)
instead of networkid.UserID(agent.ID); if catalogAgentID(agent) is empty, keep
the existing fallback behavior, and ensure this change happens before calling
UserLogin.Bridge.GetGhostByID so the ghost hydrate uses the correct ID (refer to
functions agentContactResponse, catalogAgentID, oc.ResolveResponderForAgent, and
oc.agentUserID).
In `@bridges/ai/metadata.go`:
- Around line 313-325: cloneUserLoginMetadata performs a JSON round-trip which
strips non-serialized/transient fields (credentials, caches, profile data,
custom agents, error counters), so update cloneUserLoginMetadata to perform the
JSON marshal/unmarshal for the serializable fields and then explicitly copy the
transient fields from src into the resulting clone (e.g., copy Credentials,
Cache, Profile, CustomAgents, ErrorCounters or their pointer/deep copies as
appropriate) so callers that clone before mutating/persisting don't lose runtime
state; locate and modify cloneUserLoginMetadata to perform the manual transfer
after unmarshal and ensure pointer safety (deep-copy if needed) before returning
the clone.
In `@bridges/codex/constructors.go`:
- Around line 42-55: StartConnector calls aidb.EnsureSchema with cc.bridgeDB()
even when InitConnector left cc.db nil; wrap the EnsureSchema call in a
nil-check so it only runs when cc.bridgeDB() != nil (e.g. assign db :=
cc.bridgeDB(); if db == nil { return nil } else if err := aidb.EnsureSchema(ctx,
db); err != nil { return err }), ensuring StartConnector short-circuits cleanly
in no-DB scenarios and avoids passing nil to aidb.EnsureSchema.
---
Duplicate comments:
In `@bridges/ai/handleai.go`:
- Around line 387-390: Set meta.WelcomeSent = true and persist that change with
oc.savePortalQuiet using a cancellable context (bgCtx from
context.WithTimeout(oc.backgroundContext(ctx), 10*time.Second)) before any code
that emits the welcome notice; ensure you call oc.savePortalQuiet immediately
after setting meta.WelcomeSent and check/handle its error (log/stop the notice
emission) so the notice is only sent if the persistence succeeds.
- Around line 319-321: The AutoGreetingSent flag is being set in-memory but not
guaranteed persisted before sending the greeting; change the order and add error
handling so persistence happens first: set currentMeta.AutoGreetingSent = true,
call oc.savePortalQuiet(bgCtx, current, "auto greeting state") and check its
error (do not call oc.dispatchInternalMessage if save failed), logging or
returning the save error; only after a successful save call
oc.dispatchInternalMessage(..., "auto-greeting", true). This uses the existing
symbols currentMeta.AutoGreetingSent, oc.savePortalQuiet, and
oc.dispatchInternalMessage.
- Around line 456-463: The portal title is only persisted locally
(portal.Name/portal.NameSet and oc.savePortalQuiet) but not pushed to Matrix;
after oc.savePortalQuiet(bgCtx, portal, "room title") call the existing
lifecycle/materialization sync that pushes portal metadata to Matrix (i.e., the
oc method responsible for materializing/syncing portals) so the updated
portal.Title/portal.Name is sent to clients; if no such helper exists, add and
call a method like oc.MaterializePortal(ctx, portal) or
oc.SyncPortalTitleToMatrix(ctx, portal) immediately after saving to trigger the
external update.
In `@bridges/ai/integration_host.go`:
- Around line 238-250: SessionTranscript currently requests the entire portal
message count (CountMessagesInPortal) and passes it unbounded into
getAIHistoryMessages which can blow up memory for long-lived chats; cap the
fetch size by defining a sensible max (e.g. maxHistoryMessages) and clamp the
count before calling getAIHistoryMessages (use min(count, maxHistoryMessages)),
update SessionTranscript to pass the capped value to getAIHistoryMessages and
consider logging or annotating when the returned history was truncated;
reference functions/values: SessionTranscript, CountMessagesInPortal,
getAIHistoryMessages.
In `@bridges/ai/tools.go`:
- Around line 390-397: The routing in executeMessageReact incorrectly treats an
empty emoji as a removal request; change the conditional so only remove == true
forwards to executeMessageReactRemove (i.e., if remove { return
executeMessageReactRemove(...) }), and add explicit validation before attempting
an add reaction that rejects empty or missing emoji (return an error) so
malformed add-reaction calls are rejected instead of sent to
executeMessageReactRemove; reference executeMessageReact and
executeMessageReactRemove when making the change.
---
Nitpick comments:
In `@bridges/ai/image_generation_tool.go`:
- Around line 255-272: The nil check for btc.Client.UserLogin.Metadata in
supportsOpenAIImageGen is redundant because effectiveLoginMetadata() is the
authoritative source; remove the btc.Client.UserLogin.Metadata == nil check from
the initial guard and rely on the existing nil check for loginMeta returned by
btc.Client.effectiveLoginMetadata(context.Background()), keeping the rest of the
logic (including Provider switch, magic proxy credential checks via
loginCredentialAPIKey/loginCredentialBaseURL, and resolveOpenAIAPIKey)
unchanged.
- Around line 623-660: In resolveOpenRouterImageGenEndpoint, remove the
redundant nil check for btc.Client.UserLogin.Metadata from the initial guard
(the check that includes btc.Client.UserLogin.Metadata == nil) because you call
btc.Client.effectiveLoginMetadata(...) later which already handles missing
Metadata; keep the other nil checks (btc, btc.Client, btc.Client.UserLogin)
intact and ensure the function still returns the same false/empty values when
required.
- Around line 280-295: Remove the redundant btc.Client.UserLogin.Metadata nil
check in supportsGeminiImageGen: rely on
effectiveLoginMetadata(context.Background()) for login metadata validation (as
in supportsOpenAIImageGen). Update the initial guard to only check btc,
btc.Client and btc.Client.UserLogin as needed, call
btc.Client.effectiveLoginMetadata(...) and return false if it is nil, then keep
the existing provider switch (ProviderMagicProxy -> false, default -> false);
reference supportsGeminiImageGen, BridgeToolContext,
btc.Client.UserLogin.Metadata, and effectiveLoginMetadata to locate and edit the
code.
- Around line 480-507: Remove the redundant btc.Client.UserLogin.Metadata == nil
check from the initial guard in buildOpenAIImagesBaseURL; keep the nil checks
for btc, btc.Client, and btc.Client.UserLogin only, and rely on loginMeta :=
btc.Client.effectiveLoginMetadata(...) to handle metadata presence and return
the appropriate error if nil—update the initial conditional accordingly so the
function compiles and behavior is consistent with effectiveLoginMetadata.
- Around line 509-533: Remove the redundant nil check for UserLogin.Metadata in
buildGeminiBaseURL: keep the existing nil guards for btc, btc.Client, and
btc.Client.UserLogin but delete the btc.Client.UserLogin.Metadata == nil
condition since effectiveLoginMetadata (btc.Client.effectiveLoginMetadata) is
already called and validated; ensure buildGeminiBaseURL still returns the same
errors when loginMeta is nil and that Provider-specific logic
(ProviderMagicProxy branch, connector.resolveServiceConfig,
normalizeProxyBaseURL, joinProxyPath) remains unchanged.
In `@bridges/ai/logout_cleanup.go`:
- Around line 94-109: Remove the redundant wrapper function bestEffortExec and
replace its uses with direct calls to execDelete; specifically delete the
bestEffortExec declaration and update the six call sites that currently call
bestEffortExec (across delete_chat.go, internal_prompt_db.go, and
login_state_db.go) to call execDelete(ctx, db, logger, query, args...) directly
so argument expansion and behavior remain unchanged.
In `@bridges/ai/mcp_helpers.go`:
- Around line 82-101: The type switch in clearLoginMCPServer does not include a
default branch, making it inconsistent with loginCredentials() and
ensureLoginCredentials(); update the switch in clearLoginMCPServer (the one
switching on owner in function clearLoginMCPServer) to add a defensive default
case that does nothing (no-op) so unsupported owner types are ignored safely;
keep the existing cases for *UserLoginMetadata and *aiLoginConfig and ensure the
default mirrors the harmless behavior used in
loginCredentials()/ensureLoginCredentials().
In `@bridges/ai/media_understanding_runner.go`:
- Line 926: The call to oc.effectiveLoginMetadata currently uses
context.Background() which discards caller tracing/cancellation; update calls
like
oc.connector.resolveServiceConfig(oc.effectiveLoginMetadata(context.Background()))
to pass the upstream context (e.g., the local ctx variable) instead so
effectiveLoginMetadata and resolveServiceConfig receive the real request
context; apply the same change at all similar call sites in this file
(references: effectiveLoginMetadata, resolveServiceConfig).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd0b8b37-f583-4ab0-a29a-c4740330be73
📒 Files selected for processing (70)
.github/workflows/docker-agentremote.yml.github/workflows/go.yml.github/workflows/publish-release.ymlREADME.mdbridges/ai/agentstore.gobridges/ai/bridge_db.gobridges/ai/bridge_info.gobridges/ai/bridge_info_test.gobridges/ai/broken_login_client.gobridges/ai/chat.gobridges/ai/client.gobridges/ai/commands.gobridges/ai/constructors.gobridges/ai/custom_agents_db.gobridges/ai/delete_chat.gobridges/ai/desktop_api_sessions.gobridges/ai/gravatar.gobridges/ai/handleai.gobridges/ai/handlematrix.gobridges/ai/heartbeat_events.gobridges/ai/heartbeat_session.gobridges/ai/image_generation_tool.gobridges/ai/image_understanding.gobridges/ai/integration_host.gobridges/ai/integrations_config.gobridges/ai/login.gobridges/ai/login_config_db.gobridges/ai/login_loaders.gobridges/ai/login_state_db.gobridges/ai/logout_cleanup.gobridges/ai/mcp_client.gobridges/ai/mcp_helpers.gobridges/ai/mcp_servers.gobridges/ai/media_understanding_cli.gobridges/ai/media_understanding_runner.gobridges/ai/message_state_db.gobridges/ai/metadata.gobridges/ai/model_catalog.gobridges/ai/prompt_builder.gobridges/ai/provisioning.gobridges/ai/response_finalization.gobridges/ai/scheduler_heartbeat_test.gobridges/ai/session_store.gobridges/ai/sessions_tools.gobridges/ai/status_text.gobridges/ai/subagent_announce.gobridges/ai/timezone.gobridges/ai/token_resolver.gobridges/ai/tool_approvals_policy.gobridges/ai/tool_configured.gobridges/ai/tool_policy_chain.gobridges/ai/tool_policy_chain_test.gobridges/ai/tools.gobridges/ai/tools_beeper_feedback.gobridges/codex/config.gobridges/codex/connector.gobridges/codex/connector_test.gobridges/codex/constructors.gobridges/openclaw/README.mdbridges/openclaw/gateway_client.gobridges/openclaw/gateway_client_test.gocmd/agentremote/commands.godocker/agentremote/README.mddocs/bridge-orchestrator.mdpkg/agents/prompt.gopkg/aidb/001-init.sqlpkg/aidb/db.gopkg/aidb/db_test.gosdk/room_features.gotools/generate-homebrew-cask.sh
✅ Files skipped from review due to trivial changes (18)
- docs/bridge-orchestrator.md
- bridges/ai/media_understanding_cli.go
- .github/workflows/go.yml
- pkg/agents/prompt.go
- .github/workflows/docker-agentremote.yml
- bridges/ai/tool_policy_chain_test.go
- docker/agentremote/README.md
- bridges/ai/tools_beeper_feedback.go
- bridges/ai/integrations_config.go
- bridges/ai/mcp_client.go
- bridges/ai/tool_policy_chain.go
- tools/generate-homebrew-cask.sh
- README.md
- bridges/openclaw/gateway_client.go
- bridges/codex/config.go
- bridges/openclaw/README.md
- bridges/ai/provisioning.go
- .github/workflows/publish-release.yml
🚧 Files skipped from review as they are similar to previous changes (10)
- bridges/ai/broken_login_client.go
- bridges/ai/bridge_info.go
- bridges/ai/commands.go
- pkg/aidb/db_test.go
- bridges/ai/handlematrix.go
- bridges/ai/login_loaders.go
- bridges/codex/connector_test.go
- bridges/ai/login_config_db.go
- bridges/ai/session_store.go
- bridges/ai/constructors.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-agentremote-cli-docker (amd64)
- GitHub Check: build-docker
- GitHub Check: Lint
- GitHub Check: build-docker
- GitHub Check: Lint
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-16T09:01:24.464Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 71
File: bridges/ai/connector.go:53-63
Timestamp: 2026-03-16T09:01:24.464Z
Learning: In package ai, the AI connector’s configuration type (Config) defines Bridge as a value field of type BridgeConfig (not a pointer). Therefore, accessing oc.Config.Bridge.CommandPrefix in OpenAIConnector.applyRuntimeDefaults (bridges/ai/connector.go) is safe and does not require a nil-check.
Applied to files:
bridges/ai/tool_configured.gobridges/ai/bridge_info_test.gobridges/ai/media_understanding_runner.gobridges/ai/token_resolver.gobridges/ai/bridge_db.gobridges/ai/login.gobridges/codex/constructors.gobridges/codex/connector.gobridges/ai/handleai.gobridges/ai/chat.gobridges/ai/client.go
🔇 Additional comments (46)
bridges/openclaw/gateway_client_test.go (1)
83-84: Nice alignment with production user-agent composition.Using
openClawGatewayUserAgentBase + resolveGatewayClientVersion()makes this test less brittle than a hardcoded literal and keeps it consistent with the gateway client implementation.sdk/room_features.go (1)
74-74: Namespace fallback update looks correct.Switching the default capability ID to the
agentremoteSDK namespace is consistent with the refactor and keeps the fallback aligned with current package identity.bridges/ai/bridge_info_test.go (3)
38-52: Rename and call-site migration are consistent.
TestApplyAIChatsBridgeInfoand the updatedapplyAIChatsBridgeInfo(...)invocation are aligned, and the visible-DM expectations still validate the intended behavior.
58-71: Beeper-room protocol assertion now matches new contract.Switching this assertion to
aiBridgeProtocolIDcorrectly reflects the fixed AI protocol behavior for beeper-addressed rooms.
88-88: Background-room path was correctly migrated too.The
applyAIChatsBridgeInfo(...)call update in the internal/background test keeps normalization coverage intact.cmd/agentremote/commands.go (6)
302-303: Branding update fordoctorcommand looks consistent.Nice cleanup on the user-facing wording here; it aligns with the new AgentRemote Manager naming.
347-358: Centralized command-spec normalization is a solid refactor.Applying the
binaryNamerewrite in one place is clean and helps keep help text/completions in sync.
476-477: Usage banner changes are clear and user-friendly.Good move using branded header text while keeping invocation dynamic via
binaryName.
503-577: Bash completion parameterization is implemented cleanly.All key wiring points now respect
binaryName, which keeps completion generation aligned with renamed binaries.
585-635: Zsh completion updates are consistent and complete.Nice job propagating
binaryNamethrough compdef/function naming and top-level command descriptions.
676-736: Fish completion migration tobinaryNameis thorough.The command target replacement is complete across top-level, positional, and flag completion entries.
bridges/codex/connector.go (1)
53-76: Runtime defaulting migration looks consistent.The switch to the SDK helpers keeps the defaults centralized, and the new client-info fallbacks avoid hardcoded duplicates in this path.
bridges/ai/timezone.go (1)
44-46: Snapshot-based timezone resolution looks correct.The new config-snapshot lookup keeps the existing normalize/fallback behavior intact and safely falls back to env/default when needed.
bridges/ai/bridge_db.go (3)
11-21: AI table-name constants centralization is a good cleanup.Consolidating these names reduces hard-coded drift across DB call sites.
29-29:db_section=ailogger scoping change looks good.This aligns child-DB logs with the new AI schema ownership.
77-82:bridgeDBFromPortalnil guards are solid.The helper is defensive and consistent with the existing login-based DB resolver flow.
bridges/ai/heartbeat_session.go (1)
41-42: Including bridge/login insessionStoreRefis the right direction.This improves cross-login/session isolation compared to agent-only scoping.
bridges/ai/mcp_servers.go (1)
148-148: Snapshot-based MCP token sourcing looks correct.This is consistent with the ongoing move away from direct login metadata reads.
bridges/ai/model_catalog.go (1)
220-220: Using effective login metadata here is a good fix.The call site now matches the newer metadata resolution flow while keeping the existing guard path.
bridges/ai/scheduler_heartbeat_test.go (1)
123-124:aidb.EnsureSchemamigration in test setup looks good.This keeps test initialization aligned with the current AI schema bootstrap API.
bridges/ai/image_understanding.go (1)
81-82: No nil-contract issue;loginStateSnapshotis guaranteed non-nil.The implementation always returns a non-nil
*loginRuntimeState:
ensureLoginStateLoadedhandles load errors by allocating&loginRuntimeState{}(line 239) and never returns nilcloneLoginRuntimeStatechecks for nil input and returns an allocated struct in all cases (lines 50–51)- Therefore,
loginState.ModelCacheat line 85 is accessed on a guaranteed non-nil pointer without panic risk.bridges/ai/image_generation_tool.go (1)
182-186: LGTM!The migration from
loginMetadata(btc.Client.UserLogin)tobtc.Client.effectiveLoginMetadata(context.Background())is consistent with the broader refactor. The nil check onloginMetais preserved.bridges/ai/mcp_helpers.go (1)
68-80: LGTM!The refactor to accept
owner anywithensureLoginCredentials(owner)provides flexibility for both*UserLoginMetadataand*aiLoginConfigtypes.bridges/ai/gravatar.go (1)
184-189: LGTM!The migration to
loginStateSnapshotis correct with proper nil checks for bothloginStateand nestedGravatar/Primaryfields.bridges/ai/sessions_tools.go (3)
118-127: LGTM!The migration to
getAIHistoryMessagesmaintains the existing error handling (only using messages on success) and properly limits the result set.
270-286: LGTM!Consistent migration to
getAIHistoryMessageswith proper error handling that returns an error result on failure.
572-581: LGTM!The migration in
lastMessageTimestampcorrectly handles errors and empty results by returning 0.bridges/ai/tool_approvals_policy.go (1)
17-28: LGTM!The narrowed approval bypass list correctly aligns with the removal of corresponding message tool actions (
reactions,read,member-info,channel-info,list-pins). The remaining exemptions are appropriately limited to read-only operations.bridges/ai/subagent_announce.go (2)
45-74: LGTM!The migration to
getAIHistoryMessagesinreadLatestAssistantReplypreserves the error handling and message filtering logic.
105-138: LGTM!The migration in
buildSubagentStatsLineappropriately ignores errors (using_) since stats are optional and the function already handles empty message slices gracefully.bridges/ai/token_resolver.go (1)
185-225: LGTM!The refactor cleanly separates metadata-based resolution from config-based resolution. The extracted
resolveProviderAPIKeyForConfigmaintains the same provider-specific logic while enabling config-driven API key resolution.bridges/ai/logout_cleanup.go (1)
12-91: LGTM on expanded cleanup coverage.The comprehensive cleanup now covers all AI-specific tables, and the in-memory state reset ensures the client is properly cleared on logout.
bridges/ai/prompt_builder.go (1)
164-172: Internal prompt rows can evict chat history from replay.The
hr.limitis applied after merging internal prompt rows intocandidates. This means a burst of internal prompt rows could push out actual user/assistant messages entirely.Additionally, the image-injection logic at line 201 (
i < maxHistoryImageMessages) now counts internal rows too, potentially corrupting the vision injection window.Consider:
- Tracking internal rows separately and not counting them toward
hr.limit- Using a counter of non-internal entries for image injection rather than the loop index
bridges/ai/agentstore.go (2)
547-547:CreateRoomreturns success even when portal persistence fails silently.
savePortalQuietlogs errors but does not return them. The caller receives a successful room creation response even if metadata persistence failed. This could lead to inconsistent state where the Matrix room exists but portal metadata is missing.Consider using a persistence function that returns errors, or accepting this as a trade-off for user experience (room works but some metadata may be missing).
36-66: LGTM - Clean migration to login-scoped table helpers.The
LoadAgentsfunction properly:
- Accepts context parameter for DB operations
- Loads custom agents via
listCustomAgentsForLogin- Gates presets based on provider
- Handles nil checks appropriately
bridges/ai/delete_chat.go (1)
62-93: LGTM - Comprehensive cleanup of persisted session artifacts.The delete function now properly cleans up multiple tables:
- AI sessions
- System events
- Portal state
- Message state
This aligns with the migration to DB-backed tables for portal/session state.
Regarding the past review concern about login-level portal references: given that this PR migrates away from in-memory
UserLoginmetadata to DB-backed tables, stale references in login metadata should no longer be a concern as long as the new tables (aiPortalStateTable,aichats_message_state) are the source of truth.bridges/ai/media_understanding_runner.go (1)
601-601: Temp directory prefix updated to match new branding.The change from
agentremote-media-*toaichats-media-*aligns with the broader rename in this PR.bridges/ai/response_finalization.go (2)
32-47: LGTM - SDK migration for continuation messages.The function correctly migrates from
agentremote.EventTimingtosdk.EventTimingand usessdk.BuildContinuationMessage. The timing parameters are properly passed through.
135-137: LGTM - Proper session store keying with bridge and login IDs.The heartbeat session store reference now correctly derives
BridgeIDandLoginIDfromloginDBContext(oc), aligning with the broader session-store changes. This ensures proper scoping of heartbeat state per login.bridges/ai/heartbeat_events.go (2)
61-66: LGTM - Robust composite key for heartbeat login keying.The new
heartbeatLoginKeyfunction properly creates a composite key fromBridgeID|LoginIDwith appropriate nil checks. This is more robust than usingUserLoginIDalone.
117-126: LGTM - Proper state update with deduplication.The persistence logic correctly:
- Compares the new event against the previous state
- Only persists if there's a meaningful change
- Returns
falseto skip unnecessary writes when events are identicalbridges/ai/login.go (3)
33-36: LGTM - SDK error helper migration.The login error definitions properly migrate to
sdk.NewLoginRespErrorwith appropriate status codes and error codes.
213-257: LGTM - Robust login config handling with proper persistence.The login flow now:
- Loads existing config for relogin scenarios (line 219)
- Handles credentials via
aiLoginConfig(lines 239-242)- Validates using the new
loginMetadataViewpattern (line 243)- Properly persists config with error handling (lines 255-257)
Good addition of the
SAVE_LOGIN_FAILEDerror case.
322-337: The implementation correctly handles credential resolution for new logins.The
loginMetadataViewfunction properly reads credentials fromcfg.Credentialsand clones them into the temporary metadata object, which is then used in the API key resolution. The null-safety check forcfgensures this works correctly even when the configuration is not yet fully initialized.pkg/aidb/db.go (1)
27-39: LGTM — Simplified schema initialization for unreleased bridge.The
EnsureSchemafunction is appropriate for a bridge that hasn't been released yet. The comment clearly documents the rationale, and the schema file correctly uses idempotent statements (CREATE TABLE IF NOT EXISTS,CREATE INDEX IF NOT EXISTS) that allow safe re-application on every startup.Once this bridge is released, implement proper migration logic if schema changes are required.
pkg/aidb/001-init.sql (1)
1-2: Plan migration strategy before first release.The bridge has not been released yet (as documented in the code), so the concern about existing installs breaking is not currently applicable. However,
EnsureSchemaonly applies001-init.sqlwithout any incremental migration support. Once the first release ships, any subsequent schema changes will have no path to upgrade existing databases. Add a migration or version-tracking mechanism now rather than waiting for the first schema update request.> Likely an incorrect or invalid review comment.
Change dispatch/queue flow to return errors and centralize status events handling. Key changes: - dispatchOrQueueCore now returns error instead of bool and no longer saves messages directly; callers save messages and handle responses/errors. - Removed queueAcceptedStatusKey and related logic; status events are now propagated via statusEventsKey and statusEventsFromContext. - Added buildUserMessageResponse and postSaveUserMessage to centralize post-save persistence, AI-turn persistence and session mutation notifications. - notifyMatrixSendFailure now uses status events from context and sends status for each explicit event. - Updated handlers (matrix, media, internal dispatch, debounce) to call dispatchOrQueueCore and handle returned errors; pending/pendingSent logic adjusted. - Simplified queue runtime logic (dispatchPromptRun/processing) and removed some pending-status side effects from dispatchOrQueueCore. - Updated tests to match new behavior and expectations (queue status, dispatch/queue error handling). - Minor SDK change: Turn.buildRelatesTo simplified to use SetThread/SetReplyTo helpers. These changes consolidate status propagation, improve error handling for dispatch/queue operations, and centralize message post-save logic.
Introduce tracking and persistence of accepted user messages tied to room-run state and improve queued message status handling. Key changes: - Add acceptedMessage to pendingQueueItem and record accepted messages in roomRunState.acceptedMessages. - Replace buildUserMessageResponse with persistAcceptedUserMessage and add persistAcceptedRoomRunMessages + consumeRoomRunAcceptedMessages to mirror accepted messages into the AI turn store. - Implement sendPendingMessageStatus to emit Matrix message status events for queued items and use roomRunStatusEvents instead of context-based statusEvents; remove status_events_context.go. - Update dispatch/queue/handler callsites to attach accepted messages to queue items and return Pending responses for enqueued items. - markMessageSendSuccess now persists accepted room-run messages and marks statusSent appropriately. - Adjust tests to use the new room-run status mechanism.
Replace single accepted-message echo flow with direct persistence and Message Status (MSS) handling. Key changes: - Replace pendingQueueItem.acceptedMessage with acceptedMessages []*database.Message and thread this through queue/dispatch preparation. - Remove remote accepted-message echo events; persist accepted messages directly via persistAcceptedUserMessage and send success MSS using new sendSuccessMessageStatus/consumeRoomRunStatusEvents helpers. - Add room run helpers: consumeRoomRunStatusEvents, roomRunAccepted, and streamingState.accepted flag with markAccepted/isAccepted. - Introduce markTurnAccepted to mark a turn accepted, trigger acceptance side-effects, and start the UI placeholder send once (idempotent). - Capture current user-visible text into streamingState.currentUserMessage using promptCurrentUserVisibleText and populate it before streaming runs. - Simplify runAgentLoopStreamStep signature (remove portal/evt/shouldMarkSuccess) and update call sites. - Add/adjust tests for accepted-message collection, status event draining, prompt text extraction, acceptance persistence/MSS sending, and placeholder send idempotency. - Update testMatrixConnector to record sent MessageStatus and MessageStatusEventInfo for assertions. These changes consolidate acceptance logic, ensure accepted messages are saved to the DB, and ensure Matrix message status events are sent appropriately when agent turns are accepted.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bridges/ai/pending_queue.go (2)
383-387:⚠️ Potential issue | 🔴 CriticalHandle overflow-summary candidates with an empty
itemsslice.This branch allows
droppedCount > 0even whensnapshot.itemsis empty, but it still readssnapshot.items[0]before checkingsnapshot.lastItem. Once the queue has been drained and only the overflow summary remains, that becomes a panic.Possible fix
- item := snapshot.items[0] - if snapshot.lastItem != nil { - item = *snapshot.lastItem - } + var item pendingQueueItem + switch { + case snapshot.lastItem != nil: + item = *snapshot.lastItem + case len(snapshot.items) > 0: + item = snapshot.items[0] + default: + return nil + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/pending_queue.go` around lines 383 - 387, The code reads snapshot.items[0] before ensuring the slice is non-empty which can panic when droppedCount>0 but items is empty; update the branch handling snapshot.dropPolicy == airuntime.QueueDropSummarize to first check len(snapshot.items)>0 and use snapshot.items[0] only in that case, otherwise fall back to snapshot.lastItem if non-nil (or skip/return early if both are absent) so that accesses to snapshot.items and snapshot.lastItem are safe (refer to snapshot.dropPolicy, snapshot.droppedCount, snapshot.items, snapshot.lastItem in the pending queue logic).
444-450:⚠️ Potential issue | 🟠 MajorStrip real-message side effects from synthetic overflow summaries.
This synthetic path rewrites the carrier item's body/event, but it leaves
acceptedMessages,pending.StatusEvents, and ack metadata intact. The overflow-summary run can therefore persist/send success/remove ack for a real queued message before that message actually executes, and then do it again when the real item is dequeued later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/pending_queue.go` around lines 444 - 450, The synthetic-summary branch currently mutates the real carrier item (candidate.items[0]) leaving acceptedMessages, pending.StatusEvents, and ack-related metadata intact, which can cause premature acks or duplicate sends; instead, create a newItem copy of item (do not modify candidate.items[0]) and mutate only newItem: set newItem.pending.Event = nil, newItem.pending.MessageBody = candidate.summaryPrompt, newItem.backlogAfter = false, newItem.allowDuplicate = false, and explicitly clear newItem.acceptedMessages, newItem.pending.StatusEvents and any ack-related fields on newItem.pending (reset/remove Ack* or similar metadata) before returning newItem, candidate.summaryPrompt, true.
♻️ Duplicate comments (9)
bridges/ai/agentstore.go (2)
82-95:⚠️ Potential issue | 🟡 MinorGuard write helpers before dereferencing
s.client.UserLogin.On Line 88 and Line 94,
saveAgent/deleteAgentcan panic for nil/partially initialized adapters, while the read helpers already handle this safely.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) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/agentstore.go` around lines 82 - 95, The saveAgent and deleteAgent methods currently dereference s.client.UserLogin and can panic when the adapter or its client is nil; before acquiring s.mu or calling saveCustomAgentForLogin/deleteCustomAgentForLogin, mirror the read-helper guards: check if s == nil || s.client == nil || s.client.UserLogin == "" (or whatever empty-login sentinel the read helpers use) and return nil (or the same no-op behavior used by the read helpers) if so, otherwise proceed to lock and call saveCustomAgentForLogin/deleteCustomAgentForLogin; reference the AgentStoreAdapter methods saveAgent and deleteAgent and the functions saveCustomAgentForLogin and deleteCustomAgentForLogin to locate where to add these pre-checks.
518-529:⚠️ Potential issue | 🟡 MinorNormalize
updates.Namebefore applying room updates.Line 518 currently accepts whitespace-only names and can propagate blank values to room metadata/state.
Suggested fix
- if updates.Name != "" { - name := updates.Name + if name := strings.TrimSpace(updates.Name); name != "" { if portal.MXID != "" { portal.UpdateInfo(ctx, &bridgev2.ChatInfo{ Name: &name, ExcludeChangesFromTimeline: true, }, b.client.UserLogin, nil, time.Time{}) } else { portal.Name = name portal.NameSet = true } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/agentstore.go` around lines 518 - 529, Trim and validate updates.Name before applying: replace direct use of updates.Name with a trimmed variable (e.g., name := strings.TrimSpace(updates.Name)), and only proceed if name != "" — call portal.UpdateInfo(ctx, &bridgev2.ChatInfo{Name: &name, ...}) when portal.MXID != "" and otherwise set portal.Name = name and portal.NameSet = true; ensure you import strings if needed and do not apply/update when the trimmed name is empty to avoid propagating whitespace-only names.bridges/ai/login_loaders.go (2)
89-102:⚠️ Potential issue | 🔴 CriticalStop if the fallback still leaves
metanil.
loginMetadata(login)can also return nil, so Line 102 can dereferencemeta.Providerand crash instead of marking the login unusable.Proposed 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 { var err error cfg, err = loadAILoginConfig(ctx, login)🤖 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 89 - 102, loginMetadata(login) can still return nil so dereferencing meta.Provider later is unsafe; update the logic around meta and cfg (in the block handling meta := loginMetadata(login), cfg := loadAILoginConfig(...), and the subsequent key := oc.resolveProviderAPIKeyForConfig(...)) to check whether meta is still nil after calling loginMetadata and, if so, return a clear error (or mark the login unusable) before calling oc.resolveProviderAPIKeyForConfig; ensure the new guard references meta (the variable from loginMetadata(login)) and avoids using meta.Provider when meta == nil.
24-25:⚠️ Potential issue | 🔴 CriticalGuard
loginMetadata(existing.UserLogin)before reading.Provider.
existing.UserLogin != nildoes not guarantee the metadata helper returns a value. A cached login with missing/corrupt metadata will still panic here during the rebuild check.Proposed fix
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 24 - 25, Guard the call to loginMetadata before reading .Provider: call meta := loginMetadata(existing.UserLogin) and check that meta is non-nil (and/or meta.Provider is non-empty) before doing strings.TrimSpace(meta.Provider); assign existingProvider only when meta is valid, otherwise leave existingProvider empty or handle the missing/corrupt metadata path so the rebuild check cannot panic. Ensure you update the branch that references existing.UserLogin and existingProvider so corrupted cached logins are skipped safely.bridges/ai/client.go (1)
524-617:⚠️ Potential issue | 🟠 MajorTreat accepted-message persistence as part of acceptance.
upsertTransportPortalMessageandpersistAIConversationMessageare still best-effort here. If either write fails, the run keeps going and downstream code can still emit success, leaving future history/session reads out of sync with what was actually accepted. This is the same acceptance-vs-persistence bug that was previously flagged, just through the newpersistAcceptedUserMessage/postSaveUserMessagepath.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/client.go` around lines 524 - 617, The acceptance flow currently treats persistence as best-effort: persistAcceptedUserMessage calls upsertTransportPortalMessage and then postSaveUserMessage (which in turn calls persistAIConversationMessage) but ignores errors, allowing the API to report success even if DB writes fail; change persistAcceptedUserMessage to treat these writes as part of acceptance by checking and returning errors from upsertTransportPortalMessage and persistAIConversationMessage (do not call postSaveUserMessage for the acceptance path or change postSaveUserMessage to return an error), i.e., call persistAIConversationMessage directly from persistAcceptedUserMessage, propagate any error back to the caller, and ensure callers of persistAcceptedUserMessage handle returned errors so failed persistence causes acceptance to fail.bridges/ai/chat.go (2)
979-992:⚠️ Potential issue | 🟠 MajorDon't ignore agent-portal persistence failures.
This mutates durable portal identity/avatar state, but
savePortalQuiet(...)only logs andcreateChat(...)keeps going. If that write drops, room materialization proceeds with in-memory agent config only and can reopen with the wrong persisted target later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/chat.go` around lines 979 - 992, The portal persistence call savePortalQuiet currently swallows failures; change this so the write is checked and propagated: have oc.savePortalQuiet (or replace with a savePortal that returns an error) surface any error and make the caller (e.g., createChat flow that calls setPortalResolvedTarget/oc.savePortalQuiet) return/handle that error instead of proceeding; specifically, after calling setPortalResolvedTarget and computing portal.AvatarID/AvatarMXC, call a save method that returns an error, check the error and abort/return it (or retry) so room materialization never continues with only in-memory agent config.
551-568:⚠️ Potential issue | 🟠 MajorResolve the model responder before creating the DM.
createChat(...)can already create/materialize the room beforeresolveResponder(...)fails here, so callers see an error even though the chat now exists. Resolve first, or fall back to defaultUserInfoinstead of erroring after the side effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/chat.go` around lines 551 - 568, The createChat call (oc.createChat) can materialize a room before resolveResponder (oc.resolveResponder) fails, producing an error after a side-effect; fix by resolving the model responder first (call oc.resolveResponder with the PortalMetadata/ResolvedTarget for modelID and ResponderResolveOptions before calling oc.createChat), and if reordering isn't possible, catch resolveResponder errors and fall back to a default responder/UserInfo (do not return an error after createChat succeeds) while logging the failure; update the logic around createChat, resolveResponder, PortalMetadata, ResolvedTarget, and ResponderResolveOptions to ensure no created chat results in a hard error.bridges/ai/handlematrix.go (1)
399-401:⚠️ Potential issue | 🟠 MajorStop the edit flow when AI-history persistence fails.
After this write fails, the code still mirrors metadata into the bridge row and may regenerate from stale AI history. Return the error here instead of continuing with a partially applied edit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handlematrix.go` around lines 399 - 401, The edit flow continues after persistAIConversationMessage fails, causing partial application and potential regeneration from stale AI history; update the callers around persistAIConversationMessage (the block that currently logs via oc.loggerForContext(ctx).Warn().Err(err).Msg("Failed to persist edited conversation turn")) to return the error immediately instead of just warning and proceeding to mirror metadata into the bridge row or continue the edit flow—i.e., propagate the error from persistAIConversationMessage back to the caller so subsequent operations (metadata mirroring/regeneration) do not run on a partially applied edit.bridges/ai/handleai.go (1)
223-229:⚠️ Potential issue | 🟠 MajorDon't send the disclaimer before its one-shot flag is durable.
If
sendSystemNoticeMessagesucceeds andsavePortalfails,DisclaimerSentstays false and the next retry/restart can emit the disclaimer again. This is the same one-shot persistence problem that already showed up in the greeting/welcome flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handleai.go` around lines 223 - 229, The disclaimer is being sent before its one-shot flag is persisted, so if sendSystemNoticeMessage succeeds but oc.savePortal fails the disclaimer can be re-sent; change the order to mark the portal metadata durable first: set meta.DisclaimerSent = true, call oc.savePortal(bgCtx, portal, "disclaimer state") and ensure it succeeds, then call oc.sendSystemNoticeMessage(bgCtx, portal, disclaimer). If the send fails after persistence, decide whether to clear the flag (meta.DisclaimerSent = false) and re-save or log/handle the inconsistency; reference the sendSystemNoticeMessage, savePortal, and meta.DisclaimerSent symbols when making the change.
🧹 Nitpick comments (5)
bridges/ai/media_send.go (1)
94-127: Clean migration to sdk.SendViaPortal.Both call sites correctly populate
SendViaPortalParamswith explicit timing values. The duplicated param construction is acceptable given the differentConvertedpayloads and keeps each code path self-contained.If you want to reduce repetition, you could extract a helper that builds base params, but this is minor and optional.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/media_send.go` around lines 94 - 127, There are two duplicated constructions of sdk.SendViaPortalParams around the calls to sdk.SendViaPortal with only the Converted payload differing; extract a small helper (e.g., buildSendViaPortalParams or sendViaPortalWithConverted) that fills common fields (Login, Portal, Sender, IDPrefix, LogKey, StreamOrder and Timestamp via time.Now()) and accepts the varying Converted *bridgev2.ConvertedMessage (and optionally timestamp) so both call sites call the helper and then invoke sdk.SendViaPortal with the returned params to remove repetition while preserving behavior; update the two call sites that currently create SendViaPortalParams directly to use this helper.bridges/ai/commands.go (1)
148-164: Potential race in transition detection.
currentlyEnabledis captured at line 152 outside the mutex, while the callback at lines 153-160 executes under the mutex. If another goroutine modifies the config between these operations, the return value at line 163 could incorrectly report a transition (e.g., returningtruewhen this call didn't actually enable agents).The persistence logic is correct since the callback re-checks state under the lock, but the return value drives downstream behavior (e.g., "should create default chat").
♻️ Thread-safe transition detection
func applyAgentsEnabledChange(ctx context.Context, client *AIClient, enabled bool) (bool, error) { if client == nil { return false, nil } - currentlyEnabled := agentsEnabledForLoginConfig(client.loginConfigSnapshot(ctx)) + var wasTransition bool if err := client.updateLoginConfig(ctx, func(cfg *aiLoginConfig) bool { current := agentsEnabledForLoginConfig(cfg) if current == enabled && cfg.Agents != nil { + wasTransition = false return false } + wasTransition = enabled && !current cfg.Agents = &enabled return true }); err != nil { return false, err } - return enabled && !currentlyEnabled, nil + return wasTransition, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/commands.go` around lines 148 - 164, The transition-detection races because currentlyEnabled is read outside the updateLoginConfig mutex; move the read inside the locked callback by declaring a local variable (e.g., prevEnabled) in applyAgentsEnabledChange, assign prevEnabled = agentsEnabledForLoginConfig(cfg) inside the client.updateLoginConfig callback before mutating cfg.Agents, and then use that closure-set prevEnabled after update to compute and return the actual transition (enabled && !prevEnabled); keep the nil client guard and preserve error handling from client.updateLoginConfig.bridges/ai/internal_dispatch.go (1)
75-79: Redundant nil check foroc.Line 24 already returns early when
oc == nil, making theoc != nilcheck at line 76 redundant. Theoc.connector != nilpart is still needed.♻️ Suggested simplification
var cfg *Config - if oc != nil && oc.connector != nil { + if oc.connector != nil { cfg = &oc.connector.Config }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/internal_dispatch.go` around lines 75 - 79, The code redundantly checks oc != nil before accessing oc.connector; since earlier logic already returns when oc == nil, simplify the assignment to only guard on oc.connector by changing the block that sets cfg to check oc.connector != nil and then set cfg = &oc.connector.Config (i.e., replace the `if oc != nil && oc.connector != nil` conditional with `if oc.connector != nil`) so resolveQueueSettings(queueResolveParams{cfg: cfg, ...}) receives the same cfg only when the connector exists.bridges/ai/heartbeat_delivery_test.go (1)
98-103: Redundant assertion on lines 101-103.The assertion on line 101-103 checks
route.SessionPortal.MXID != lastPortal.MXID, but this is already verified by the assertion on lines 98-100 which checksroute.SessionPortal.MXID != lastPortal.MXID. Both assertions fail on the same condition.Remove redundant assertion
if route.SessionPortal == nil || route.SessionPortal.MXID != lastPortal.MXID { t.Fatalf("expected last active portal fallback to %q, got %#v", lastPortal.MXID, route.SessionPortal) } - if route.SessionPortal.MXID != lastPortal.MXID { - t.Fatalf("expected last active room %q, got %q", lastPortal.MXID, route.SessionPortal.MXID) - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/heartbeat_delivery_test.go` around lines 98 - 103, The second redundant assertion block should be removed: the first check (if route.SessionPortal == nil || route.SessionPortal.MXID != lastPortal.MXID) already verifies both nil and MXID mismatch for route.SessionPortal vs lastPortal.MXID, so delete the subsequent if block that again checks route.SessionPortal.MXID != lastPortal.MXID to avoid duplicate failure paths in the Test (heartbeat_delivery_test.go) — keep the combined nil+MXID check and remove the lone MXID-only assertion.bridges/ai/login_loaders.go (1)
134-139: Remove the second rebinding block afterpublishOrReuseClient.
publishOrReuseClientalready attaches the returned client tologinon every non-nil path, so this block only duplicates that contract.Proposed cleanup
- chosen := oc.publishOrReuseClient(login, client, existing) - if chosen != nil { - chosen.UserLogin = login - chosen.ClientBase.SetUserLogin(login) - login.Client = chosen - } + oc.publishOrReuseClient(login, client, existing) return nil🤖 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 134 - 139, The block that rebinds the returned client after calling publishOrReuseClient is redundant — publishOrReuseClient already attaches the chosen client to login — so remove the entire if chosen != nil { chosen.UserLogin = login; chosen.ClientBase.SetUserLogin(login); login.Client = chosen } block; rely on publishOrReuseClient's contract instead of duplicating assignments to chosen.UserLogin, chosen.ClientBase.SetUserLogin, and login.Client.
🤖 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`:
- Around line 496-499: CreateRoom is populating chatCreateParams with RoomName
instead of the Title field that createChat (in chat.go) actually consumes;
update the call in CreateRoom (where b.client.createChat is invoked) to set
chatCreateParams.Title = room.Name (and remove or stop using RoomName) so the
requested room name is passed through to createChat correctly.
In `@bridges/ai/client.go`:
- Around line 1866-1873: The error path after
buildPromptContextForPendingMessage can panic because last.Meta is optional and
only entries[0].AckEventID is removed; fix by guarding access to last.Meta
(e.g., check last.Meta != nil) before reading AckReactionRemoveAfter, and
instead of only removing entries[0], iterate over all entries and call
removeAckReactionByID(ctx, last.Portal, entry.AckEventID) for each non-empty
AckEventID when prompt construction fails; keep the existing logging
(loggerForContext) and notifyMatrixSendFailure behavior, perform the removals
before returning, and ensure no nil dereference occurs.
In `@bridges/ai/handleai.go`:
- Around line 261-279: Currently getAIHistoryMessages(bgCtx, portal, 10) is
called unconditionally even when userMessageHint is provided; move that call
into the fallback branch so history is only fetched if userMessageHint is empty.
Change the flow in handleai.go to first set userMessage :=
strings.TrimSpace(userMessageHint) and if userMessage == "" then call
oc.getAIHistoryMessages(bgCtx, portal, 10), handle the error with
oc.loggerForContext(ctx).Warn() as before, and then iterate messages (using
MessageMetadata.Role/Body checks) to populate userMessage; keep existing
logging/return behavior on fetch error.
- Around line 127-146: The issue is that crossedThreshold/recovered are derived
before the DB write so BridgeState.Send may be emitted for a state change that
never persisted; fix by making updateLoginState return whether the state was
successfully persisted (e.g., a committed bool) and only emit
oc.UserLogin.BridgeState.Send(...) when both the change flag (crossedThreshold
or recovered) AND the commit result are true; update calls in
recordProviderError and AIClient.recordProviderSuccess to check the commit
result from updateLoginState before sending BridgeState, referencing
updateLoginState, recordProviderError/recordProviderSuccess, crossedThreshold
and recovered.
In `@bridges/ai/heartbeat_delivery_test.go`:
- Around line 79-104: The test
TestResolveHeartbeatRouteFallsBackFromMismatchedExplicitSessionRoom is missing
client.SetLoggedIn(true), causing resolveHeartbeatRoute/resolveHeartbeatDelivery
to potentially return deliveryTarget with Reason "channel-not-ready" when
!IsLoggedIn(); either call client.SetLoggedIn(true) before invoking
client.resolveHeartbeatRoute to match the other tests and ensure
route.SessionPortal is resolved as expected, or explicitly assert route.Delivery
to validate the unauthenticated behavior — update the test to use
client.SetLoggedIn(true) if you intend to verify portal fallback rather than
delivery behavior.
In `@bridges/ai/messages.go`:
- Around line 90-112: The helper promptCurrentUserVisibleText currently falls
back to scanning prompt.Messages when the CurrentTurnData has no text parts;
change it so that if prompt.CurrentTurnData is present you only consider it
(return joined text or empty string) and do NOT scan history — only perform the
prompt.Messages backward scan when CurrentTurnData is absent (nil). Update
promptCurrentUserVisibleText to first check presence of prompt.CurrentTurnData,
collect and return its text parts (possibly empty string), and move the Messages
fallback into the else branch so previous user turns aren't used for non-text
current turns.
---
Outside diff comments:
In `@bridges/ai/pending_queue.go`:
- Around line 383-387: The code reads snapshot.items[0] before ensuring the
slice is non-empty which can panic when droppedCount>0 but items is empty;
update the branch handling snapshot.dropPolicy == airuntime.QueueDropSummarize
to first check len(snapshot.items)>0 and use snapshot.items[0] only in that
case, otherwise fall back to snapshot.lastItem if non-nil (or skip/return early
if both are absent) so that accesses to snapshot.items and snapshot.lastItem are
safe (refer to snapshot.dropPolicy, snapshot.droppedCount, snapshot.items,
snapshot.lastItem in the pending queue logic).
- Around line 444-450: The synthetic-summary branch currently mutates the real
carrier item (candidate.items[0]) leaving acceptedMessages,
pending.StatusEvents, and ack-related metadata intact, which can cause premature
acks or duplicate sends; instead, create a newItem copy of item (do not modify
candidate.items[0]) and mutate only newItem: set newItem.pending.Event = nil,
newItem.pending.MessageBody = candidate.summaryPrompt, newItem.backlogAfter =
false, newItem.allowDuplicate = false, and explicitly clear
newItem.acceptedMessages, newItem.pending.StatusEvents and any ack-related
fields on newItem.pending (reset/remove Ack* or similar metadata) before
returning newItem, candidate.summaryPrompt, true.
---
Duplicate comments:
In `@bridges/ai/agentstore.go`:
- Around line 82-95: The saveAgent and deleteAgent methods currently dereference
s.client.UserLogin and can panic when the adapter or its client is nil; before
acquiring s.mu or calling saveCustomAgentForLogin/deleteCustomAgentForLogin,
mirror the read-helper guards: check if s == nil || s.client == nil ||
s.client.UserLogin == "" (or whatever empty-login sentinel the read helpers use)
and return nil (or the same no-op behavior used by the read helpers) if so,
otherwise proceed to lock and call
saveCustomAgentForLogin/deleteCustomAgentForLogin; reference the
AgentStoreAdapter methods saveAgent and deleteAgent and the functions
saveCustomAgentForLogin and deleteCustomAgentForLogin to locate where to add
these pre-checks.
- Around line 518-529: Trim and validate updates.Name before applying: replace
direct use of updates.Name with a trimmed variable (e.g., name :=
strings.TrimSpace(updates.Name)), and only proceed if name != "" — call
portal.UpdateInfo(ctx, &bridgev2.ChatInfo{Name: &name, ...}) when portal.MXID !=
"" and otherwise set portal.Name = name and portal.NameSet = true; ensure you
import strings if needed and do not apply/update when the trimmed name is empty
to avoid propagating whitespace-only names.
In `@bridges/ai/chat.go`:
- Around line 979-992: The portal persistence call savePortalQuiet currently
swallows failures; change this so the write is checked and propagated: have
oc.savePortalQuiet (or replace with a savePortal that returns an error) surface
any error and make the caller (e.g., createChat flow that calls
setPortalResolvedTarget/oc.savePortalQuiet) return/handle that error instead of
proceeding; specifically, after calling setPortalResolvedTarget and computing
portal.AvatarID/AvatarMXC, call a save method that returns an error, check the
error and abort/return it (or retry) so room materialization never continues
with only in-memory agent config.
- Around line 551-568: The createChat call (oc.createChat) can materialize a
room before resolveResponder (oc.resolveResponder) fails, producing an error
after a side-effect; fix by resolving the model responder first (call
oc.resolveResponder with the PortalMetadata/ResolvedTarget for modelID and
ResponderResolveOptions before calling oc.createChat), and if reordering isn't
possible, catch resolveResponder errors and fall back to a default
responder/UserInfo (do not return an error after createChat succeeds) while
logging the failure; update the logic around createChat, resolveResponder,
PortalMetadata, ResolvedTarget, and ResponderResolveOptions to ensure no created
chat results in a hard error.
In `@bridges/ai/client.go`:
- Around line 524-617: The acceptance flow currently treats persistence as
best-effort: persistAcceptedUserMessage calls upsertTransportPortalMessage and
then postSaveUserMessage (which in turn calls persistAIConversationMessage) but
ignores errors, allowing the API to report success even if DB writes fail;
change persistAcceptedUserMessage to treat these writes as part of acceptance by
checking and returning errors from upsertTransportPortalMessage and
persistAIConversationMessage (do not call postSaveUserMessage for the acceptance
path or change postSaveUserMessage to return an error), i.e., call
persistAIConversationMessage directly from persistAcceptedUserMessage, propagate
any error back to the caller, and ensure callers of persistAcceptedUserMessage
handle returned errors so failed persistence causes acceptance to fail.
In `@bridges/ai/handleai.go`:
- Around line 223-229: The disclaimer is being sent before its one-shot flag is
persisted, so if sendSystemNoticeMessage succeeds but oc.savePortal fails the
disclaimer can be re-sent; change the order to mark the portal metadata durable
first: set meta.DisclaimerSent = true, call oc.savePortal(bgCtx, portal,
"disclaimer state") and ensure it succeeds, then call
oc.sendSystemNoticeMessage(bgCtx, portal, disclaimer). If the send fails after
persistence, decide whether to clear the flag (meta.DisclaimerSent = false) and
re-save or log/handle the inconsistency; reference the sendSystemNoticeMessage,
savePortal, and meta.DisclaimerSent symbols when making the change.
In `@bridges/ai/handlematrix.go`:
- Around line 399-401: The edit flow continues after
persistAIConversationMessage fails, causing partial application and potential
regeneration from stale AI history; update the callers around
persistAIConversationMessage (the block that currently logs via
oc.loggerForContext(ctx).Warn().Err(err).Msg("Failed to persist edited
conversation turn")) to return the error immediately instead of just warning and
proceeding to mirror metadata into the bridge row or continue the edit
flow—i.e., propagate the error from persistAIConversationMessage back to the
caller so subsequent operations (metadata mirroring/regeneration) do not run on
a partially applied edit.
In `@bridges/ai/login_loaders.go`:
- Around line 89-102: loginMetadata(login) can still return nil so dereferencing
meta.Provider later is unsafe; update the logic around meta and cfg (in the
block handling meta := loginMetadata(login), cfg := loadAILoginConfig(...), and
the subsequent key := oc.resolveProviderAPIKeyForConfig(...)) to check whether
meta is still nil after calling loginMetadata and, if so, return a clear error
(or mark the login unusable) before calling oc.resolveProviderAPIKeyForConfig;
ensure the new guard references meta (the variable from loginMetadata(login))
and avoids using meta.Provider when meta == nil.
- Around line 24-25: Guard the call to loginMetadata before reading .Provider:
call meta := loginMetadata(existing.UserLogin) and check that meta is non-nil
(and/or meta.Provider is non-empty) before doing
strings.TrimSpace(meta.Provider); assign existingProvider only when meta is
valid, otherwise leave existingProvider empty or handle the missing/corrupt
metadata path so the rebuild check cannot panic. Ensure you update the branch
that references existing.UserLogin and existingProvider so corrupted cached
logins are skipped safely.
---
Nitpick comments:
In `@bridges/ai/commands.go`:
- Around line 148-164: The transition-detection races because currentlyEnabled
is read outside the updateLoginConfig mutex; move the read inside the locked
callback by declaring a local variable (e.g., prevEnabled) in
applyAgentsEnabledChange, assign prevEnabled = agentsEnabledForLoginConfig(cfg)
inside the client.updateLoginConfig callback before mutating cfg.Agents, and
then use that closure-set prevEnabled after update to compute and return the
actual transition (enabled && !prevEnabled); keep the nil client guard and
preserve error handling from client.updateLoginConfig.
In `@bridges/ai/heartbeat_delivery_test.go`:
- Around line 98-103: The second redundant assertion block should be removed:
the first check (if route.SessionPortal == nil || route.SessionPortal.MXID !=
lastPortal.MXID) already verifies both nil and MXID mismatch for
route.SessionPortal vs lastPortal.MXID, so delete the subsequent if block that
again checks route.SessionPortal.MXID != lastPortal.MXID to avoid duplicate
failure paths in the Test (heartbeat_delivery_test.go) — keep the combined
nil+MXID check and remove the lone MXID-only assertion.
In `@bridges/ai/internal_dispatch.go`:
- Around line 75-79: The code redundantly checks oc != nil before accessing
oc.connector; since earlier logic already returns when oc == nil, simplify the
assignment to only guard on oc.connector by changing the block that sets cfg to
check oc.connector != nil and then set cfg = &oc.connector.Config (i.e., replace
the `if oc != nil && oc.connector != nil` conditional with `if oc.connector !=
nil`) so resolveQueueSettings(queueResolveParams{cfg: cfg, ...}) receives the
same cfg only when the connector exists.
In `@bridges/ai/login_loaders.go`:
- Around line 134-139: The block that rebinds the returned client after calling
publishOrReuseClient is redundant — publishOrReuseClient already attaches the
chosen client to login — so remove the entire if chosen != nil {
chosen.UserLogin = login; chosen.ClientBase.SetUserLogin(login); login.Client =
chosen } block; rely on publishOrReuseClient's contract instead of duplicating
assignments to chosen.UserLogin, chosen.ClientBase.SetUserLogin, and
login.Client.
In `@bridges/ai/media_send.go`:
- Around line 94-127: There are two duplicated constructions of
sdk.SendViaPortalParams around the calls to sdk.SendViaPortal with only the
Converted payload differing; extract a small helper (e.g.,
buildSendViaPortalParams or sendViaPortalWithConverted) that fills common fields
(Login, Portal, Sender, IDPrefix, LogKey, StreamOrder and Timestamp via
time.Now()) and accepts the varying Converted *bridgev2.ConvertedMessage (and
optionally timestamp) so both call sites call the helper and then invoke
sdk.SendViaPortal with the returned params to remove repetition while preserving
behavior; update the two call sites that currently create SendViaPortalParams
directly to use this helper.
🪄 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: 382d7ebb-3ed2-4090-8ded-836cd0526e72
📒 Files selected for processing (56)
bridges/ai/abort_helpers.gobridges/ai/accepted_echo.gobridges/ai/agent_activity.gobridges/ai/agent_activity_test.gobridges/ai/agent_loop_runtime.gobridges/ai/agent_loop_steering_test.gobridges/ai/agentstore.gobridges/ai/chat.gobridges/ai/chat_bootstrap_test.gobridges/ai/client.gobridges/ai/commands.gobridges/ai/commands_test.gobridges/ai/default_chat_test.gobridges/ai/delete_chat.gobridges/ai/handleai.gobridges/ai/handlematrix.gobridges/ai/heartbeat_delivery_test.gobridges/ai/heartbeat_execute.gobridges/ai/identifiers.gobridges/ai/internal_dispatch.gobridges/ai/login_loaders.gobridges/ai/media_send.gobridges/ai/message_send.gobridges/ai/messages.gobridges/ai/metadata.gobridges/ai/metadata_test.gobridges/ai/pending_queue.gobridges/ai/persistence_boundaries_test.gobridges/ai/portal_materialize.gobridges/ai/portal_send.gobridges/ai/portal_send_test.gobridges/ai/queue_runtime.gobridges/ai/queue_status_test.gobridges/ai/response_finalization.gobridges/ai/response_retry.gobridges/ai/room_info_projection_test.gobridges/ai/room_runs.gobridges/ai/scheduler_cron.gobridges/ai/scheduler_rooms.gobridges/ai/session_greeting.gobridges/ai/status_events_context.gobridges/ai/streaming_chat_completions.gobridges/ai/streaming_executor.gobridges/ai/streaming_init_test.gobridges/ai/streaming_responses_api.gobridges/ai/streaming_state.gobridges/ai/streaming_success.gobridges/ai/subagent_spawn.gobridges/ai/test_login_helpers_test.gobridges/ai/tools.gobridges/ai/turn_store.gobridges/ai/typing_state.gobridges/codex/client.gobridges/codex/directory_manager.gobridges/dummybridge/connector_session.gobridges/dummybridge/runtime_commands.go
💤 Files with no reviewable changes (1)
- bridges/ai/default_chat_test.go
✅ Files skipped from review due to trivial changes (2)
- bridges/ai/agent_activity_test.go
- bridges/ai/persistence_boundaries_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- bridges/ai/message_send.go
- bridges/ai/portal_send_test.go
- bridges/ai/identifiers.go
- bridges/ai/chat_bootstrap_test.go
- bridges/ai/agent_loop_steering_test.go
- bridges/ai/delete_chat.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:11.979Z
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:03.718Z
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:56.093Z
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:19.585Z
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:11.979Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/codex/portal_state_db.go:127-140
Timestamp: 2026-04-12T18:51:11.979Z
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/media_send.gobridges/ai/metadata_test.gobridges/ai/portal_materialize.gobridges/ai/portal_send.gobridges/ai/commands.gobridges/ai/agent_activity.gobridges/ai/accepted_echo.gobridges/ai/abort_helpers.gobridges/ai/login_loaders.gobridges/ai/heartbeat_execute.gobridges/ai/heartbeat_delivery_test.gobridges/ai/internal_dispatch.gobridges/ai/handleai.gobridges/ai/agentstore.gobridges/ai/client.gobridges/ai/chat.gobridges/ai/handlematrix.gobridges/ai/metadata.go
📚 Learning: 2026-04-12T17:55:56.093Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/codex/portal_state_db.go:78-92
Timestamp: 2026-04-12T17:55:56.093Z
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/media_send.gobridges/ai/metadata_test.gobridges/ai/portal_materialize.gobridges/ai/portal_send.gobridges/ai/agent_activity.gobridges/ai/accepted_echo.gobridges/ai/agent_loop_runtime.gobridges/ai/abort_helpers.gobridges/ai/login_loaders.gobridges/ai/heartbeat_execute.gobridges/ai/heartbeat_delivery_test.gobridges/ai/internal_dispatch.gobridges/ai/handleai.gobridges/ai/agentstore.gobridges/ai/client.gobridges/ai/chat.gobridges/ai/handlematrix.gobridges/ai/metadata.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/media_send.gobridges/ai/commands_test.gobridges/ai/metadata_test.gobridges/ai/portal_materialize.gobridges/ai/portal_send.gobridges/ai/commands.gobridges/ai/agent_activity.gobridges/ai/messages.gobridges/ai/accepted_echo.gobridges/ai/agent_loop_runtime.gobridges/ai/abort_helpers.gobridges/ai/login_loaders.gobridges/ai/heartbeat_execute.gobridges/ai/heartbeat_delivery_test.gobridges/ai/internal_dispatch.gobridges/ai/pending_queue.gobridges/ai/handleai.gobridges/ai/agentstore.gobridges/ai/client.gobridges/ai/chat.gobridges/ai/handlematrix.gobridges/ai/metadata.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/media_send.gobridges/ai/commands_test.gobridges/ai/metadata_test.gobridges/ai/portal_materialize.gobridges/ai/portal_send.gobridges/ai/commands.gobridges/ai/agent_activity.gobridges/ai/messages.gobridges/ai/accepted_echo.gobridges/ai/agent_loop_runtime.gobridges/ai/abort_helpers.gobridges/ai/login_loaders.gobridges/ai/heartbeat_execute.gobridges/ai/heartbeat_delivery_test.gobridges/ai/internal_dispatch.gobridges/ai/pending_queue.gobridges/ai/handleai.gobridges/ai/agentstore.gobridges/ai/client.gobridges/ai/chat.gobridges/ai/handlematrix.gobridges/ai/metadata.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_test.gobridges/ai/commands.gobridges/ai/login_loaders.gobridges/ai/handleai.gobridges/ai/client.gobridges/ai/metadata.go
📚 Learning: 2026-04-12T17:46:48.384Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/openclaw/metadata.go:310-337
Timestamp: 2026-04-12T17:46:48.384Z
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/metadata.go
🔇 Additional comments (28)
bridges/ai/media_send.go (1)
28-35: LGTM on validation and intent resolution.The early portal validation is correctly placed since the portal and intent are used for upload before reaching
sdk.SendViaPortal. The intent resolution properly handles both failure modes (!okandnilintent).bridges/ai/agent_loop_runtime.go (2)
82-101: LGTM!Clean async wrapper implementation with correct defer ordering (LIFO: cancel → onExit → close(done)). The done channel pattern properly allows callers to either discard (queue_runtime.go) or monitor completion (heartbeat_execute.go). Context management with the inactivity timeout is correctly scoped to the goroutine.
103-131: LGTM!The simplified signature (removing
evt *event.EventandshouldMarkSuccess func(T) bool) aligns with the refactored orchestration where status updates are handled centrally. The stream iteration logic remains correct with proper nil-stream handling and activity touch propagation.bridges/ai/commands.go (3)
13-13: LGTM!The import migration from
bridgesdktosdkand corresponding usage change aligns with the PR's objective of cleaning up SDK usage.Also applies to: 35-35
166-172: LGTM!This wrapper correctly discards the transition boolean since the command handler doesn't need to trigger default chat creation.
182-200: LGTM!The refactor to use
loginConfigSnapshotandapplyAgentsCommandChangealigns with the new config-based state management approach, andstrings.TrimSpaceis a reasonable simplification for the reply formatting.bridges/ai/commands_test.go (3)
3-6: LGTM!Clean import grouping with the added
contextpackage.
49-82: LGTM!Thorough test coverage for the transition detection logic:
- Verifies disable doesn't trigger default chat creation
- Verifies first enable triggers the transition signal
- Verifies subsequent enable calls are no-ops
This directly validates the intended behavior of
applyAgentsEnabledChange.
84-94: LGTM!Good coverage for
applyAgentsCommandChange, verifying it updates config without triggering the additional room bootstrap behavior.bridges/ai/internal_dispatch.go (4)
13-13: SDK migration looks correct.The import and usage of
sdk.NewEventID(prefix)align with the PR objective to migrate fromagentremotetosdkpackage.Also applies to: 42-42
46-51: Prompt context construction is correct.The call to
buildPromptContextForTurnwithcurrentTurnPromptOptionscorrectly wires theincludeLinkScope: truesetting, matching the signature shown in the codebase.
53-55: Resilient error handling for persistence.Logging the persistence error but continuing to dispatch is a reasonable resilience pattern—ensures the user interaction proceeds even if there's a transient database issue. The actual dispatch error is still returned to the caller at line 82.
80-82: No action needed — nil event handling is appropriately guarded.The
evtparameter passed asnilfor internally-generated messages is intentionally designed and handled gracefully throughout the codebase. ThequeueStatusEventsfunction guards against nil events (checkingif evt == nil || evt.ID == ""before processing), and all downstream usages ofpending.Eventinclude proper nil checks before dereferencing.bridges/ai/agentstore.go (1)
53-80: Good migration to login-scoped custom-agent state without legacy fallback.This load path consistently uses login-owned DB helpers and propagates
ctx/errors cleanly.
Based on learnings: For unreleased AI bridge code underbridges/ai, do not add migration paths, legacy-compatibility fallbacks, or dual-read/backfill logic.bridges/ai/agent_activity.go (2)
11-31: LGTM! Clean migration to DB-backed session tracking.The function properly:
- Guards against nil/empty inputs and internal rooms
- Skips activity recording during heartbeat runs (preventing routing feedback loops)
- Derives
storeAgentIDviasessionStoreAgentIDfor correct scope handling (global vs per-agent)- Uses
touchStoredSessionto persist activity with the portal's MXID as the session keyThis aligns well with the new sidecar/state approach per the learnings.
33-53: LGTM! Clean replacement of metadata-based lookup.The function correctly:
- Uses
lastRoutedSessionKeyto query the most recent session from the database- Applies
strings.TrimSpacebefore validation (defensive)- Validates agent assignment hasn't changed (guards against stale mappings)
One minor observation: the
TrimSpaceon line 41 may be redundant sincelastRoutedSessionKeyalready queries with trimmed keys and the DB value should be clean, but it's a safe defensive check.bridges/ai/heartbeat_delivery_test.go (3)
14-46: Helper function looks well-structured for test setup.The helper properly:
- Persists portal fields through the bridge's
GetPortalByKey/Savecycle- Builds lookup maps and injects them via
setUnexportedField- Handles nil portals gracefully
One consideration: if
GetPortalByKeyreturns an error, the test fails immediately (line 26), which is appropriate for test setup failures.
48-77: Good test coverage for session mismatch fallback.The test correctly validates that when
HeartbeatConfig.Sessioncontains a mismatched value ("other-session"),resolveHeartbeatRoutefalls back to the last-active portal. The assertions verify:
- Delivery portal matches the last active portal
- Delivery room ID matches
- Reason is "last-active"
106-124: Good test for no-history error case.The test properly verifies that without any recorded activity (no
recordAgentActivitycall),resolveHeartbeatRoutereturns an error androute.Delivery.Portalis nil.bridges/ai/heartbeat_execute.go (9)
30-41: LGTM! Clean struct definitions for routing.The
deliveryTargetandheartbeatRoutestructs are well-defined with clear field purposes:
deliveryTargetcaptures where to send heartbeat output with reason trackingheartbeatRoutebundles session resolution, session portal, and delivery target
118-127: Room busyness pre-check is good, but there's a TOCTOU gap.The pre-check at lines 122-127 verifies rooms aren't busy before proceeding, but there's a time-of-check-to-time-of-use (TOCTOU) gap between this check and the actual lock acquisition at lines 239-248. Another goroutine could acquire the room in between.
However, this is mitigated by the actual lock acquisition later (lines 239-248) which will catch this case and return "room-busy". The pre-check serves as an early-exit optimization to avoid unnecessary work, so the current design is acceptable.
286-296: LGTM! Simplified systemEventsOwnerKey.The function now uses
canonicalLoginBridgeIDandcanonicalLoginIDhelpers which handle nil safety internally (per context snippet 3). The logic correctly returns empty string only when login identity is unavailable.
298-346: LGTM! Well-structured route resolution.
resolveHeartbeatRoutecleanly orchestrates:
- Session resolution via
resolveHeartbeatSession- Session portal lookup via
firstHeartbeatPortalwith fallback candidates- Delivery target resolution with explicit
To/Targethandling and "last-active" fallbackThe priority order for delivery is clear:
target: "none"→ no delivery- Explicit
to→ direct delivery- Explicit
target(not "last") → target delivery- Default → session room with "last-active" fallback
348-391: Session resolution logic handles multiple key formats.The
resolveHeartbeatSessionfunction correctly:
- Normalizes agent IDs for both resolved and store contexts
- Handles global scope where all agents share one session
- Recognizes room IDs (starting with "!") as explicit session rooms
- Builds qualified session keys with "agent:" prefix
- Retrieves
UpdatedAtfrom stored sessions for non-main keysThe
usesMainKeyhelper correctly identifies various main key representations.
421-452: LGTM! Delivery resolution with fallback candidates.The function correctly:
- Builds candidate list with primary room first, then last-active fallback if
fallbackReasonis provided- Validates each candidate portal via
heartbeatPortalForAgent- Returns "channel-not-ready" if not logged in (preventing message send failures)
- Returns "no-target" when no valid candidate is found
The fallback pattern ensures delivery attempts the explicit target first, then falls back to last-active routing.
454-470: LGTM! File check now uses centralized store accessor.The function correctly:
- Returns
true(should run) when dependencies are unavailable — safe default- Uses
textFSStoreForAgentfor consistent store access- Maintains existing logic for empty heartbeat file detection
Per context snippet 2, returning
trueon store unavailability is intentional — it's safer to proceed with heartbeat than to block it.
185-199: Good refactor: inlined failure emission closure.The
emitFailureclosure captureshbCfgandstartedAtMsfor consistent failure event emission. It properly:
- Only includes indicator when
hbCfg.UseIndicatoris true- Uses
resolveIndicatorType("failed")for indicator selection- Emits all required fields for failure tracking
This replaces the previous
emitHeartbeatFailurehelper with context-aware behavior.
238-255: Room lock acquisition and release pattern is correct and safely handles panics.The lock acquisition and LIFO release on failure is properly implemented. The callback passed to
launchAgentLoopRuncorrectly releases locks via a deferred function, and the implementation safely handles panics:
- The goroutine is spawned immediately in
launchAgentLoopRun- The callback (
onExit) is deferred inside the goroutine before any potentially panic-prone work- In Go, deferred functions execute even when a goroutine panics, so locks will always be released
No lock leak risk exists—the implementation is sound.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bridges/ai/agentstore.go (1)
130-136:⚠️ Potential issue | 🟡 MinorMissing login state gets reported as “agent not found” in delete flow.
When login state is unavailable,
loadCustomAgentreturnsnil, nil, soDeleteAgentreturnsagents.ErrAgentNotFoundinstead of a login-state error. This can mask runtime state problems as false 404s.Suggested fix
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") + } if agents.IsPreset(agentID) || agents.IsBossAgent(agentID) { return agents.ErrAgentIsPreset }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/agentstore.go` around lines 130 - 136, The delete flow currently treats a nil, nil result from s.loadCustomAgent as agents.ErrAgentNotFound, masking missing login state; update the logic so that loadCustomAgent returns a distinct error (e.g., ErrLoginStateMissing) when login state is unavailable and then in DeleteAgent check for that error first and return it instead of agents.ErrAgentNotFound; specifically modify loadCustomAgent to return that sentinel error on missing login credentials/state, and change the DeleteAgent call that inspects existing and err (the s.loadCustomAgent call and the existing variable handling) to propagate the login-state error before mapping a truly nil existing to agents.ErrAgentNotFound.
🧹 Nitpick comments (6)
bridges/ai/heartbeat_delivery_test.go (2)
99-104: Remove duplicated assertion forSessionPortal.MXID.Line 99-Line 101 already checks
route.SessionPortal.MXID != lastPortal.MXID; Line 102-Line 104 repeats the same condition.Proposed cleanup
if route.SessionPortal == nil || route.SessionPortal.MXID != lastPortal.MXID { t.Fatalf("expected last active portal fallback to %q, got %#v", lastPortal.MXID, route.SessionPortal) } - if route.SessionPortal.MXID != lastPortal.MXID { - t.Fatalf("expected last active room %q, got %q", lastPortal.MXID, route.SessionPortal.MXID) - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/heartbeat_delivery_test.go` around lines 99 - 104, The test contains a duplicated assertion checking route.SessionPortal.MXID against lastPortal.MXID; remove the redundant t.Fatalf that repeats the same condition (the second block checking route.SessionPortal.MXID != lastPortal.MXID) so only the first assertion (which also guards nil SessionPortal) remains, leaving a single t.Fatalf that reports the mismatch using lastPortal.MXID and route.SessionPortal.
14-46: Add an explicit precondition forclient.UserLogin.Bridgein the helper.At Line 44 and Line 45,
client.UserLogin.Bridgeis dereferenced unconditionally. Add a fail-fast guard near the top so helper failures are clear instead of panicking.Proposed refactor
func cacheHeartbeatTestPortals(t *testing.T, client *AIClient, portals ...*bridgev2.Portal) { t.Helper() + if client == nil || client.UserLogin == nil || client.UserLogin.Bridge == nil { + t.Fatalf("cacheHeartbeatTestPortals requires non-nil client, user login, and bridge") + } byKey := make(map[networkid.PortalKey]*bridgev2.Portal, len(portals)) byMXID := make(map[id.RoomID]*bridgev2.Portal, len(portals)) for _, portal := range portals { if portal == nil { continue } - if client != nil && client.UserLogin != nil && client.UserLogin.Bridge != nil { - persisted, err := client.UserLogin.Bridge.GetPortalByKey(context.Background(), portal.PortalKey) - if err != nil { - t.Fatalf("GetPortalByKey(%v) returned error: %v", portal.PortalKey, err) - } - persisted.Receiver = portal.Receiver - persisted.OtherUserID = portal.OtherUserID - persisted.MXID = portal.MXID - persisted.Name = portal.Name - persisted.Topic = portal.Topic - persisted.Metadata = portal.Metadata - if err = persisted.Save(context.Background()); err != nil { - t.Fatalf("Save(%v) returned error: %v", portal.PortalKey, err) - } - portal = persisted - } + persisted, err := client.UserLogin.Bridge.GetPortalByKey(context.Background(), portal.PortalKey) + if err != nil { + t.Fatalf("GetPortalByKey(%v) returned error: %v", portal.PortalKey, err) + } + persisted.Receiver = portal.Receiver + persisted.OtherUserID = portal.OtherUserID + persisted.MXID = portal.MXID + persisted.Name = portal.Name + persisted.Topic = portal.Topic + persisted.Metadata = portal.Metadata + if err = persisted.Save(context.Background()); err != nil { + t.Fatalf("Save(%v) returned error: %v", portal.PortalKey, err) + } + portal = persisted byKey[portal.PortalKey] = portal if portal.MXID != "" { byMXID[portal.MXID] = portal } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/heartbeat_delivery_test.go` around lines 14 - 46, The helper cacheHeartbeatTestPortals currently dereferences client.UserLogin.Bridge without a precondition; add a fail-fast guard near the top of cacheHeartbeatTestPortals that checks client != nil, client.UserLogin != nil and client.UserLogin.Bridge != nil, and call t.Fatalf with a clear message if any are nil so test failures are explicit; ensure the subsequent uses of client.UserLogin.Bridge (GetPortalByKey, setUnexportedField("portalsByKey"/"portalsByMXID")) only run after this guard.bridges/ai/account_hints_test.go (1)
99-118: Add direct coverage for empty-bridge account ID formatting branches.
normalizeDesktopBridgeType("") == ""is covered, but the newformatDesktopAccountIDbehavior for empty bridge tokens (single vs multi-instance) is still untested.🧪 Suggested test addition
+func TestFormatDesktopAccountIDEmptyBridge(t *testing.T) { + if got := formatDesktopAccountID(false, "Main Desktop", "", "acc_123"); got != "acc_123" { + t.Fatalf("single-instance empty bridge should return raw account id, got %q", got) + } + if got := formatDesktopAccountID(true, "Main Desktop", "", "acc_123"); got != "main_desktop_acc_123" { + t.Fatalf("multi-instance empty bridge should include instance key, got %q", got) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/account_hints_test.go` around lines 99 - 118, Add unit tests that directly cover formatDesktopAccountID when the bridge token is empty: create one test that calls formatDesktopAccountID with an empty network/bridge token in the single-instance scenario and assert the exact returned account ID (matching current implementation), and another test that calls formatDesktopAccountID with an empty network/bridge token in the multi-instance scenario (e.g., using the parameter or flag that indicates multiple instances) and assert the exact returned account ID for that branch; reference normalizeDesktopBridgeType and formatDesktopAccountID to locate the logic and name the tests clearly (e.g., "empty-bridge single-instance" and "empty-bridge multi-instance") so both empty-bridge code paths are covered.bridges/ai/handleai.go (1)
164-176: Redundant nil check onintent.Line 174 checks
if intent == nilbut lines 166-168 already return early if!ok || intent == nil. The second check is unreachable.🧹 Suggested cleanup
if typing { if err := intent.EnsureJoined(ctx, portal.MXID); err != nil { return } } - if intent == nil { - return - } var timeout time.Duration🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handleai.go` around lines 164 - 176, Remove the unreachable redundant nil check for intent: since intent and ok are already validated by the earlier guard (the call to portal.GetIntentFor and the if !ok || intent == nil return), delete the secondary "if intent == nil { return }" after the EnsureJoined block; keep the EnsureJoined call as-is (intent.EnsureJoined) and only retain the initial guard that returns when intent is nil or not ok to simplify the flow.bridges/ai/handlematrix.go (1)
386-398: Redundant clone of just-assignedCanonicalTurnData.Line 395 clones
transcriptMeta.CanonicalTurnDataimmediately after it was assigned on line 391. SinceturnData.ToMap()already returns a new map, the subsequent clone is unnecessary.🧹 Suggested cleanup
if role == "user" { if _, turnData, ok := buildUserPromptTurn([]PromptBlock{{ Type: PromptBlockText, Text: newBody, }}); ok { transcriptMeta.CanonicalTurnData = turnData.ToMap() } else { transcriptMeta.CanonicalTurnData = nil } - transcriptMeta.CanonicalTurnData = cloneCanonicalTurnData(transcriptMeta.CanonicalTurnData) } else { transcriptMeta.CanonicalTurnData = nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handlematrix.go` around lines 386 - 398, transcriptMeta.CanonicalTurnData is being cloned immediately after assignment from turnData.ToMap(), which already returns a fresh map; remove the redundant cloneCanonicalTurnData call and simply assign the result of turnData.ToMap() (or nil) directly to transcriptMeta.CanonicalTurnData in the user branch where buildUserPromptTurn returns ok, keeping the existing fallback that sets CanonicalTurnData = nil in the non-user branch; update the code around buildUserPromptTurn, turnData.ToMap(), and cloneCanonicalTurnData to delete the extra clone invocation.bridges/ai/login_loaders.go (1)
18-31: Thread the caller context through the rebuild check.
loginConfigSnapshotcan lazily load config, so the currentcontext.Background()call drops the cancellation/deadline fromloadAIUserLogin. Since this function already acceptsctx, pass it through to preserve deadline semantics.Proposed refactor
-func aiClientNeedsRebuildConfig(existing *AIClient, key string, provider string, cfg *aiLoginConfig) bool { +func aiClientNeedsRebuildConfig(ctx context.Context, existing *AIClient, key string, provider string, cfg *aiLoginConfig) bool { if existing == nil { return true } existingProvider := "" existingBaseURL := "" if existing.UserLogin != nil { if meta := loginMetadata(existing.UserLogin); meta != nil { existingProvider = strings.TrimSpace(meta.Provider) } } - existingBaseURL = stringutil.NormalizeBaseURL(loginCredentialBaseURL(existing.loginConfigSnapshot(context.Background()))) + existingBaseURL = stringutil.NormalizeBaseURL(loginCredentialBaseURL(existing.loginConfigSnapshot(ctx))) targetProvider := strings.TrimSpace(provider) targetBaseURL := stringutil.NormalizeBaseURL(loginCredentialBaseURL(cfg)) return existing.apiKey != key || !strings.EqualFold(existingProvider, targetProvider) || existingBaseURL != targetBaseURL } @@ - if existing != nil && !aiClientNeedsRebuildConfig(existing, key, meta.Provider, cfg) { + if existing != nil && !aiClientNeedsRebuildConfig(ctx, existing, key, meta.Provider, cfg) {Also applies to: test calls at lines 33, 36, 39, 42, 45
🤖 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 18 - 31, aiClientNeedsRebuildConfig is dropping caller cancellation by calling loginConfigSnapshot(context.Background()); replace those Background contexts with the caller's ctx so deadlines/cancellations from loadAIUserLogin are preserved. Specifically, update aiClientNeedsRebuildConfig to use the passed-in ctx when calling loginConfigSnapshot (and any other calls in this function that use context.Background()), and update the related test invocations that invoke loginConfigSnapshot at the other locations to pass the appropriate ctx as well; reference symbols: aiClientNeedsRebuildConfig, loginConfigSnapshot, loadAIUserLogin.
🤖 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/login_loaders.go`:
- Around line 129-135: When rebuild of the client fails, do not rebind the
cached client that was already flagged for rebuild: remove the code that sets
existing.UserLogin, calls existing.ClientBase.SetUserLogin(login), and sets
login.Client = existing; instead surface the rebuild error (return err) or clear
the cached reference so a broken/old endpoint is not reactivated. Locate the
error branch where variables existing and login are used (the block that updates
existing.UserLogin and login.Client) and change it to return the error or
invalidate the cache entry rather than restoring the old client.
---
Duplicate comments:
In `@bridges/ai/agentstore.go`:
- Around line 130-136: The delete flow currently treats a nil, nil result from
s.loadCustomAgent as agents.ErrAgentNotFound, masking missing login state;
update the logic so that loadCustomAgent returns a distinct error (e.g.,
ErrLoginStateMissing) when login state is unavailable and then in DeleteAgent
check for that error first and return it instead of agents.ErrAgentNotFound;
specifically modify loadCustomAgent to return that sentinel error on missing
login credentials/state, and change the DeleteAgent call that inspects existing
and err (the s.loadCustomAgent call and the existing variable handling) to
propagate the login-state error before mapping a truly nil existing to
agents.ErrAgentNotFound.
---
Nitpick comments:
In `@bridges/ai/account_hints_test.go`:
- Around line 99-118: Add unit tests that directly cover formatDesktopAccountID
when the bridge token is empty: create one test that calls
formatDesktopAccountID with an empty network/bridge token in the single-instance
scenario and assert the exact returned account ID (matching current
implementation), and another test that calls formatDesktopAccountID with an
empty network/bridge token in the multi-instance scenario (e.g., using the
parameter or flag that indicates multiple instances) and assert the exact
returned account ID for that branch; reference normalizeDesktopBridgeType and
formatDesktopAccountID to locate the logic and name the tests clearly (e.g.,
"empty-bridge single-instance" and "empty-bridge multi-instance") so both
empty-bridge code paths are covered.
In `@bridges/ai/handleai.go`:
- Around line 164-176: Remove the unreachable redundant nil check for intent:
since intent and ok are already validated by the earlier guard (the call to
portal.GetIntentFor and the if !ok || intent == nil return), delete the
secondary "if intent == nil { return }" after the EnsureJoined block; keep the
EnsureJoined call as-is (intent.EnsureJoined) and only retain the initial guard
that returns when intent is nil or not ok to simplify the flow.
In `@bridges/ai/handlematrix.go`:
- Around line 386-398: transcriptMeta.CanonicalTurnData is being cloned
immediately after assignment from turnData.ToMap(), which already returns a
fresh map; remove the redundant cloneCanonicalTurnData call and simply assign
the result of turnData.ToMap() (or nil) directly to
transcriptMeta.CanonicalTurnData in the user branch where buildUserPromptTurn
returns ok, keeping the existing fallback that sets CanonicalTurnData = nil in
the non-user branch; update the code around buildUserPromptTurn,
turnData.ToMap(), and cloneCanonicalTurnData to delete the extra clone
invocation.
In `@bridges/ai/heartbeat_delivery_test.go`:
- Around line 99-104: The test contains a duplicated assertion checking
route.SessionPortal.MXID against lastPortal.MXID; remove the redundant t.Fatalf
that repeats the same condition (the second block checking
route.SessionPortal.MXID != lastPortal.MXID) so only the first assertion (which
also guards nil SessionPortal) remains, leaving a single t.Fatalf that reports
the mismatch using lastPortal.MXID and route.SessionPortal.
- Around line 14-46: The helper cacheHeartbeatTestPortals currently dereferences
client.UserLogin.Bridge without a precondition; add a fail-fast guard near the
top of cacheHeartbeatTestPortals that checks client != nil, client.UserLogin !=
nil and client.UserLogin.Bridge != nil, and call t.Fatalf with a clear message
if any are nil so test failures are explicit; ensure the subsequent uses of
client.UserLogin.Bridge (GetPortalByKey,
setUnexportedField("portalsByKey"/"portalsByMXID")) only run after this guard.
In `@bridges/ai/login_loaders.go`:
- Around line 18-31: aiClientNeedsRebuildConfig is dropping caller cancellation
by calling loginConfigSnapshot(context.Background()); replace those Background
contexts with the caller's ctx so deadlines/cancellations from loadAIUserLogin
are preserved. Specifically, update aiClientNeedsRebuildConfig to use the
passed-in ctx when calling loginConfigSnapshot (and any other calls in this
function that use context.Background()), and update the related test invocations
that invoke loginConfigSnapshot at the other locations to pass the appropriate
ctx as well; reference symbols: aiClientNeedsRebuildConfig, loginConfigSnapshot,
loadAIUserLogin.
🪄 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: a62b51ee-efcc-4046-99a3-3de15d9c22c1
📒 Files selected for processing (20)
README.mdbridges/ai/account_hints.gobridges/ai/account_hints_test.gobridges/ai/agentstore.gobridges/ai/chat.gobridges/ai/client.gobridges/ai/custom_agents_db.gobridges/ai/desktop_api_native_test.gobridges/ai/handleai.gobridges/ai/handlematrix.gobridges/ai/heartbeat_delivery_test.gobridges/ai/login_config_db.gobridges/ai/login_loaders.gobridges/ai/message_helpers.gobridges/ai/messages.gobridges/ai/queue_runtime.gobridges/ai/tool_approvals.gobridges/ai/tools_message_actions.gobridges/codex/directory_manager.gobridges/codex/portal_state.go
✅ Files skipped from review due to trivial changes (2)
- bridges/ai/login_config_db.go
- bridges/ai/custom_agents_db.go
🚧 Files skipped from review as they are similar to previous changes (2)
- bridges/ai/desktop_api_native_test.go
- bridges/ai/chat.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Lint
- GitHub Check: build-docker
- GitHub Check: build-agentremote-cli-docker (arm64)
- GitHub Check: build-agentremote-cli-docker (amd64)
- GitHub Check: Lint
- GitHub Check: build-docker
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/codex/portal_state_db.go:127-140
Timestamp: 2026-04-12T18:51:11.979Z
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:03.718Z
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:56.093Z
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:19.585Z
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: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/account_hints_test.gobridges/ai/messages.gobridges/ai/account_hints.gobridges/ai/heartbeat_delivery_test.gobridges/ai/message_helpers.gobridges/ai/login_loaders.gobridges/ai/handleai.gobridges/ai/agentstore.gobridges/ai/client.gobridges/ai/handlematrix.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/account_hints_test.gobridges/ai/messages.gobridges/ai/account_hints.gobridges/ai/heartbeat_delivery_test.gobridges/ai/message_helpers.gobridges/ai/login_loaders.gobridges/ai/handleai.gobridges/ai/agentstore.gobridges/ai/client.gobridges/ai/handlematrix.go
📚 Learning: 2026-04-12T18:51:11.979Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/codex/portal_state_db.go:127-140
Timestamp: 2026-04-12T18:51:11.979Z
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/account_hints.gobridges/ai/heartbeat_delivery_test.gobridges/ai/message_helpers.goREADME.mdbridges/ai/handleai.gobridges/ai/agentstore.gobridges/ai/client.gobridges/ai/handlematrix.go
📚 Learning: 2026-04-12T17:55:56.093Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/codex/portal_state_db.go:78-92
Timestamp: 2026-04-12T17:55:56.093Z
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/heartbeat_delivery_test.gobridges/ai/message_helpers.gobridges/ai/login_loaders.gobridges/ai/handleai.gobridges/ai/agentstore.gobridges/ai/client.gobridges/ai/handlematrix.go
📚 Learning: 2026-04-12T17:47:03.718Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/ai/metadata.go:95-100
Timestamp: 2026-04-12T17:47:03.718Z
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:
README.md
📚 Learning: 2026-04-12T17:47:19.585Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/ai/login_state_db.go:0-0
Timestamp: 2026-04-12T17:47:19.585Z
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:
README.md
📚 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/login_loaders.gobridges/ai/handleai.gobridges/ai/client.go
🔇 Additional comments (24)
bridges/ai/heartbeat_delivery_test.go (2)
48-77: Good fallback coverage for mismatched session input.This test cleanly validates the expected
last-activedelivery fallback (Portal,RoomID, andReason) for a mismatchedHeartbeatConfig.Session.
107-125: No-history case is asserted correctly.This test correctly checks the
no sessionpath and verifiesroute.Delivery.Portalremains nil when there is no prior activity.bridges/ai/account_hints_test.go (1)
108-108: Good expectation update for empty-network normalization.This test change matches the new normalization contract and prevents regressions to synthetic
"unknown"values.bridges/ai/account_hints.go (2)
15-29: Network derivation + downstream usage looks consistent.Parsing a bridge token from
AccountIDand reusing it for both hint IDs and display keeps behavior coherent across collection/rendering paths.Also applies to: 92-92, 170-170
254-254: Empty-bridge fallback formatting is a solid fix.Returning
""from normalization and omitting the bridge segment in account IDs avoids synthetic network labels while still producing stable identifiers.Also applies to: 257-273
bridges/ai/agentstore.go (4)
271-271: Good call normalizingMemorySearchin both conversion directions.This prevents config drift between persisted and runtime agent definitions.
Also applies to: 306-306
374-374:sdk.NewEventIDmigration looks correct.Nice cleanup toward the new SDK namespace.
502-505:CreateRoomnow passesTitlecorrectly tocreateChat.This addresses the earlier field mismatch and preserves requested room names.
548-550: Great improvement: room update persistence failures now propagate.Switching to
b.client.savePortal(...)with error return avoids silent success on failed persistence.bridges/ai/messages.go (1)
90-103: Fallback to history scan occurs even whenCurrentTurnDatais populated with non-text parts.The condition on line 91 checks if
CurrentTurnDatahas any content (Role, ID, or Parts), but if it's an image-only turn with Parts but no text,turnDataVisibleTextreturns""and the code correctly stops there. However, looking at the condition more carefully:if prompt.CurrentTurnData.Role != "" || prompt.CurrentTurnData.ID != "" || len(prompt.CurrentTurnData.Parts) > 0 { return turnDataVisibleText(prompt.CurrentTurnData) }This is actually correct - if CurrentTurnData is populated (even with non-text parts), it returns the result of
turnDataVisibleTextwhich may be empty. The fallback toprompt.Messagesonly happens whenCurrentTurnDatais completely empty (no Role, no ID, no Parts).This appears to be intentional: for a current turn with only images, return
""rather than the previous user's text.README.md (1)
1-77: Documentation updates look good.The terminology changes (agents → bridges, CLI → AgentRemote Manager) are consistent throughout. The SDK section accurately reflects the package path changes (
sdk.*constructors). The Codex table row grammar has been fixed with a semicolon.bridges/ai/handleai.go (2)
191-239: Disclaimer flow looks correct.The
sendDisclaimerNoticefunction properly:
- Canonicalizes the portal via
resolvePortalForAIDB- Guards against internal rooms and already-sent disclaimers
- Persists
DisclaimerSentafter sending the notice- Returns errors appropriately for caller handling
267-282: History fetch now correctly deferred until hint is empty.The
userMessageHintis checked first, andgetAIHistoryMessagesis only called inside theif userMessage == ""branch. This addresses the previous concern about unnecessary history lookups.bridges/ai/client.go (3)
644-659: Auth validation during Connect with graceful degradation.The connect flow validates credentials via
provider.ListModelswith a timeout, properly transitions toBadCredentialson auth errors, and gracefully continues with a warning for non-auth failures. This is appropriate for handling transient API issues while still catching invalid credentials early.
1866-1877: Past review concerns addressed:last.Metaguard and full batch ack cleanup.The error path now:
- Guards
last.Meta != nilbefore accessingAckReactionRemoveAfter(line 1870)- Iterates all
entriesto remove ack reactions, not justentries[0](lines 1871-1875)This properly handles build failures for debounced message batches.
1879-1905: Accepted messages now populated with proper TurnData for debounced entries.Each debounced entry creates a
database.Messagewith:
- Correct
sdk.MatrixMessageIDandsdk.MatrixEventTimestampMessageMetadatawithCanonicalTurnDatacontaining propersdk.TurnDatastructureThis ensures turn persistence works correctly for combined debounced messages.
bridges/ai/handlematrix.go (4)
399-410: Edit flow now properly handles persistence failures and metadata preservation.The refactored edit handling:
- Returns error if
persistAIConversationMessagefails (line 401)- Uses
cloneMessageMetadata(transcriptMeta)instead of empty metadata (line 404)This addresses previous review concerns about swallowed errors and cleared metadata.
979-991: Portal save now follows proper order: canonicalize → persist.The
savePortalfunction correctly:
- Resolves the portal via
resolvePortalForAIDBfirst- Then calls
portal.Save(ctx)to persistThis aligns with the new sidecar/state approach where AI-local state is managed separately via the turn store, not through
saveAIPortalStatein this function.
329-332:HandleMatrixTypingnow no-ops.The function returns
nilwithout any state updates. Verify this is intentional - the AI summary mentions "ignores local typing updates (no DB writes)" which appears correct for this bridge where typing indicators are handled through the agent loop rather than user-initiated typing.
32-37: Portal canonicalization ensures consistent AI DB keying.The early canonicalization via
resolvePortalForAIDBand reassignment tomsg.Portalensures all downstream operations use the correct portal identity for AI-local state operations.bridges/ai/message_helpers.go (4)
13-27: Nice compact metadata summary helper.
transcriptMetaSummaryis deterministic, nil-safe, and keeps logs concise.
71-80: History message cloning is clean and side-effect-safe for AI metadata.Good use of struct copy plus metadata-specific cloning.
82-135: MXID upsert flow looks correct now.Great fix on updating any found
MXIDrow instead of re-inserting; this avoids duplicate transport rows.
44-69: The fallback clone path already deep-copies these fields throughCopyFrom.
CopyFromcallssdk.CopyFromBaseAndAssistant, which in turn callsbase.CopyFromBase. TheCopyFromBaseimplementation already properly handles:
CanonicalTurnData: deep-cloned viajsonutil.DeepCloneMap(line 162 of sdk/message_metadata.go)ToolCalls: deep-cloned with new slice and element-by-element construction including deep cloning of nested maps (lines 173-190)GeneratedFiles: shallow-copied viacopy(), but sinceGeneratedFileRefcontains only string fields, this is safeThe fallback blocks do not have a clone isolation issue.
> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
bridges/ai/desktop_api_native_test.go (2)
245-250: Prefer explicit found-flag assertion over early return.Using a
foundflag keeps control flow clearer and makes future assertions in this test easier to extend.🧪 Optional readability tweak
candidates := desktopChatLabelCandidates(chat, account) + found := false for _, candidate := range candidates { if candidate == "whatsapp:Family" { - return + found = true + break } } - t.Fatalf("expected derived network alias, got %v", candidates) + if !found { + t.Fatalf("expected derived network alias, got %v", candidates) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/desktop_api_native_test.go` around lines 245 - 250, Replace the early return inside the candidates loop with an explicit found flag: declare a boolean (e.g., found) before the for _, candidate := range candidates loop, set found = true and break when candidate == "whatsapp:Family", and after the loop assert that found is true using the existing t.Fatalf message (or a new assertion) instead of returning early; update the loop in the test that iterates candidates and the final t.Fatalf assertion accordingly.
13-19: Consider extracting repeated desktop fixtures into a helper.The same chat/account fixture literals appear in multiple tests; pulling them into a small helper/constant would reduce drift risk.
♻️ Optional refactor sketch
+func testDesktopAccounts() map[string]beeperdesktopapi.Account { + return map[string]beeperdesktopapi.Account{ + "local-whatsapp_ba_wa": {AccountID: "local-whatsapp_ba_wa"}, + "local-instagram_ba_ig": {AccountID: "local-instagram_ba_ig"}, + } +} + +func testDesktopFamilyChats() []beeperdesktopapi.Chat { + return []beeperdesktopapi.Chat{ + {ID: "c1", Title: "Family", AccountID: "local-whatsapp_ba_wa"}, + {ID: "c2", Title: "Family", AccountID: "local-instagram_ba_ig"}, + } +}Also applies to: 39-45, 60-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/desktop_api_native_test.go` around lines 13 - 19, Extract the repeated desktop test fixtures into a shared helper so tests reuse the same data; create a helper like newDesktopFixtures() or constants testDesktopAccounts and testDesktopChats that return the accounts map (including "local-whatsapp_ba_wa" and "local-instagram_ba_ig") and the chat slice ({ID: "c1", Title: "Family", AccountID: "local-whatsapp_ba_wa"}, etc.), then update the tests to call the helper instead of inlining the literals (references: accounts map, the chat entries with IDs "c1"/"c2"); ensure the helper is placed in the test package so all affected tests (the ones showing the repeated fixtures) can import it.bridges/ai/desktop_api_sessions.go (1)
207-214: Minor readability note on slice reordering.The slice manipulation to move
defaultto the front works correctly. For future maintainability, consider usingslices.Delete+ prepend:names = slices.Delete(names, i, i+1) names = slices.Insert(names, 0, desktopDefaultInstance)Not blocking; current implementation is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/desktop_api_sessions.go` around lines 207 - 214, Replace the append-based reordering inside the loop that moves desktopDefaultInstance to the front: instead of building a new slice with append(names[:i], names[i+1:]...) and prepending, use slices.Delete to remove the element at index i and then slices.Insert to insert desktopDefaultInstance at index 0; target the code block that iterates over names and checks for desktopDefaultInstance (the loop using variables names and i) and replace its body accordingly to improve readability and maintainability.bridges/ai/handlematrix.go (1)
386-398: Consider removing redundant clone on Line 395.
buildUserPromptTurnalready returns fresh turn data at Line 391. The subsequentcloneCanonicalTurnDatacall is defensive but unnecessary since the data was just constructed.♻️ Optional cleanup
if role == "user" { if _, turnData, ok := buildUserPromptTurn([]PromptBlock{{ Type: PromptBlockText, Text: newBody, }}); ok { transcriptMeta.CanonicalTurnData = turnData.ToMap() } else { transcriptMeta.CanonicalTurnData = nil } - transcriptMeta.CanonicalTurnData = cloneCanonicalTurnData(transcriptMeta.CanonicalTurnData) } else { transcriptMeta.CanonicalTurnData = nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handlematrix.go` around lines 386 - 398, The code redundantly clones freshly constructed turn data: when role == "user" the call to buildUserPromptTurn returns new turn data and you immediately set transcriptMeta.CanonicalTurnData = turnData.ToMap(), then reassign transcriptMeta.CanonicalTurnData = cloneCanonicalTurnData(...). Remove the unnecessary clone by deleting the final cloneCanonicalTurnData assignment so that transcriptMeta.CanonicalTurnData is set directly from turnData.ToMap() (still set to nil in the non-ok or non-user branches); references: buildUserPromptTurn, transcriptMeta.CanonicalTurnData, cloneCanonicalTurnData.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bridges/ai/desktop_api_native_test.go`:
- Around line 245-250: Replace the early return inside the candidates loop with
an explicit found flag: declare a boolean (e.g., found) before the for _,
candidate := range candidates loop, set found = true and break when candidate ==
"whatsapp:Family", and after the loop assert that found is true using the
existing t.Fatalf message (or a new assertion) instead of returning early;
update the loop in the test that iterates candidates and the final t.Fatalf
assertion accordingly.
- Around line 13-19: Extract the repeated desktop test fixtures into a shared
helper so tests reuse the same data; create a helper like newDesktopFixtures()
or constants testDesktopAccounts and testDesktopChats that return the accounts
map (including "local-whatsapp_ba_wa" and "local-instagram_ba_ig") and the chat
slice ({ID: "c1", Title: "Family", AccountID: "local-whatsapp_ba_wa"}, etc.),
then update the tests to call the helper instead of inlining the literals
(references: accounts map, the chat entries with IDs "c1"/"c2"); ensure the
helper is placed in the test package so all affected tests (the ones showing the
repeated fixtures) can import it.
In `@bridges/ai/desktop_api_sessions.go`:
- Around line 207-214: Replace the append-based reordering inside the loop that
moves desktopDefaultInstance to the front: instead of building a new slice with
append(names[:i], names[i+1:]...) and prepending, use slices.Delete to remove
the element at index i and then slices.Insert to insert desktopDefaultInstance
at index 0; target the code block that iterates over names and checks for
desktopDefaultInstance (the loop using variables names and i) and replace its
body accordingly to improve readability and maintainability.
In `@bridges/ai/handlematrix.go`:
- Around line 386-398: The code redundantly clones freshly constructed turn
data: when role == "user" the call to buildUserPromptTurn returns new turn data
and you immediately set transcriptMeta.CanonicalTurnData = turnData.ToMap(),
then reassign transcriptMeta.CanonicalTurnData = cloneCanonicalTurnData(...).
Remove the unnecessary clone by deleting the final cloneCanonicalTurnData
assignment so that transcriptMeta.CanonicalTurnData is set directly from
turnData.ToMap() (still set to nil in the non-ok or non-user branches);
references: buildUserPromptTurn, transcriptMeta.CanonicalTurnData,
cloneCanonicalTurnData.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb80c965-7248-4da1-8c45-1f87c48ccd5b
📒 Files selected for processing (14)
bridges/ai/client.gobridges/ai/desktop_api_native_test.gobridges/ai/desktop_api_sessions.gobridges/ai/handlematrix.gobridges/ai/metadata.gobridges/ai/reaction_feedback.gobridges/ai/session_transcript.gobridges/ai/session_transcript_test.gobridges/ai/sessions_tools.gobridges/ai/tool_policy_chain.gobridges/ai/tool_policy_chain_test.gobridges/ai/tool_schema_sanitize.gobridges/ai/tools.gobridges/ai/typing_controller.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Lint
- GitHub Check: build-docker
- GitHub Check: build-agentremote-cli-docker (amd64)
- GitHub Check: build-agentremote-cli-docker (arm64)
- GitHub Check: Lint
- GitHub Check: build-docker
🧰 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:11.979Z
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:03.718Z
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:56.093Z
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:19.585Z
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/metadata.gobridges/ai/desktop_api_native_test.gobridges/ai/desktop_api_sessions.gobridges/ai/client.gobridges/ai/handlematrix.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/metadata.gobridges/ai/desktop_api_native_test.gobridges/ai/desktop_api_sessions.gobridges/ai/client.gobridges/ai/handlematrix.go
📚 Learning: 2026-04-12T18:51:11.979Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/codex/portal_state_db.go:127-140
Timestamp: 2026-04-12T18:51:11.979Z
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/metadata.gobridges/ai/desktop_api_native_test.gobridges/ai/client.gobridges/ai/handlematrix.go
📚 Learning: 2026-04-12T17:55:56.093Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/codex/portal_state_db.go:78-92
Timestamp: 2026-04-12T17:55:56.093Z
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/metadata.gobridges/ai/desktop_api_sessions.gobridges/ai/client.gobridges/ai/handlematrix.go
📚 Learning: 2026-04-12T17:46:48.384Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/openclaw/metadata.go:310-337
Timestamp: 2026-04-12T17:46:48.384Z
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/metadata.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/metadata.gobridges/ai/client.go
🔇 Additional comments (22)
bridges/ai/desktop_api_native_test.go (4)
13-19: Good fixture update for alias/account-qualified matching.These fixture changes make the alias and account-qualified label paths consistent and keep the test intent clear.
Also applies to: 31-34
39-47: Canonical account filtering coverage looks solid.Nice job validating both single-instance and multi-instance canonical account ID inputs in the resolver filter path.
Also applies to: 60-75
81-81: Builder rename/use is consistent and correctly wired.The test rename and function call update are aligned and keep naming coherent with the current API surface.
Also applies to: 108-108
255-265: Session account ID expectations are well asserted.The updated expected values for single- and multi-instance forms are precise and validate canonical formatting behavior clearly.
bridges/ai/desktop_api_sessions.go (5)
152-216: LGTM on context threading.The refactoring properly threads
context.ContextthroughdesktopAPIInstances,desktopAPIInstanceConfig,desktopAPIClient, anddesktopAPIInstanceNames. This ensures request deadlines and cancellation signals propagate correctly through tologinConfigSnapshot(ctx)→ I/O operations, addressing the previous review concern.
279-282: Consistent network extraction via helper.Using
desktopAccountNetwork(account)throughout (lines 279, 299, 483, 958-959, 966, 981-982, 1008, 1045) provides a single source of truth for parsing network fromAccountID. This is more reliable than direct field access.
843-875: LGTM on return value cleanup.Removing the unused
_assignments inarchiveDesktopChat,setDesktopChatReminder, andclearDesktopChatReminderis cleaner. The current pattern is acceptable.
407-434: LGTM on message builder rename.
buildAgentRemoteDesktopSessionMessagesproduces the AgentRemote-style format with structuredcontentarrays, whilebuildDesktopSessionMessages(lines 436-492) maintains the flat content format with additional metadata. Both serve different consumers.
75-75: Consistent instance key sanitization.The switch to
sanitizeDesktopInstanceKeyacross all instance key handling points (lines 75, 112, 144, 162, 183) ensures uniform normalization. The function is defined atbridges/ai/account_hints.go:221and properly available throughout the package.bridges/ai/metadata.go (6)
13-14: LGTM!Clean import additions supporting the sdk migration and memory state integration.
106-142: LGTM!The credential accessor refactoring to use
*aiLoginConfigis clean and maintains consistent nil-safety throughout.
214-247: LGTM!The
PortalMetadatarestructuring is well-designed. Using a pointer forTypingIntervalSecondscorrectly distinguishes unset from zero, andInternalRoomKindprovides a cleaner approach than module-meta maps for internal room identification.
260-279: LGTM!The accessor methods follow a consistent pattern with proper nil guards.
EnsureMemoryState()correctly handles nil receiver before attempting state initialization.
347-349: LGTM!Type aliases cleanly redirect to sdk types, maintaining API compatibility during the migration.
362-362: No issues found. Thesdk.CopyFromBaseAndAssistantfunction exists with the correct signature and the call in line 362 matches it exactly.bridges/ai/client.go (3)
638-659: LGTM! Credential validation on connect is a solid improvement.The early validation via
provider.ListModelswith a timeout ensures authentication errors are surfaced immediately asBadCredentialsstate, rather than failing silently on first message.
1866-1923: LGTM! Debounced message handling properly addressed.The error path now correctly:
- Guards
last.Metabefore accessingAckReactionRemoveAfter- Iterates over all entries to remove ack reactions on prompt build failure
- Builds
acceptedMessageswith proper null checks before dispatch
1927-1967: LGTM! Ack reaction removal migrated to simplevent pattern.The switch from direct redaction to
simplevent.MessageRemoveviaQueueRemoteEventaligns with the bridgev2 event pipeline and the SDK migration.bridges/ai/handlematrix.go (4)
32-37: LGTM! Portal canonicalization ensures consistent AI DB operations.Both message and edit handlers now resolve the portal via
resolvePortalForAIDBearly in the flow, ensuring consistent portal identity for all downstream persistence operations.Also applies to: 340-345
399-410: LGTM! Edit persistence now fails fast on AI-history errors.The error is returned immediately after logging, preventing downstream updates from proceeding with stale or missing AI history. The bridge message metadata is also correctly cloned from the persisted transcript metadata.
979-992: LGTM! Save order prevents partial commits.The refactored
savePortalnow:
- Resolves portal identity first
- Saves the portal row via
portal.Save(ctx)- Returns early on any error
This ensures AI portal state is never persisted when the underlying portal save fails.
1093-1113: LGTM! Ack reaction storage properly keyed by portal.The
storeAckReactionfunction now correctly guards against nil portals and usesportal.MXIDas the storage key, aligning with the lookup pattern inremoveAckReaction.
Move memory state into pkg/integrations/memory (add State type and tests), update PortalMetadata and memory integration code to use the new integrationmemory.State and Meta type, and remove the old runtime MemoryState implementation. Simplify OpenAIConnector client caching and lifecycle: remove complex lookup/publish logic and replace with a straightforward map swap that disconnects the previous AIClient when replaced. Misc: tighten group chat activation handling, stop returning Pending on MatrixMessageResponse in several handlers, set DeleteOnConflict when persisting login completions, remove several unused/legacy bridge AI helpers and toast files, and remove various schema backfill/migration and column helper code. Also remove CommandDefinition Aliases field in module hooks and adjust imports accordingly.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
bridges/ai/metadata.go (1)
357-363:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
CopyFromdoesn't copy MessageMetadata-specific fields, potential data loss during merge.The method delegates to
sdk.CopyFromBaseAndAssistantfor embedded fields but doesn't copyMediaUnderstanding,MediaUnderstandingDecisions,MediaURL, orMimeType. Whendatabase.MetaMergerinvokes this during metadata updates, these fields fromsrcwill be silently dropped.🐛 Proposed fix to copy local fields
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 = src.MediaUnderstanding + mm.MediaUnderstandingDecisions = 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 357 - 363, The CopyFrom method currently only delegates embedded fields to sdk.CopyFromBaseAndAssistant and fails to copy MessageMetadata-specific fields; update MessageMetadata.CopyFrom so that after calling sdk.CopyFromBaseAndAssistant it also copies MediaUnderstanding, MediaUnderstandingDecisions, MediaURL, and MimeType from src to mm (perform a deep copy for slices/maps if those fields are composite types) so these local fields are preserved when merging.bridges/ai/login.go (2)
221-226:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't leak connector-level API keys into
RemoteName.
resolveCustomLogincan populateapiKeyfrom the bridge config, soformatRemoteName(provider, apiKey)ends up exposing the prefix/suffix of a shared server-side credential in a user-visible login name. If the selected token came from connector config rather than per-login credentials, fall back to a plain provider label instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/login.go` around lines 221 - 226, The current code builds remoteNameBase using formatRemoteName(provider, apiKey) which can leak connector-level API keys; change resolveCustomLogin (or its caller) to also indicate whether the selected token is a connector-config token (e.g., return tokenFromConnectorConfig bool or similar), then only call formatRemoteName(provider, apiKey) when tokenFromConnectorConfig is false; if tokenFromConnectorConfig is true, set remoteNameBase to a plain provider label (e.g., provider or provider.DisplayName) instead. Keep the existing override.RemoteName and ordinal logic intact (use override.RemoteName if present, else apply ordinal suffix to the chosen remoteNameBase).
248-258:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelogin currently drops unprompted credential fields.
On the relogin path,
cfgis loaded fromloadAILoginConfig(ctx, override), but these lines replacecfg.Credentialswith a fresh struct containing onlyAPIKey,BaseURL, and the small subset of service tokens collected by this step. Existing per-login credential fields that are not part of the login form get silently wiped on relogin. Merge into the existing credentials instead of replacing the whole object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/login.go` around lines 248 - 258, The current logic creates a new LoginCredentials and unconditionally replaces cfg.Credentials, which discards any existing unstored fields on relogin; instead, when loadAILoginConfig(ctx, override) returns cfg with non-nil cfg.Credentials, merge into that object rather than overwriting it: take the existing cfg.Credentials (type LoginCredentials) and update only the APIKey, BaseURL and service-tokens fields (use cloneServiceTokens to merge/append tokens rather than replace if needed), leaving all other fields intact; if cfg.Credentials is nil create a new LoginCredentials, populate it, and then assign it back to cfg.Credentials; also preserve behavior of loginCredentialsEmpty to set cfg.Credentials = nil when appropriate.
♻️ Duplicate comments (1)
bridges/ai/chat.go (1)
965-993:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate agent-portal save failures here too.
configureAgentChatPortalstill callssavePortalQuiet, soResolvedTarget, avatar, and runtime override can remain in memory only while chat creation keeps going. A reload can reopen the room as the wrong target. This needs to return an error tocreateChatinstead of logging and continuing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/chat.go` around lines 965 - 993, configureAgentChatPortal currently swallows portal save failures by calling savePortalQuiet; change configureAgentChatPortal to return an error (e.g., change signature to return (string, error)), call the non-quiet save that returns an error (or modify savePortalQuiet to return an error) after setPortalResolvedTarget and avatar/runtime assignments, and return that error up to createChat so createChat can abort on save failures; update callers (notably createChat) to check and propagate the error, and keep returning the resolved agentGhostID on success.
🧹 Nitpick comments (1)
bridges/ai/message_helpers.go (1)
27-52: Add regression test for clone isolation to prevent future field loss.
cloneMessageMetadatalacks test coverage despite being used in critical persistence and transcript flows. All MessageMetadata fields have proper JSON tags, so the primary JSON round-trip path should serialize/deserialize correctly. However, add a test that verifies:
- All fields (including nested slices and maps like
CanonicalTurnDataandMediaUnderstanding) are fully cloned with no shared references- Mutations to source fields after cloning do not affect the clone
- Both the JSON path and fallback path produce equivalent results
This ensures the function maintains invariants as the struct evolves.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/message_helpers.go` around lines 27 - 52, Write a regression test for cloneMessageMetadata that ensures deep-clone isolation for MessageMetadata: construct a fully-populated MessageMetadata (including nested slices/maps like CanonicalTurnData, MediaUnderstanding, MediaUnderstandingDecisions, and any other fields), call cloneMessageMetadata on it, then mutate every mutable field on the source (append/modify slice entries, change map values, alter strings/urls, mime types, nested structs) and assert the clone's corresponding fields remain unchanged and have equal values; also force the fallback path (e.g., by making json.Marshal fail or by simulating non-serializable data) and assert that the fallback clone is equivalent in value and isolation to the JSON round-trip clone, referencing cloneMessageMetadata, MessageMetadata, CanonicalTurnData, MediaUnderstanding, MediaUnderstandingDecision to locate fields to test.
🤖 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/chat.go`:
- Around line 273-276: collectContactResponses currently always calls
oc.sdkAgentCatalog().ListAgents which causes SearchUsers/GetContactList to
hard-fail when agents are disabled or the agent catalog is temporarily
unavailable; change collectContactResponses to first check whether agents are
enabled for the current login (use the existing feature/check used elsewhere for
oc.UserLogin) and skip calling oc.sdkAgentCatalog().ListAgents when agents are
disabled, and if ListAgents returns an error when agents are enabled treat that
error as partial/fallback (log the error and continue without agents) so model
contact discovery still proceeds rather than returning an error to callers like
SearchUsers/GetContactList.
In `@bridges/ai/connector.go`:
- Around line 42-48: In applyRuntimeDefaults, normalize the Bridge.CommandPrefix
by trimming whitespace before checking emptiness so a value like " " is
treated as empty and the default "!ai" is applied; specifically, call
strings.TrimSpace on oc.Config.Bridge.CommandPrefix inside
OpenAIConnector.applyRuntimeDefaults and then test == "" to assign the default,
noting that oc.Config.Bridge is a value (no nil check required) and this keeps
command parsing consistent with BridgeName’s trimming logic.
In `@bridges/ai/desktop_api_sessions.go`:
- Around line 958-966: In resolveDesktopSessionByLabelWithOptions(), when
Accounts.List lookup returns no account for chat.AccountID you must seed
account.AccountID from chat.AccountID before calling
desktopAccountNetwork(account) so the network is preserved for matching (same
approach used in listDesktopSessions()); update the code paths that call
desktopAccountNetwork(account) (including the blocks that build single/multi
account IDs and the network filter checks) to set account.AccountID =
chat.AccountID when account is empty/lookup failed, then proceed to derive the
network and perform accountId/network-aware comparisons.
In `@bridges/ai/persistence_boundaries_test.go`:
- Around line 619-692: The test
TestLoadAIPromptHistoryTurns_UsesCanonicalPortalScopeForTransientPortal
currently only checks portalScopeForPortal(transientPortal) and never calls the
loader it claims to cover; update the test to exercise the prompt-history loader
(or remove the test). Specifically, after building transientPortal and asserting
portalScopeForPortal is nil, call the loader under test (e.g.,
LoadAIPromptHistoryTurns or loadAIPromptHistoryTurns used elsewhere in tests)
with ctx and transientPortal, capture the returned turns, and assert that the
assistant turn with CanonicalTurnData ID "client-scope-turn-1" is present and
that the user turn was canonicalized per setCanonicalTurnDataFromPromptMessages;
if you prefer to drop the misleading case, delete the test instead of adding the
loader call.
---
Outside diff comments:
In `@bridges/ai/login.go`:
- Around line 221-226: The current code builds remoteNameBase using
formatRemoteName(provider, apiKey) which can leak connector-level API keys;
change resolveCustomLogin (or its caller) to also indicate whether the selected
token is a connector-config token (e.g., return tokenFromConnectorConfig bool or
similar), then only call formatRemoteName(provider, apiKey) when
tokenFromConnectorConfig is false; if tokenFromConnectorConfig is true, set
remoteNameBase to a plain provider label (e.g., provider or
provider.DisplayName) instead. Keep the existing override.RemoteName and ordinal
logic intact (use override.RemoteName if present, else apply ordinal suffix to
the chosen remoteNameBase).
- Around line 248-258: The current logic creates a new LoginCredentials and
unconditionally replaces cfg.Credentials, which discards any existing unstored
fields on relogin; instead, when loadAILoginConfig(ctx, override) returns cfg
with non-nil cfg.Credentials, merge into that object rather than overwriting it:
take the existing cfg.Credentials (type LoginCredentials) and update only the
APIKey, BaseURL and service-tokens fields (use cloneServiceTokens to
merge/append tokens rather than replace if needed), leaving all other fields
intact; if cfg.Credentials is nil create a new LoginCredentials, populate it,
and then assign it back to cfg.Credentials; also preserve behavior of
loginCredentialsEmpty to set cfg.Credentials = nil when appropriate.
In `@bridges/ai/metadata.go`:
- Around line 357-363: The CopyFrom method currently only delegates embedded
fields to sdk.CopyFromBaseAndAssistant and fails to copy
MessageMetadata-specific fields; update MessageMetadata.CopyFrom so that after
calling sdk.CopyFromBaseAndAssistant it also copies MediaUnderstanding,
MediaUnderstandingDecisions, MediaURL, and MimeType from src to mm (perform a
deep copy for slices/maps if those fields are composite types) so these local
fields are preserved when merging.
---
Duplicate comments:
In `@bridges/ai/chat.go`:
- Around line 965-993: configureAgentChatPortal currently swallows portal save
failures by calling savePortalQuiet; change configureAgentChatPortal to return
an error (e.g., change signature to return (string, error)), call the non-quiet
save that returns an error (or modify savePortalQuiet to return an error) after
setPortalResolvedTarget and avatar/runtime assignments, and return that error up
to createChat so createChat can abort on save failures; update callers (notably
createChat) to check and propagate the error, and keep returning the resolved
agentGhostID on success.
---
Nitpick comments:
In `@bridges/ai/message_helpers.go`:
- Around line 27-52: Write a regression test for cloneMessageMetadata that
ensures deep-clone isolation for MessageMetadata: construct a fully-populated
MessageMetadata (including nested slices/maps like CanonicalTurnData,
MediaUnderstanding, MediaUnderstandingDecisions, and any other fields), call
cloneMessageMetadata on it, then mutate every mutable field on the source
(append/modify slice entries, change map values, alter strings/urls, mime types,
nested structs) and assert the clone's corresponding fields remain unchanged and
have equal values; also force the fallback path (e.g., by making json.Marshal
fail or by simulating non-serializable data) and assert that the fallback clone
is equivalent in value and isolation to the JSON round-trip clone, referencing
cloneMessageMetadata, MessageMetadata, CanonicalTurnData, MediaUnderstanding,
MediaUnderstandingDecision to locate fields to test.
🪄 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: 08ab9a24-36ca-4f86-9246-68b21c966a9e
📒 Files selected for processing (33)
bridges/ai/bridge_db.gobridges/ai/chat.gobridges/ai/client.gobridges/ai/command_aliases.gobridges/ai/connector.gobridges/ai/constructors.gobridges/ai/debounce.gobridges/ai/desktop_api_helpers.gobridges/ai/desktop_api_native_test.gobridges/ai/desktop_api_sessions.gobridges/ai/group_activation.gobridges/ai/handlematrix.gobridges/ai/login.gobridges/ai/login_loaders.gobridges/ai/login_loaders_test.gobridges/ai/message_helpers.gobridges/ai/metadata.gobridges/ai/metadata_test.gobridges/ai/persistence_boundaries_test.gobridges/ai/queue_status_test.gobridges/ai/reaction_feedback.gobridges/ai/reaction_handling.gobridges/ai/session_transcript.gobridges/ai/session_transcript_test.gobridges/ai/sessions_tools.gobridges/ai/toast.gobridges/ai/tool_policy_chain.gobridges/ai/tool_policy_chain_test.gobridges/ai/tool_schema_sanitize.gobridges/ai/tools.gobridges/ai/typing_controller.gobridges/codex/client.gobridges/codex/constructors.go
💤 Files with no reviewable changes (2)
- bridges/ai/desktop_api_helpers.go
- bridges/ai/command_aliases.go
✅ Files skipped from review due to trivial changes (2)
- bridges/ai/debounce.go
- bridges/ai/metadata_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- bridges/ai/login_loaders.go
- bridges/ai/desktop_api_native_test.go
- bridges/ai/client.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-agentremote-cli-docker (amd64)
- GitHub Check: Lint
- GitHub Check: build-docker
- 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:11.979Z
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:03.718Z
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:56.093Z
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:19.585Z
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/message_helpers.gobridges/ai/persistence_boundaries_test.gobridges/ai/group_activation.gobridges/ai/connector.gobridges/ai/bridge_db.gobridges/ai/constructors.gobridges/ai/login_loaders_test.gobridges/ai/login.gobridges/ai/metadata.gobridges/ai/desktop_api_sessions.gobridges/ai/chat.gobridges/ai/handlematrix.go
📚 Learning: 2026-04-12T18:51:11.979Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/codex/portal_state_db.go:127-140
Timestamp: 2026-04-12T18:51:11.979Z
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/message_helpers.gobridges/ai/persistence_boundaries_test.gobridges/ai/connector.gobridges/ai/bridge_db.gobridges/ai/constructors.gobridges/ai/login_loaders_test.gobridges/ai/login.gobridges/ai/metadata.gobridges/ai/chat.gobridges/ai/handlematrix.go
📚 Learning: 2026-04-12T17:55:56.093Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/codex/portal_state_db.go:78-92
Timestamp: 2026-04-12T17:55:56.093Z
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/message_helpers.gobridges/ai/persistence_boundaries_test.gobridges/ai/connector.gobridges/ai/bridge_db.gobridges/ai/constructors.gobridges/ai/login_loaders_test.gobridges/ai/login.gobridges/ai/metadata.gobridges/ai/desktop_api_sessions.gobridges/ai/chat.gobridges/ai/handlematrix.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/message_helpers.gobridges/ai/persistence_boundaries_test.gobridges/ai/group_activation.gobridges/ai/connector.gobridges/ai/bridge_db.gobridges/ai/constructors.gobridges/ai/login_loaders_test.gobridges/ai/login.gobridges/ai/metadata.gobridges/ai/desktop_api_sessions.gobridges/ai/chat.gobridges/ai/handlematrix.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/group_activation.gobridges/ai/connector.gobridges/ai/constructors.gobridges/ai/login_loaders_test.gobridges/ai/metadata.go
📚 Learning: 2026-04-12T17:46:48.384Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/openclaw/metadata.go:310-337
Timestamp: 2026-04-12T17:46:48.384Z
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/metadata.go
🔇 Additional comments (10)
bridges/ai/group_activation.go (1)
5-7: Direct activation matching with safe fallback looks good.This keeps accepted values explicit and cleanly defaults to
"mention"for anything else.Based on learnings: For unreleased AI bridge code under
bridges/ai, avoid migration paths, legacy-compatibility fallbacks, or dual-read/backfill logic.bridges/ai/metadata.go (7)
13-14: LGTM!New imports align with the SDK migration and memory module state integration.
95-104: LGTM!Simplified
UserLoginMetadataaligns with the sidecar/state approach—removes legacy persisted fields without migration fallbacks per the unreleased bridge guidance.
106-142: LGTM!Credential helpers correctly refactored to use
*aiLoginConfigwith appropriate nil guards.
214-247: LGTM!
PortalMetadataextended with memory compaction state, internal room identification, and typing configuration. Pointer forTypingIntervalSecondscorrectly distinguishes unset from zero.
260-279: LGTM!Methods follow idiomatic Go patterns with appropriate nil-receiver handling.
EnsureMemoryStatecorrectly documents its behavior by returning nil when the receiver is nil.
334-349: LGTM!Clean migration to SDK-based embedded types with local fields preserved for media understanding. Type aliases maintain compatibility.
319-322: No changes needed. Theintegrationmemory.Statestruct contains only primitive types (bool, int, int64, string), so the shallow copy via struct dereference is safe and equivalent to a deep copy.bridges/ai/message_helpers.go (2)
11-25: Looks good: metadata summary stays compact and safe.Nil handling and the summary shape are clean; this avoids body-content leakage while still being useful for debugging.
54-63: Looks good: history clone path is nil-safe and type-aware.The shallow struct copy plus targeted deep clone for
*MessageMetadatais a good boundary for AI history persistence.
| agentsList, err := oc.sdkAgentCatalog().ListAgents(ctx, oc.UserLogin) | ||
| if err != nil { | ||
| oc.loggerForContext(ctx).Error().Err(err).Msg("Failed to load agents") | ||
| return nil, fmt.Errorf("failed to load agents: %w", err) | ||
| } |
There was a problem hiding this comment.
Don't hard-fail model contact search on agent catalog errors.
collectContactResponses fetches agents before checking whether agents are enabled for this login. A disabled agent feature or a temporary agent-catalog failure now causes SearchUsers / GetContactList to fail before model contacts are even attempted. Skip ListAgents when agents are disabled, and treat agent-load failures as partial results so model discovery still works.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/chat.go` around lines 273 - 276, collectContactResponses currently
always calls oc.sdkAgentCatalog().ListAgents which causes
SearchUsers/GetContactList to hard-fail when agents are disabled or the agent
catalog is temporarily unavailable; change collectContactResponses to first
check whether agents are enabled for the current login (use the existing
feature/check used elsewhere for oc.UserLogin) and skip calling
oc.sdkAgentCatalog().ListAgents when agents are disabled, and if ListAgents
returns an error when agents are enabled treat that error as partial/fallback
(log the error and continue without agents) so model contact discovery still
proceeds rather than returning an error to callers like
SearchUsers/GetContactList.
| func (oc *OpenAIConnector) applyRuntimeDefaults() { | ||
| if oc.Config.ModelCacheDuration == 0 { | ||
| oc.Config.ModelCacheDuration = 6 * time.Hour | ||
| } | ||
| bridgesdk.ApplyDefaultCommandPrefix(&oc.Config.Bridge.CommandPrefix, "!ai") | ||
| if oc.Config.Bridge.CommandPrefix == "" { | ||
| oc.Config.Bridge.CommandPrefix = "!ai" | ||
| } |
There was a problem hiding this comment.
Normalize whitespace-only command prefixes before defaulting.
applyRuntimeDefaults only checks == "", so a value like " " survives as the runtime prefix even though BridgeName trims it when advertising defaults. Trim before the emptiness check so command parsing and bridge metadata stay aligned.
Based on learnings: 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 is safe and does not require a nil-check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/connector.go` around lines 42 - 48, In applyRuntimeDefaults,
normalize the Bridge.CommandPrefix by trimming whitespace before checking
emptiness so a value like " " is treated as empty and the default "!ai" is
applied; specifically, call strings.TrimSpace on oc.Config.Bridge.CommandPrefix
inside OpenAIConnector.applyRuntimeDefaults and then test == "" to assign the
default, noting that oc.Config.Bridge is a value (no nil check required) and
this keeps command parsing consistent with BridgeName’s trimming logic.
| single := formatDesktopAccountID(false, instance, desktopAccountNetwork(account), chatAccountID) | ||
| multi := formatDesktopAccountID(true, instance, desktopAccountNetwork(account), chatAccountID) | ||
| if accountID != single && accountID != multi { | ||
| continue | ||
| } | ||
| } | ||
| } | ||
| if network != "" { | ||
| if !desktopNetworkFilterMatches(networkFilter, account.Network) { | ||
| if !desktopNetworkFilterMatches(networkFilter, desktopAccountNetwork(account)) { |
There was a problem hiding this comment.
Preserve network-aware matching when Accounts.List misses a chat account.
These paths now derive network via desktopAccountNetwork(account), but resolveDesktopSessionByLabelWithOptions() still treats account lookup as best-effort. When accounts[chat.AccountID] is empty, desktopAccountNetwork(account) loses the network even though chat.AccountID still has it, so canonical accountId filters from sessions_list and network-qualified labels can incorrectly stop matching. Please seed account.AccountID from chat.AccountID before deriving the network here, the same way listDesktopSessions() already does.
Suggested fix
for _, chat := range chats {
chatAccountID := strings.TrimSpace(chat.AccountID)
account := accounts[chatAccountID]
+ if strings.TrimSpace(account.AccountID) == "" && chatAccountID != "" {
+ account.AccountID = chatAccountID
+ }
if accountID != "" {
// Accept raw account IDs and canonical account IDs from sessions_list/account hints.
if chatAccountID != accountID {
single := formatDesktopAccountID(false, instance, desktopAccountNetwork(account), chatAccountID)
multi := formatDesktopAccountID(true, instance, desktopAccountNetwork(account), chatAccountID)
@@
func desktopChatLabelCandidates(chat beeperdesktopapi.Chat, account beeperdesktopapi.Account) []string {
title := strings.TrimSpace(chat.Title)
if title == "" {
return nil
}
accountID := strings.TrimSpace(chat.AccountID)
+ if strings.TrimSpace(account.AccountID) == "" && accountID != "" {
+ account.AccountID = accountID
+ }
network := canonicalDesktopNetwork(desktopAccountNetwork(account))
rawNetwork := normalizeDesktopNetworkToken(desktopAccountNetwork(account))
@@
func describeDesktopChatForLabel(chat beeperdesktopapi.Chat, account beeperdesktopapi.Account) string {
title := strings.TrimSpace(chat.Title)
if title == "" {
title = strings.TrimSpace(chat.ID)
}
accountID := strings.TrimSpace(chat.AccountID)
+ if strings.TrimSpace(account.AccountID) == "" && accountID != "" {
+ account.AccountID = accountID
+ }
network := strings.TrimSpace(desktopAccountNetwork(account))Also applies to: 981-983, 1008-1009
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/desktop_api_sessions.go` around lines 958 - 966, In
resolveDesktopSessionByLabelWithOptions(), when Accounts.List lookup returns no
account for chat.AccountID you must seed account.AccountID from chat.AccountID
before calling desktopAccountNetwork(account) so the network is preserved for
matching (same approach used in listDesktopSessions()); update the code paths
that call desktopAccountNetwork(account) (including the blocks that build
single/multi account IDs and the network filter checks) to set account.AccountID
= chat.AccountID when account is empty/lookup failed, then proceed to derive the
network and perform accountId/network-aware comparisons.
| func TestLoadAIPromptHistoryTurns_UsesCanonicalPortalScopeForTransientPortal(t *testing.T) { | ||
| ctx := context.Background() | ||
| client := newDBBackedTestAIClient(t, ProviderOpenAI) | ||
| client.UserLogin.Client = client | ||
|
|
||
| portal := newTranscriptTestPortal(t, client, "client-scoped-history") | ||
|
|
||
| userMeta := &MessageMetadata{ | ||
| BaseMessageMetadata: sdk.BaseMessageMetadata{Role: "user", Body: "hello world"}, | ||
| } | ||
| setCanonicalTurnDataFromPromptMessages(userMeta, []PromptMessage{{ | ||
| Role: PromptRoleUser, | ||
| Blocks: []PromptBlock{{ | ||
| Type: PromptBlockText, | ||
| Text: "hello world", | ||
| }}, | ||
| }}) | ||
| userMsg := &database.Message{ | ||
| ID: sdk.MatrixMessageID(id.EventID("$client-scope-user-1")), | ||
| MXID: id.EventID("$client-scope-user-1"), | ||
| Room: portal.PortalKey, | ||
| SenderID: humanUserID(client.UserLogin.ID), | ||
| Metadata: userMeta, | ||
| Timestamp: time.UnixMilli(1000), | ||
| } | ||
| client.saveUserMessage(ctx, &event.Event{ID: userMsg.MXID}, userMsg) | ||
|
|
||
| assistantMsg := &database.Message{ | ||
| ID: networkid.MessageID("client-scope-assistant-1"), | ||
| MXID: id.EventID("$client-scope-assistant-1"), | ||
| Room: portal.PortalKey, | ||
| SenderID: modelUserID("openai/gpt-4.1"), | ||
| Metadata: &MessageMetadata{ | ||
| BaseMessageMetadata: sdk.BaseMessageMetadata{ | ||
| Role: "assistant", | ||
| Body: "Hi there", | ||
| CanonicalTurnData: sdk.TurnData{ | ||
| ID: "client-scope-turn-1", | ||
| Role: "assistant", | ||
| Parts: []sdk.TurnPart{{ | ||
| Type: "text", | ||
| Text: "Hi there", | ||
| }}, | ||
| }.ToMap(), | ||
| }, | ||
| }, | ||
| Timestamp: time.UnixMilli(2000), | ||
| } | ||
| if err := client.persistAIConversationMessage(ctx, portal, assistantMsg); err != nil { | ||
| t.Fatalf("persist assistant turn: %v", err) | ||
| } | ||
|
|
||
| transientBridgeDB := *client.UserLogin.Bridge.DB | ||
| transientBridgeDB.BridgeID = "" | ||
| transientBridge := &bridgev2.Bridge{ | ||
| DB: &transientBridgeDB, | ||
| Config: client.UserLogin.Bridge.Config, | ||
| Log: client.UserLogin.Bridge.Log, | ||
| Matrix: client.UserLogin.Bridge.Matrix, | ||
| } | ||
| transientPortal := &bridgev2.Portal{ | ||
| Portal: &database.Portal{ | ||
| PortalKey: portal.PortalKey, | ||
| MXID: portal.MXID, | ||
| Metadata: portal.Metadata, | ||
| }, | ||
| Bridge: transientBridge, | ||
| } | ||
|
|
||
| if scope := portalScopeForPortal(transientPortal); scope != nil { | ||
| t.Fatalf("expected transient portal scope lookup to fail, got %#v", scope) | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
This test never hits the loader it claims to cover.
After building transientPortal, the body only verifies that portalScopeForPortal is nil and then exits. It never calls the prompt-history loader or asserts the canonicalized output, so this passes even if that behavior regresses. Add the actual load/assertion path or drop the misleading test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/persistence_boundaries_test.go` around lines 619 - 692, The test
TestLoadAIPromptHistoryTurns_UsesCanonicalPortalScopeForTransientPortal
currently only checks portalScopeForPortal(transientPortal) and never calls the
loader it claims to cover; update the test to exercise the prompt-history loader
(or remove the test). Specifically, after building transientPortal and asserting
portalScopeForPortal is nil, call the loader under test (e.g.,
LoadAIPromptHistoryTurns or loadAIPromptHistoryTurns used elsewhere in tests)
with ctx and transientPortal, capture the returned turns, and assert that the
assistant turn with CanonicalTurnData ID "client-scope-turn-1" is present and
that the user turn was canonicalized per setCanonicalTurnDataFromPromptMessages;
if you prefer to drop the misleading case, delete the test instead of adding the
loader call.
Introduce SkipRoomCreation to chat creation so chats can be created without materializing Matrix rooms or sending disclaimers (left to bridgev2). Add corresponding test updates and propagate the flag into Codex welcome chat flows. Register static integration commands (cron, memory) at connector startup and fix command handler copying to avoid shared closures. Replace turn sequence allocation with an atomic upsert that handles nil scopes/contexts and return context values; ensure senders are joined before sending events. Convert ApprovalFlow.SendPrompt to return an error and update its caller; remove legacy conversation-state metadata bootstrap in favor of a DB-backed sdk_conversation_state table (added to SQL/migrations and tests). Misc: adjust DM power levels, tweak memory session SQL formatting, and change aichats_memory_session_files primary key.
Extract network message redaction into redactNetworkMessageViaPortal and replace inline simplevent.MessageRemove usages to centralize error handling. Add purgeLoginRuntimeState to consolidate login cleanup. Ensure pending queue items are populated with Portal/Meta/Event when queued or dispatched, prevent duplication of accepted messages and status events for steer+backlog, and add tests covering queue summary and steering behaviors. Also tighten executeMessageDelete to use redactEventViaPortal and propagate errors, and add a nil-check for portal.Bridge in typing context to avoid panics.
Multiple refactors and feature updates across bridges and SDK: - Migrate AI DB schema handling to db.Upgrade: aidb now registers an UpgradeTable and NewChild uses it; code and tests updated to call Upgrade instead of EnsureSchema. - Add SDK conversation state deletion helpers and call them on chat delete and login purge to clean persisted SDK state. - Make custom agents DB functions return an explicit error when the DB scope is unavailable. - Remove scheduler_rooms and stop forcing scheduled room creation during cron/heartbeat handling; code updated to resolve delivery targets without ensuring rooms. - Rename Codex command prefix and user-facing strings from "!ai" to "!codex" and update related behavior and tests. - Simplify bridge entrypoint RunMain signature and call path (bridgeentry changes) and adjust run_bridge invocation. - Simplify reaction/message parsing in SDK and remove several deprecated SDK types and hooks (message edits, reactions, model/chat/user info), trimming unused code. - Clean up agentremote CLI: remove legacy up/down commands and deprecated flags; update README to reference start instead of up. - Misc tests and small cleanups (memory purge query tweak, scheduler/heartbeat test updates, various nil checks and error messages). These changes centralize DB migration handling, improve SDK state cleanup, simplify scheduler semantics, and update the Codex bridge command UX.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bridges/ai/client.go (1)
1831-1835:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStore ack reaction IDs here, not the message event IDs.
pending.AckEventIDsis supposed to hold the temporary ack reactions to remove later, but this loop appendsentry.Event.ID. That means the completion path will either leave the acks behind or try to redact the original user messages instead of the reactions.Suggested fix
ackRemoveIDs := make([]id.EventID, 0, len(entries)) for _, entry := range entries { - if entry.Event != nil { - ackRemoveIDs = append(ackRemoveIDs, entry.Event.ID) + if entry.AckEventID != "" { + ackRemoveIDs = append(ackRemoveIDs, entry.AckEventID) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/client.go` around lines 1831 - 1835, The loop building ackRemoveIDs is appending the original message event IDs (entry.Event.ID) but should collect the temporary ack reaction event IDs to later remove from pending.AckEventIDs; change the append to use the reaction/ack field on the entry (e.g., entry.AckEventID or entry.ReactionID) and only append when that ack ID is non-nil/zero so ackRemoveIDs contains the reaction IDs (matching pending.AckEventIDs) instead of the message IDs.
♻️ Duplicate comments (1)
bridges/ai/chat.go (1)
273-276:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't make model contacts depend on the agent catalog.
This still loads agents before the login-level agent gate, and any
ListAgentsfailure abortsSearchUsers/GetContactListbefore model contacts are returned. When agents are disabled or the catalog is temporarily unavailable, contact discovery should degrade to model-only results instead of failing the whole request.Suggested fix
- agentsList, err := oc.sdkAgentCatalog().ListAgents(ctx, oc.UserLogin) - if err != nil { - return nil, fmt.Errorf("failed to load agents: %w", err) - } + var agentsList []*sdk.Agent + if oc.agentsEnabledForLogin() { + var err error + agentsList, err = oc.sdkAgentCatalog().ListAgents(ctx, oc.UserLogin) + if err != nil { + oc.loggerForContext(ctx).Warn().Err(err).Msg("Failed to load agent contacts") + } + }Also applies to: 295-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/chat.go` around lines 273 - 276, The current calls to oc.sdkAgentCatalog().ListAgents in SearchUsers/GetContactList abort the whole request on error; change both usages (the ListAgents call at the block referencing agentsList and the similar block at lines ~295-297) to be non-fatal: call ListAgents(ctx, oc.UserLogin), and if it returns an error, log/debug the error and set agentsList to nil/empty instead of returning the error so model-only contacts can still be returned; ensure subsequent code handles a nil/empty agentsList safely.
🧹 Nitpick comments (2)
README.md (1)
34-43: ⚡ Quick winAlign Quick start command with the new manager command model.
Line 37 still shows
agentremote run codex, while Line 42 introducesagentremote start <bridge>. Ifrunis deprecated or not the intended path, this is confusing in first-run docs—please align or explicitly state when to use each.✏️ Suggested doc tweak
```bash agentremote login agentremote list -agentremote run codex +agentremote start codex</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 34 - 43, The Quick Start shows a
deprecated/inconsistent command: replace the exampleagentremote run codex
with the new manager-style commandagentremote start codex(or, ifrunis
still supported, add a one-line clarification stating when to useagentremote run <bridge>vsagentremote start <bridge>). Update the block so it
consistently usesagentremote start <bridge>(e.g.,agentremote start codex)
and, if keepingrun, add a short parenthetical after the example explaining
its legacy behavior or use-case.</details> </blockquote></details> <details> <summary>bridges/ai/constructors.go (1)</summary><blockquote> `42-59`: _⚡ Quick win_ **Use the `bridge` callback argument inside `StartConnector`.** This hook currently assumes `InitConnector` already populated `oc.br`, then dereferences that field for commands and logging. Rebinding `oc.br = bridge` and using the callback argument locally removes that hidden lifecycle dependency and makes direct test invocation safer. <details> <summary>Suggested diff</summary> ```diff - StartConnector: func(ctx context.Context, _ *bridgev2.Bridge) error { + StartConnector: func(ctx context.Context, bridge *bridgev2.Bridge) error { + oc.br = bridge db := oc.bridgeDB() if db == nil { return fmt.Errorf("ai database not initialized") } if err := db.Upgrade(ctx); err != nil { return err } oc.applyRuntimeDefaults() oc.registerStaticIntegrationCommands() - if proc, ok := oc.br.Commands.(*commands.Processor); ok { - registerCommandsWithOwnerGuard(proc, &oc.Config, &oc.br.Log, HelpSectionAI) - oc.br.Log.Info().Msg("Registered AI commands with command processor") + if proc, ok := bridge.Commands.(*commands.Processor); ok { + registerCommandsWithOwnerGuard(proc, &oc.Config, &bridge.Log, HelpSectionAI) + bridge.Log.Info().Msg("Registered AI commands with command processor") } else { - oc.br.Log.Warn().Type("commands_type", oc.br.Commands).Msg("Failed to register AI commands: command processor type assertion failed") + bridge.Log.Warn().Type("commands_type", bridge.Commands).Msg("Failed to register AI commands: command processor type assertion failed") } oc.initProvisioning() return nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/constructors.go` around lines 42 - 59, StartConnector currently reads oc.br directly; rebind the passed-in bridge into the owner connector and use the local callback argument to avoid lifecycle dependency: assign oc.br = bridge at the start of StartConnector, then replace uses of oc.br (for commands and logging checks and messages) with the local bridge variable (or access via oc.br after rebinding), keeping the existing logic for db := oc.bridgeDB(), db.Upgrade(ctx), oc.applyRuntimeDefaults(), registerCommandsWithOwnerGuard(proc, &oc.Config, &oc.br.Log, HelpSectionAI) (ensure proc type assertion uses bridge.Commands), oc.initProvisioning(), and return behavior unchanged.
🤖 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/chat.go`:
- Around line 981-997: The portal is having ResolvedTarget set but
portal.OtherUserID remains pointing to the model ghost, so senderForPortal still
short-circuits on OtherUserID and messages appear from the wrong ghost; update
the portal initialization code (around resolveAgentDisplayName, agentUserID,
portalMeta, setPortalResolvedTarget) to also set portal.OtherUserID =
agentGhostID (and persist it via oc.savePortalQuiet) whenever you set the
ResolvedTarget for an agent chat (ensure this change aligns with
initPortalForChat and senderForPortal expectations), so the agent ghost is used
as the sender.
In `@bridges/ai/custom_agents_db.go`:
- Around line 116-120: The DB scan error is being shadowed by the empty-JSON
check because the code checks strings.TrimSpace(raw) before handling
non-sql.ErrNoRows errors; change the order so you first handle scan errors:
after the Scan/Query call, check if err == sql.ErrNoRows and return nil,nil,
then if err != nil return err, and only then check if strings.TrimSpace(raw) ==
"" and return nil,nil; update the block that references sql.ErrNoRows, raw, and
the surrounding error handling in this function to follow that order.
In `@bridges/ai/heartbeat_delivery_test.go`:
- Around line 20-34: The test reassigns the loop-local variable `portal =
persisted`, which doesn't update the original fixture the caller holds; instead,
after loading and saving via `client.UserLogin.Bridge.GetPortalByKey` and
`persisted.Save`, copy the persisted fields back into the caller's object (e.g.,
assign fields or do a deep copy like `*portal = *persisted`) so the external
pointer reflects the persisted state; update the code around `persisted`,
`portal`, `GetPortalByKey`, and `Save` to perform the copy rather than
reassigning the local variable.
In `@bridges/ai/pending_queue.go`:
- Around line 198-216: Dropped items in the overflow branch (when
overflow.ItemsToDrop > 0) can have non-nil acceptedMessages and currently are
removed without finalization; update the overflow handling in the same area that
manipulates state.Items so that before discarding each dropped entry you collect
entries with non-nil acceptedMessages and call finalizeStoppedQueueItems for
them (mirroring clearPendingQueue behavior), then clear their acceptedMessages
and proceed to update state.DroppedCount and SummaryLines as now; reference
symbols: overflow.ItemsToDrop, state.Items, entry.acceptedMessages,
finalizeStoppedQueueItems, clearPendingQueue, enqueuePendingItem.
---
Outside diff comments:
In `@bridges/ai/client.go`:
- Around line 1831-1835: The loop building ackRemoveIDs is appending the
original message event IDs (entry.Event.ID) but should collect the temporary ack
reaction event IDs to later remove from pending.AckEventIDs; change the append
to use the reaction/ack field on the entry (e.g., entry.AckEventID or
entry.ReactionID) and only append when that ack ID is non-nil/zero so
ackRemoveIDs contains the reaction IDs (matching pending.AckEventIDs) instead of
the message IDs.
---
Duplicate comments:
In `@bridges/ai/chat.go`:
- Around line 273-276: The current calls to oc.sdkAgentCatalog().ListAgents in
SearchUsers/GetContactList abort the whole request on error; change both usages
(the ListAgents call at the block referencing agentsList and the similar block
at lines ~295-297) to be non-fatal: call ListAgents(ctx, oc.UserLogin), and if
it returns an error, log/debug the error and set agentsList to nil/empty instead
of returning the error so model-only contacts can still be returned; ensure
subsequent code handles a nil/empty agentsList safely.
---
Nitpick comments:
In `@bridges/ai/constructors.go`:
- Around line 42-59: StartConnector currently reads oc.br directly; rebind the
passed-in bridge into the owner connector and use the local callback argument to
avoid lifecycle dependency: assign oc.br = bridge at the start of
StartConnector, then replace uses of oc.br (for commands and logging checks and
messages) with the local bridge variable (or access via oc.br after rebinding),
keeping the existing logic for db := oc.bridgeDB(), db.Upgrade(ctx),
oc.applyRuntimeDefaults(), registerCommandsWithOwnerGuard(proc, &oc.Config,
&oc.br.Log, HelpSectionAI) (ensure proc type assertion uses bridge.Commands),
oc.initProvisioning(), and return behavior unchanged.
In `@README.md`:
- Around line 34-43: The Quick Start shows a deprecated/inconsistent command:
replace the example `agentremote run codex` with the new manager-style command
`agentremote start codex` (or, if `run` is still supported, add a one-line
clarification stating when to use `agentremote run <bridge>` vs `agentremote
start <bridge>`). Update the block so it consistently uses `agentremote start
<bridge>` (e.g., `agentremote start codex`) and, if keeping `run`, add a short
parenthetical after the example explaining its legacy behavior or use-case.
🪄 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: 7fbe12cc-3636-4434-9f34-b89a68fd610c
📒 Files selected for processing (31)
README.mdbridges/ai/chat.gobridges/ai/chat_bootstrap_test.gobridges/ai/client.gobridges/ai/command_registry.gobridges/ai/constructors.gobridges/ai/custom_agents_db.gobridges/ai/delete_chat.gobridges/ai/heartbeat_delivery_test.gobridges/ai/integrations.gobridges/ai/logout_cleanup.gobridges/ai/pending_queue.gobridges/ai/portal_send.gobridges/ai/queue_runtime.gobridges/ai/queue_status_test.gobridges/ai/response_finalization.gobridges/ai/scheduler_cron.gobridges/ai/scheduler_heartbeat.gobridges/ai/scheduler_heartbeat_test.gobridges/ai/scheduler_rooms.gobridges/ai/sessions_tools_test.gobridges/ai/test_login_helpers_test.gobridges/ai/tools.gobridges/ai/turn_store.gobridges/ai/typing_queue.gobridges/codex/client.gobridges/codex/config.gobridges/codex/connector.gobridges/codex/connector_test.gobridges/codex/constructors.gobridges/codex/directory_manager.go
🚧 Files skipped from review as they are similar to previous changes (2)
- bridges/ai/integrations.go
- bridges/ai/logout_cleanup.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build-agentremote-cli-docker (amd64)
- GitHub Check: build-agentremote-cli-docker (arm64)
- GitHub Check: build-docker
- GitHub Check: Lint
- GitHub Check: Lint
- GitHub Check: build-docker
🧰 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:11.979Z
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:03.718Z
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:56.093Z
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:19.585Z
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-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/command_registry.gobridges/ai/constructors.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/command_registry.gobridges/ai/heartbeat_delivery_test.gobridges/ai/delete_chat.gobridges/ai/chat_bootstrap_test.gobridges/ai/custom_agents_db.gobridges/ai/constructors.gobridges/ai/pending_queue.gobridges/ai/portal_send.gobridges/ai/client.gobridges/ai/chat.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/command_registry.gobridges/ai/heartbeat_delivery_test.gobridges/ai/delete_chat.gobridges/ai/chat_bootstrap_test.gobridges/ai/custom_agents_db.gobridges/ai/constructors.gobridges/ai/pending_queue.gobridges/ai/portal_send.gobridges/ai/client.gobridges/ai/chat.go
📚 Learning: 2026-04-12T18:51:11.979Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/codex/portal_state_db.go:127-140
Timestamp: 2026-04-12T18:51:11.979Z
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/command_registry.gobridges/ai/heartbeat_delivery_test.goREADME.mdbridges/ai/delete_chat.gobridges/ai/chat_bootstrap_test.gobridges/ai/custom_agents_db.gobridges/ai/constructors.gobridges/ai/portal_send.gobridges/ai/client.gobridges/ai/chat.go
📚 Learning: 2026-04-12T17:55:56.093Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/codex/portal_state_db.go:78-92
Timestamp: 2026-04-12T17:55:56.093Z
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/heartbeat_delivery_test.goREADME.mdbridges/ai/delete_chat.gobridges/ai/chat_bootstrap_test.gobridges/ai/custom_agents_db.gobridges/ai/portal_send.gobridges/ai/client.gobridges/ai/chat.go
📚 Learning: 2026-04-12T17:47:03.718Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 103
File: bridges/ai/metadata.go:95-100
Timestamp: 2026-04-12T17:47:03.718Z
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:
README.md
🔇 Additional comments (13)
bridges/ai/command_registry.go (2)
20-21: Allowlist update looks correct.Adding
cronandmemoryhere matches the integrated command surface and keeps owner-guard registration behavior consistent.
126-143: Handler-copy wrapping is a solid fix.Copying the handler before wrapping
Funcavoids mutating shared registry handler state and makes this registration path safer.bridges/ai/constructors.go (1)
6-6: No action needed. The module targets Go 1.25.0, which is well above the Go 1.21 minimum required forslices.ContainsFunc. The code compiles and runs without issues.> Likely an incorrect or invalid review comment.bridges/ai/heartbeat_delivery_test.go (3)
38-67: Good fallback coverage for mismatched session room deliveryThis test clearly validates portal, room, and reason (
last-active) on the delivery target path.
69-95: Session-portal fallback scenario is well coveredThis asserts the explicit-session mismatch path cleanly and verifies fallback to last active portal.
97-115: No-history behavior test looks correctThe test correctly expects an error and verifies no delivery portal is selected when no history exists.
bridges/ai/pending_queue.go (7)
6-12: LGTM!The new imports are correctly added and used throughout the file -
strconvfor building overflow summary strings anddatabasefor theacceptedMessagesfield type.
18-27: LGTM!The struct change from
rawEventContenttoacceptedMessages []*database.Messagealigns with the PR's migration to DB-backed models. The context snippets confirm proper handling of this field inqueue_runtime.goandroom_runs.go.
43-53: LGTM!Clean helper types with good use of embedding and generics. The
queueState[T any]provides type-safe state management used effectively inenqueuePendingItem.
317-332: LGTM!The overflow summary generation is clean and readable. The pluralization logic correctly handles all cases, and the optional "Summary:" section with bullet points is well-formatted.
341-410: LGTM!The simplified return signature is cleaner, and the logic correctly handles all dispatch scenarios including the edge case where items were dropped but a synthetic summary dispatch is still needed (using
lastItemas fallback).
419-445: LGTM!Clean inline prompt composition. The
acceptedMessagesaccumulation from all queued items is correctly implemented, and the prompt format with numbered queued sections is clear and readable.
496-502: LGTM!Direct struct construction is cleaner and more explicit than the removed helper function. The code clearly shows the message structure with role and text block.
| agentName := oc.resolveAgentDisplayName(ctx, agent) | ||
| agentGhostID := oc.agentUserID(agent.ID) | ||
| pm := portalMeta(portal) | ||
| setPortalResolvedTarget(portal, pm, agentGhostID) | ||
| if applyModelOverride { | ||
| pm.RuntimeModelOverride = ResolveAlias(modelID) | ||
| } | ||
|
|
||
| chatInfo := chatResp.PortalInfo | ||
| if err := oc.materializePortalRoom(ctx, newPortal, chatInfo, portalRoomMaterializeOptions{SendWelcome: true}); err != nil { | ||
| oc.sendSystemNotice(ctx, portal, "Couldn't create the room: "+err.Error()) | ||
| return | ||
| agentAvatar := strings.TrimSpace(agent.AvatarURL) | ||
| if agentAvatar == "" { | ||
| agentAvatar = strings.TrimSpace(agents.DefaultAgentAvatarMXC) | ||
| } | ||
|
|
||
| roomLink := fmt.Sprintf("https://matrix.to/#/%s", newPortal.MXID) | ||
| oc.sendSystemNotice(ctx, portal, fmt.Sprintf( | ||
| "New %s chat created.\nOpen: %s", | ||
| agentName, roomLink, | ||
| )) | ||
| } | ||
|
|
||
| func (oc *AIClient) createAndOpenModelChat(ctx context.Context, portal *bridgev2.Portal, modelID string) { | ||
| chatResp, err := oc.createNewChat(ctx, modelID) | ||
| if err != nil { | ||
| oc.sendSystemNotice(ctx, portal, "Couldn't create the chat: "+err.Error()) | ||
| return | ||
| if agentAvatar != "" { | ||
| portal.AvatarID = networkid.AvatarID(agentAvatar) | ||
| portal.AvatarMXC = id.ContentURIString(agentAvatar) | ||
| } | ||
|
|
||
| newPortal := chatResp.Portal | ||
| chatInfo := chatResp.PortalInfo | ||
| if err := oc.materializePortalRoom(ctx, newPortal, chatInfo, portalRoomMaterializeOptions{SendWelcome: true}); err != nil { | ||
| oc.sendSystemNotice(ctx, portal, "Couldn't create the room: "+err.Error()) | ||
| return | ||
| oc.savePortalQuiet(ctx, portal, saveReason) | ||
| oc.ensureAgentGhostDisplayName(ctx, agent.ID, modelID, agentName) |
There was a problem hiding this comment.
Agent chats still keep the model ghost as OtherUserID.
initPortalForChat() seeds portal.OtherUserID with the model user, and senderForPortal() now short-circuits on portal.OtherUserID before looking at ResolvedTarget. Because this helper only updates ResolvedTarget, agent rooms will keep sending notices/messages as the model ghost instead of the agent ghost.
Suggested fix
agentName := oc.resolveAgentDisplayName(ctx, agent)
agentGhostID := oc.agentUserID(agent.ID)
pm := portalMeta(portal)
setPortalResolvedTarget(portal, pm, agentGhostID)
+ portal.OtherUserID = agentGhostID
if applyModelOverride {
pm.RuntimeModelOverride = ResolveAlias(modelID)
}📝 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.
| agentName := oc.resolveAgentDisplayName(ctx, agent) | |
| agentGhostID := oc.agentUserID(agent.ID) | |
| pm := portalMeta(portal) | |
| setPortalResolvedTarget(portal, pm, agentGhostID) | |
| if applyModelOverride { | |
| pm.RuntimeModelOverride = ResolveAlias(modelID) | |
| } | |
| chatInfo := chatResp.PortalInfo | |
| if err := oc.materializePortalRoom(ctx, newPortal, chatInfo, portalRoomMaterializeOptions{SendWelcome: true}); err != nil { | |
| oc.sendSystemNotice(ctx, portal, "Couldn't create the room: "+err.Error()) | |
| return | |
| agentAvatar := strings.TrimSpace(agent.AvatarURL) | |
| if agentAvatar == "" { | |
| agentAvatar = strings.TrimSpace(agents.DefaultAgentAvatarMXC) | |
| } | |
| roomLink := fmt.Sprintf("https://matrix.to/#/%s", newPortal.MXID) | |
| oc.sendSystemNotice(ctx, portal, fmt.Sprintf( | |
| "New %s chat created.\nOpen: %s", | |
| agentName, roomLink, | |
| )) | |
| } | |
| func (oc *AIClient) createAndOpenModelChat(ctx context.Context, portal *bridgev2.Portal, modelID string) { | |
| chatResp, err := oc.createNewChat(ctx, modelID) | |
| if err != nil { | |
| oc.sendSystemNotice(ctx, portal, "Couldn't create the chat: "+err.Error()) | |
| return | |
| if agentAvatar != "" { | |
| portal.AvatarID = networkid.AvatarID(agentAvatar) | |
| portal.AvatarMXC = id.ContentURIString(agentAvatar) | |
| } | |
| newPortal := chatResp.Portal | |
| chatInfo := chatResp.PortalInfo | |
| if err := oc.materializePortalRoom(ctx, newPortal, chatInfo, portalRoomMaterializeOptions{SendWelcome: true}); err != nil { | |
| oc.sendSystemNotice(ctx, portal, "Couldn't create the room: "+err.Error()) | |
| return | |
| oc.savePortalQuiet(ctx, portal, saveReason) | |
| oc.ensureAgentGhostDisplayName(ctx, agent.ID, modelID, agentName) | |
| agentName := oc.resolveAgentDisplayName(ctx, agent) | |
| agentGhostID := oc.agentUserID(agent.ID) | |
| pm := portalMeta(portal) | |
| setPortalResolvedTarget(portal, pm, agentGhostID) | |
| portal.OtherUserID = agentGhostID | |
| if applyModelOverride { | |
| pm.RuntimeModelOverride = ResolveAlias(modelID) | |
| } | |
| agentAvatar := strings.TrimSpace(agent.AvatarURL) | |
| if agentAvatar == "" { | |
| agentAvatar = strings.TrimSpace(agents.DefaultAgentAvatarMXC) | |
| } | |
| if agentAvatar != "" { | |
| portal.AvatarID = networkid.AvatarID(agentAvatar) | |
| portal.AvatarMXC = id.ContentURIString(agentAvatar) | |
| } | |
| oc.savePortalQuiet(ctx, portal, saveReason) | |
| oc.ensureAgentGhostDisplayName(ctx, agent.ID, modelID, agentName) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/chat.go` around lines 981 - 997, The portal is having
ResolvedTarget set but portal.OtherUserID remains pointing to the model ghost,
so senderForPortal still short-circuits on OtherUserID and messages appear from
the wrong ghost; update the portal initialization code (around
resolveAgentDisplayName, agentUserID, portalMeta, setPortalResolvedTarget) to
also set portal.OtherUserID = agentGhostID (and persist it via
oc.savePortalQuiet) whenever you set the ResolvedTarget for an agent chat
(ensure this change aligns with initPortalForChat and senderForPortal
expectations), so the agent ghost is used as the sender.
| if err == sql.ErrNoRows || strings.TrimSpace(raw) == "" { | ||
| return nil, nil | ||
| } | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Handle Scan error before checking empty JSON content.
At Line 116, a non-sql.ErrNoRows DB error can be accidentally swallowed because raw is still empty and the function returns nil, nil.
Suggested fix
err := scope.db.QueryRow(ctx, `
SELECT content_json
FROM `+aiCustomAgentsTable+`
WHERE bridge_id=$1 AND login_id=$2 AND agent_id=$3
`, scope.bridgeID, scope.loginID, strings.TrimSpace(agentID)).Scan(&raw)
- if err == sql.ErrNoRows || strings.TrimSpace(raw) == "" {
+ if errors.Is(err, sql.ErrNoRows) {
return nil, nil
}
if err != nil {
return nil, err
}
+ if strings.TrimSpace(raw) == "" {
+ return nil, nil
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/custom_agents_db.go` around lines 116 - 120, The DB scan error is
being shadowed by the empty-JSON check because the code checks
strings.TrimSpace(raw) before handling non-sql.ErrNoRows errors; change the
order so you first handle scan errors: after the Scan/Query call, check if err
== sql.ErrNoRows and return nil,nil, then if err != nil return err, and only
then check if strings.TrimSpace(raw) == "" and return nil,nil; update the block
that references sql.ErrNoRows, raw, and the surrounding error handling in this
function to follow that order.
| persisted, err := client.UserLogin.Bridge.GetPortalByKey(context.Background(), portal.PortalKey) | ||
| if err != nil { | ||
| t.Fatalf("GetPortalByKey(%v) returned error: %v", portal.PortalKey, err) | ||
| } | ||
| persisted.Receiver = portal.Receiver | ||
| persisted.OtherUserID = portal.OtherUserID | ||
| persisted.MXID = portal.MXID | ||
| persisted.Name = portal.Name | ||
| persisted.Topic = portal.Topic | ||
| persisted.Metadata = portal.Metadata | ||
| if err = persisted.Save(context.Background()); err != nil { | ||
| t.Fatalf("Save(%v) returned error: %v", portal.PortalKey, err) | ||
| } | ||
| portal = persisted | ||
| } |
There was a problem hiding this comment.
portal = persisted does not update the caller’s portal object
Line 33 only reassigns the loop-local variable, so callers still hold the pre-persist pointer. If the helper’s intent is to keep test fixtures synced with persisted state, copy back into the pointed struct.
Suggested fix
if err = persisted.Save(context.Background()); err != nil {
t.Fatalf("Save(%v) returned error: %v", portal.PortalKey, err)
}
- portal = persisted
+ *portal = *persisted
}
}
}📝 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.
| persisted, err := client.UserLogin.Bridge.GetPortalByKey(context.Background(), portal.PortalKey) | |
| if err != nil { | |
| t.Fatalf("GetPortalByKey(%v) returned error: %v", portal.PortalKey, err) | |
| } | |
| persisted.Receiver = portal.Receiver | |
| persisted.OtherUserID = portal.OtherUserID | |
| persisted.MXID = portal.MXID | |
| persisted.Name = portal.Name | |
| persisted.Topic = portal.Topic | |
| persisted.Metadata = portal.Metadata | |
| if err = persisted.Save(context.Background()); err != nil { | |
| t.Fatalf("Save(%v) returned error: %v", portal.PortalKey, err) | |
| } | |
| portal = persisted | |
| } | |
| persisted, err := client.UserLogin.Bridge.GetPortalByKey(context.Background(), portal.PortalKey) | |
| if err != nil { | |
| t.Fatalf("GetPortalByKey(%v) returned error: %v", portal.PortalKey, err) | |
| } | |
| persisted.Receiver = portal.Receiver | |
| persisted.OtherUserID = portal.OtherUserID | |
| persisted.MXID = portal.MXID | |
| persisted.Name = portal.Name | |
| persisted.Topic = portal.Topic | |
| persisted.Metadata = portal.Metadata | |
| if err = persisted.Save(context.Background()); err != nil { | |
| t.Fatalf("Save(%v) returned error: %v", portal.PortalKey, err) | |
| } | |
| *portal = *persisted | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/heartbeat_delivery_test.go` around lines 20 - 34, The test
reassigns the loop-local variable `portal = persisted`, which doesn't update the
original fixture the caller holds; instead, after loading and saving via
`client.UserLogin.Bridge.GetPortalByKey` and `persisted.Save`, copy the
persisted fields back into the caller's object (e.g., assign fields or do a deep
copy like `*portal = *persisted`) so the external pointer reflects the persisted
state; update the code around `persisted`, `portal`, `GetPortalByKey`, and
`Save` to perform the copy rather than reassigning the local variable.
| } else if dropCount := overflow.ItemsToDrop; dropCount >= 1 { | ||
| dropped := state.Items[:dropCount] | ||
| state.Items = state.Items[dropCount:] | ||
| if overflow.ShouldSummarize { | ||
| for _, entry := range dropped { | ||
| state.DroppedCount++ | ||
| summary := entry.summaryLine | ||
| if summary == "" { | ||
| summary = strings.TrimSpace(entry.pending.MessageBody) | ||
| } | ||
| summary = strings.TrimSpace(summary) | ||
| if summary != "" { | ||
| state.SummaryLines = append(state.SummaryLines, airuntime.BuildQueueSummaryLine(summary, 160)) | ||
| } | ||
| } | ||
| if len(state.SummaryLines) > state.Cap { | ||
| state.SummaryLines = state.SummaryLines[len(state.SummaryLines)-state.Cap:] | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are callers that enqueue items with non-nil acceptedMessages
# that could hit the overflow path
# Find where pendingQueueItem is constructed with acceptedMessages
rg -nP --type=go -C5 'pendingQueueItem\{' | head -80
echo "---"
# Check how acceptedMessages is populated before enqueue
rg -nP --type=go -C3 'acceptedMessages.*=' bridges/ai/Repository: beeper/agentremote
Length of output: 8698
🏁 Script executed:
# Find finalizeStoppedQueueItems to understand what it does
rg -nP --type=go -B2 -A10 'finalizeStoppedQueueItems' bridges/ai/Repository: beeper/agentremote
Length of output: 4441
🏁 Script executed:
# Check the context around line 101 to see clearPendingQueue
sed -n '95,110p' bridges/ai/pending_queue.goRepository: beeper/agentremote
Length of output: 464
🏁 Script executed:
# Check the full enqueuePendingItem function to understand item flow
sed -n '170,230p' bridges/ai/pending_queue.goRepository: beeper/agentremote
Length of output: 1932
🏁 Script executed:
# Look at dispatchOrQueueCore to see if items are steered before enqueue
rg -nP --type=go -B5 -A15 'func.*dispatchOrQueueCore' bridges/ai/Repository: beeper/agentremote
Length of output: 1324
🏁 Script executed:
# Get the full dispatchOrQueueCore function
sed -n '165,280p' bridges/ai/queue_runtime.goRepository: beeper/agentremote
Length of output: 3399
🏁 Script executed:
# Also check where enqueuePendingItem is called from
rg -nP --type=go 'enqueuePendingItem' bridges/ai/ -B3 -A1Repository: beeper/agentremote
Length of output: 1280
🏁 Script executed:
# Find consumeQueueSummary and see how summaries are used
rg -nP --type=go -B3 -A10 'consumeQueueSummary' bridges/ai/Repository: beeper/agentremote
Length of output: 2548
🏁 Script executed:
# Check what happens with summaryPrompt in processPendingQueue
sed -n '350,400p' bridges/ai/pending_queue.goRepository: beeper/agentremote
Length of output: 1349
Handle dropped queue items with acceptedMessages – they need finalization.
When items are dropped due to overflow (lines 198-216), items with non-nil acceptedMessages are discarded without calling finalizeStoppedQueueItems. This can occur when items reach enqueuePendingItem without being steered (e.g., non-text types or when steering is disabled), since the code only clears acceptedMessages for steered items (line 208).
Compare to clearPendingQueue (line 101), which calls finalizeStoppedQueueItems to send a "Stopped" notification to users. Dropped items skip this, leaving users with no notification that their queued messages were discarded. The summary lines sent to the agent are internal context, not user-facing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/pending_queue.go` around lines 198 - 216, Dropped items in the
overflow branch (when overflow.ItemsToDrop > 0) can have non-nil
acceptedMessages and currently are removed without finalization; update the
overflow handling in the same area that manipulates state.Items so that before
discarding each dropped entry you collect entries with non-nil acceptedMessages
and call finalizeStoppedQueueItems for them (mirroring clearPendingQueue
behavior), then clear their acceptedMessages and proceed to update
state.DroppedCount and SummaryLines as now; reference symbols:
overflow.ItemsToDrop, state.Items, entry.acceptedMessages,
finalizeStoppedQueueItems, clearPendingQueue, enqueuePendingItem.
Major refactor of AI/codex bridge behavior and tool typing: - Remove direct Matrix MessageStatus sending in many places (pending/success/error) and update tests accordingly. - Simplify queuing/dispatch logic: remove debounce, steer/backlog mechanics and pending queue workflows; new behaviour returns a retriable error when a turn is already running in a room. - Refactor Codex client to drop per-room pending message queue and adjust room acquisition semantics (acquireRoomIfIdle); remove related helpers and tests. - Replace local ToolType alias with matrixevents.ToolType throughout streaming/tool execution code and tests; update imports. - Adjust AgentStore ListAvailableTools to return toolspec.ToolInfo and fix sessions/tools argument parsing. - Misc: import cleanup and small API/test updates to reflect these changes. These changes consolidate tool-type usage and simplify room/queue lifecycle handling across the bridges.
Move github.com/beeper/agentremote/sdk import to the end of import blocks to better group external packages, and remove a redundant portal reassignment in cacheHeartbeatTestPortals (heartbeat_delivery_test.go) that could mask saved state. No behavior changes aside from the test cleanup and import formatting.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
bridges/ai/logout_cleanup.go (1)
79-82: 💤 Low valueDuplicate deletion of
aiLoginStateTable.The table is deleted explicitly at lines 79-82, and then again via
clearLoginState(called frompurgeLoginRuntimeStateat line 102). Per Context snippet 1,clearLoginStateinternally executesDELETE FROM aiLoginStateTable WHERE bridge_id=$1 AND login_id=$2.The second DELETE is a harmless no-op, but it's unnecessary work. Consider either:
- Removing the explicit deletion at lines 79-82 since
purgeLoginRuntimeStatehandles it, or- Modifying
clearLoginStateto skip the DB deletion when called from purge (e.g., add a flag or split into two methods)Also applies to: 101-103
🤖 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 79 - 82, Remove the duplicate explicit DELETE calls that target aiLoginStateTable via recordDelete(...) using bridgeID and loginID (the calls at the spots that match the shown snippet and the other instance around lines 101-103) because purgeLoginRuntimeState already calls clearLoginState which performs the same deletion; delete those recordDelete(...) invocations and leave purgeLoginRuntimeState/clearLoginState as the single place performing the DB DELETE, ensuring no other logic depends on the removed calls.
🤖 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/accepted_echo.go`:
- Around line 27-29: The two separate calls consumeRoomRunAcceptedMessages and
consumeRoomRunStatusEvents can race and split paired messages/events; add a
single helper on AIClient (e.g., consumeRoomRunAcceptedBatch) that calls
getRoomRun(roomID), locks run.mu once, clones run.acceptedUserMessages and
run.statusEvents, clears those slices, unlocks, and returns both slices
atomically; then replace the two separate consumes in accepted_echo.go with a
single call to consumeRoomRunAcceptedBatch to use the atomic pair.
In `@bridges/ai/handlematrix.go`:
- Around line 668-697: The media/audio/video/file handling paths need the same
group-activation/mention guard and group-history prepending as the text path:
check mc.WasMentioned or groupActivation before creating/dispatching queueItem
and return/ignore when not activated; when accepted, build promptContext via
oc.buildPromptContextForPendingMessage and prepend the buffered group-history to
promptContext (same logic used for text messages) before calling
oc.dispatchOrQueueCore (see symbols: mc.WasMentioned, groupActivation,
oc.buildPromptContextForPendingMessage, promptContext, dispatchOrQueueCore,
pendingQueueItem and the branches that construct userMessage/queueItem for
media); apply this change consistently in the other media handling blocks
referenced (the other dispatch branches).
- Around line 364-370: The code currently treats any error from
loadAIConversationMessage the same as a missing row and falls back to
cloneMessageForAIHistory, which can overwrite AI-only history; change the logic
in the block that calls loadAIConversationMessage so that if err != nil you
return or propagate the error (do not set transcriptMsg to the clone), and only
use cloneMessageForAIHistory(edit.EditTarget) when err == nil and transcriptMsg
== nil (a true miss). Update the error handling around loadAIConversationMessage
in the function containing transcriptMsg so that Err cases are handled
separately from nil-result cases.
---
Nitpick comments:
In `@bridges/ai/logout_cleanup.go`:
- Around line 79-82: Remove the duplicate explicit DELETE calls that target
aiLoginStateTable via recordDelete(...) using bridgeID and loginID (the calls at
the spots that match the shown snippet and the other instance around lines
101-103) because purgeLoginRuntimeState already calls clearLoginState which
performs the same deletion; delete those recordDelete(...) invocations and leave
purgeLoginRuntimeState/clearLoginState as the single place performing the DB
DELETE, ensuring no other logic depends on the removed calls.
🪄 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: 7ce4f40d-6fc2-4dbd-8edd-0bdbb82eb435
📒 Files selected for processing (22)
bridges/ai/abort_helpers.gobridges/ai/accepted_echo.gobridges/ai/agentstore.gobridges/ai/chat.gobridges/ai/delete_chat.gobridges/ai/events.gobridges/ai/handleai.gobridges/ai/handlematrix.gobridges/ai/heartbeat_delivery_test.gobridges/ai/logout_cleanup.gobridges/ai/queue_runtime.gobridges/ai/sessions_tools.gobridges/ai/streaming_actions.gobridges/ai/streaming_function_calls.gobridges/ai/streaming_output_handlers.gobridges/ai/streaming_output_handlers_test.gobridges/ai/streaming_output_items.gobridges/ai/streaming_output_items_test.gobridges/ai/streaming_responses_api.gobridges/ai/tool_execution.gobridges/codex/client.gobridges/codex/pending_queue_test.go
💤 Files with no reviewable changes (1)
- bridges/ai/events.go
✅ Files skipped from review due to trivial changes (2)
- bridges/ai/heartbeat_delivery_test.go
- bridges/ai/chat.go
🚧 Files skipped from review as they are similar to previous changes (2)
- bridges/ai/agentstore.go
- bridges/ai/handleai.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: Lint
- GitHub Check: build-docker
- GitHub Check: build-agentremote-cli-docker (amd64)
- GitHub Check: Lint
- GitHub Check: build-docker
🧰 Additional context used
🧠 Learnings (2)
📚 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/abort_helpers.gobridges/ai/accepted_echo.gobridges/ai/logout_cleanup.gobridges/ai/delete_chat.gobridges/ai/handlematrix.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/abort_helpers.gobridges/ai/accepted_echo.gobridges/ai/logout_cleanup.gobridges/ai/delete_chat.gobridges/ai/handlematrix.go
🔇 Additional comments (8)
bridges/ai/logout_cleanup.go (3)
40-45: LGTM on error aggregation pattern.The
recordDeletehelper combined witherrors.Joinproperly addresses the prior concern about silent failures. All deletion errors are now collected and logged together, allowing the cleanup to attempt all tables while still surfacing any failures.Also applies to: 98-100
116-128: LGTM!The
execDeletehelper properly returns errors for aggregation while also logging warnings. Thecontext.Background()fallback is appropriate for cleanup scenarios where context may be unavailable.
106-114: LGTM!Runtime state cleanup is properly protected with mutex locks and handles the nil receiver case.
bridges/ai/delete_chat.go (3)
9-10: LGTM!Clean addition of the SDK import to support the new
sdk.DeleteConversationStatecall.
30-34: LGTM!The best-effort error handling for
sdk.DeleteConversationStateis appropriate—logging the warning without failing the overall deletion aligns with the documented intent. The portal ID in the log message aids debugging.
72-89:deleteAITurnsForPortalcorrectly handles its own scope—placement outside the check is intentional.The function on line 87 calls
resolveAIDBPortalScope(ctx, nil, portal)internally and returns early if scope resolution fails, so it safely executes regardless of the caller's scope validity.bridges/ai/abort_helpers.go (1)
144-154: Good move to per-event status emission through SDK boundary.The switch to
queueStatusEvents(...)+sdk.SendMessageStatus(...)is consistent with the newer runtime status flow and keeps emission behavior centralized.bridges/ai/accepted_echo.go (1)
12-20: Status helper looks solid.This cleanly emits success status through
sdk.SendMessageStatuswithout bypassing the SDK boundary.
| messages := oc.consumeRoomRunAcceptedMessages(state.roomID) | ||
| statusEvents := oc.consumeRoomRunStatusEvents(state.roomID) | ||
| if len(messages) == 0 && len(statusEvents) == 0 { |
There was a problem hiding this comment.
Consume accepted messages and status events atomically.
These two consumes happen under separate lock cycles. A concurrent append between them can split a logical pair and allow success status emission before the corresponding accepted message is persisted.
Proposed fix
- messages := oc.consumeRoomRunAcceptedMessages(state.roomID)
- statusEvents := oc.consumeRoomRunStatusEvents(state.roomID)
+ messages, statusEvents := oc.consumeRoomRunAcceptedBatch(state.roomID)// room_runs.go (new helper)
func (oc *AIClient) consumeRoomRunAcceptedBatch(roomID id.RoomID) ([]*database.Message, []*event.Event) {
run := oc.getRoomRun(roomID)
if run == nil {
return nil, nil
}
run.mu.Lock()
messages := slices.Clone(run.acceptedUserMessages)
events := slices.Clone(run.statusEvents)
run.acceptedUserMessages = nil
run.statusEvents = nil
run.mu.Unlock()
return messages, events
}📝 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.
| messages := oc.consumeRoomRunAcceptedMessages(state.roomID) | |
| statusEvents := oc.consumeRoomRunStatusEvents(state.roomID) | |
| if len(messages) == 0 && len(statusEvents) == 0 { | |
| messages, statusEvents := oc.consumeRoomRunAcceptedBatch(state.roomID) | |
| if len(messages) == 0 && len(statusEvents) == 0 { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/accepted_echo.go` around lines 27 - 29, The two separate calls
consumeRoomRunAcceptedMessages and consumeRoomRunStatusEvents can race and split
paired messages/events; add a single helper on AIClient (e.g.,
consumeRoomRunAcceptedBatch) that calls getRoomRun(roomID), locks run.mu once,
clones run.acceptedUserMessages and run.statusEvents, clears those slices,
unlocks, and returns both slices atomically; then replace the two separate
consumes in accepted_echo.go with a single call to consumeRoomRunAcceptedBatch
to use the atomic pair.
| transcriptMsg, err := oc.loadAIConversationMessage(ctx, portal, edit.EditTarget.ID, edit.EditTarget.MXID) | ||
| if err != nil { | ||
| oc.loggerForContext(ctx).Warn().Err(err).Msg("Failed to load edited conversation turn") | ||
| } | ||
| if transcriptMsg == nil { | ||
| transcriptMsg = cloneMessageForAIHistory(edit.EditTarget) | ||
| } |
There was a problem hiding this comment.
Don't fall back to the bridge row after a failed transcript read.
A real loadAIConversationMessage error currently drops into cloneMessageForAIHistory(edit.EditTarget), so a transient read failure can overwrite the AI turn with the bridge-message snapshot and lose AI-history-only fields. Only use that fallback on an actual miss; otherwise return the load error.
Suggested fix
transcriptMsg, err := oc.loadAIConversationMessage(ctx, portal, edit.EditTarget.ID, edit.EditTarget.MXID)
if err != nil {
oc.loggerForContext(ctx).Warn().Err(err).Msg("Failed to load edited conversation turn")
+ return err
}
if transcriptMsg == nil {
transcriptMsg = cloneMessageForAIHistory(edit.EditTarget)
}📝 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.
| transcriptMsg, err := oc.loadAIConversationMessage(ctx, portal, edit.EditTarget.ID, edit.EditTarget.MXID) | |
| if err != nil { | |
| oc.loggerForContext(ctx).Warn().Err(err).Msg("Failed to load edited conversation turn") | |
| } | |
| if transcriptMsg == nil { | |
| transcriptMsg = cloneMessageForAIHistory(edit.EditTarget) | |
| } | |
| transcriptMsg, err := oc.loadAIConversationMessage(ctx, portal, edit.EditTarget.ID, edit.EditTarget.MXID) | |
| if err != nil { | |
| oc.loggerForContext(ctx).Warn().Err(err).Msg("Failed to load edited conversation turn") | |
| return err | |
| } | |
| if transcriptMsg == nil { | |
| transcriptMsg = cloneMessageForAIHistory(edit.EditTarget) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/handlematrix.go` around lines 364 - 370, The code currently treats
any error from loadAIConversationMessage the same as a missing row and falls
back to cloneMessageForAIHistory, which can overwrite AI-only history; change
the logic in the block that calls loadAIConversationMessage so that if err !=
nil you return or propagate the error (do not set transcriptMsg to the clone),
and only use cloneMessageForAIHistory(edit.EditTarget) when err == nil and
transcriptMsg == nil (a true miss). Update the error handling around
loadAIConversationMessage in the function containing transcriptMsg so that Err
cases are handled separately from nil-result cases.
| promptContext, err := oc.buildPromptContextForPendingMessage(promptCtx, pending, body) | ||
| if err != nil { | ||
| return nil, sdk.MessageSendStatusError(err, "Couldn't prepare the message. Try again.", "", messageStatusForError, messageStatusReasonForError) | ||
| } | ||
| userMessage := &database.Message{ | ||
| ID: sdk.MatrixMessageID(eventID), | ||
| MXID: eventID, | ||
| Room: portal.PortalKey, | ||
| SenderID: humanUserID(oc.UserLogin.ID), | ||
| Metadata: &MessageMetadata{ | ||
| BaseMessageMetadata: sdk.BaseMessageMetadata{Role: "user", Body: body}, | ||
| }, | ||
| Timestamp: sdk.MatrixEventTimestamp(msg.Event), | ||
| } | ||
| if promptContext.CurrentTurnData.Role != "" { | ||
| userMessage.Metadata.(*MessageMetadata).CanonicalTurnData = promptContext.CurrentTurnData.ToMap() | ||
| } | ||
| queueItem := pendingQueueItem{ | ||
| pending: pending, | ||
| pending: pending, | ||
| acceptedMessages: []*database.Message{ | ||
| userMessage, | ||
| }, | ||
| messageID: string(eventID), | ||
| summaryLine: rawBody, | ||
| enqueuedAt: time.Now().UnixMilli(), | ||
| } | ||
| dbMsg, isPending := oc.dispatchOrQueue(promptCtx, pendingEvent, portal, meta, userMessage, queueItem, queueSettings, promptContext) | ||
| return &bridgev2.MatrixMessageResponse{ | ||
| DB: dbMsg, | ||
| Pending: isPending, | ||
| }, nil | ||
| if err = oc.dispatchOrQueueCore(promptCtx, pendingEvent, portal, meta, queueItem, queueSettings, promptContext); err != nil { | ||
| return nil, err | ||
| } | ||
| return &bridgev2.MatrixMessageResponse{DB: userMessage}, nil |
There was a problem hiding this comment.
Apply the same group-activation guard to media and text-file inputs.
Text messages are skipped at Lines 166-177 when the room requires a mention, but every dispatch path here proceeds regardless of mc.WasMentioned / groupActivation. In group rooms that means an unmentioned image, audio, video, or file can still trigger the assistant, and those branches also miss the buffered group-history context that the text path prepends.
Also applies to: 768-823, 928-968
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/handlematrix.go` around lines 668 - 697, The
media/audio/video/file handling paths need the same group-activation/mention
guard and group-history prepending as the text path: check mc.WasMentioned or
groupActivation before creating/dispatching queueItem and return/ignore when not
activated; when accepted, build promptContext via
oc.buildPromptContextForPendingMessage and prepend the buffered group-history to
promptContext (same logic used for text messages) before calling
oc.dispatchOrQueueCore (see symbols: mc.WasMentioned, groupActivation,
oc.buildPromptContextForPendingMessage, promptContext, dispatchOrQueueCore,
pendingQueueItem and the branches that construct userMessage/queueItem for
media); apply this change consistently in the other media handling blocks
referenced (the other dispatch branches).
1 participant
No description provided.