Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR implements agent mode gating for the AI bridge, introducing a new agent bridge instance with agents enabled while disabling agents in the existing AI instance. Configuration options allow enabling/disabling agent functionality, with permission checks preventing agent-related operations when disabled. Bridge infrastructure updated to support the new agent instance type. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
bridges/ai/handleai_test.go (1)
164-169: Assert the disabled-agent sentinel, not just the message.This still passes if
dispatchInternalMessagestarts returning a different error type with the same text. Please also pinerrors.Is(err, errAgentsDisabled)so the contract used bywriteAgentErrorstays covered.Suggested test tightening
import ( "encoding/base64" + "errors" "strings" "testing" @@ _, _, err := oc.dispatchInternalMessage(t.Context(), portal, agentModeTestMeta("beeper"), "hello", "test", false) if err == nil { t.Fatal("expected dispatchInternalMessage to fail") } + if !errors.Is(err, errAgentsDisabled) { + t.Fatalf("expected errAgentsDisabled, got %v", err) + } if !strings.Contains(strings.ToLower(err.Error()), "agents are disabled") { t.Fatalf("unexpected error: %v", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/handleai_test.go` around lines 164 - 169, The test should assert the sentinel error rather than only matching text: after calling dispatchInternalMessage in the failing case, add an assertion using errors.Is(err, errAgentsDisabled) (importing errors if needed) to ensure the returned error is the expected sentinel; keep the existing string check if you want to preserve message validation but make errors.Is(err, errAgentsDisabled) the primary contractual check tied to writeAgentError and dispatchInternalMessage.bridges/ai/provisioning_agents_test.go (1)
37-40: Make this sentinel test exercise wrapped errors.
errors.Is(errAgentsDisabled, errAgentsDisabled)only proves the standard library works. The behavior worth pinning here is that a wrappederrAgentsDisabledstill matches and still returns 403 fromwriteAgentError.Suggested replacement
import ( "context" "errors" + "fmt" "net/http/httptest" "strings" "testing" ) @@ func TestWriteAgentErrorDisabledMatchesSentinel(t *testing.T) { - if !errors.Is(errAgentsDisabled, errAgentsDisabled) { - t.Fatal("expected sentinel error to match itself") - } + wrapped := fmt.Errorf("wrapped: %w", errAgentsDisabled) + if !errors.Is(wrapped, errAgentsDisabled) { + t.Fatal("expected wrapped sentinel error to match errAgentsDisabled") + } + + rec := httptest.NewRecorder() + writeAgentError(rec, wrapped) + if rec.Code != 403 { + t.Fatalf("expected 403 for wrapped disabled error, got %d", rec.Code) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/provisioning_agents_test.go` around lines 37 - 40, Replace the trivial self-comparison in TestWriteAgentErrorDisabledMatchesSentinel with a test that wraps errAgentsDisabled and verifies both errors.Is and writeAgentError behavior: create a wrapped error (e.g., fmt.Errorf("...%w", errAgentsDisabled)), assert errors.Is(wrapped, errAgentsDisabled), then call writeAgentError with an httptest.ResponseRecorder and the wrapped error and assert the recorder.Code equals http.StatusForbidden (403). Ensure you reference the symbols TestWriteAgentErrorDisabledMatchesSentinel, errAgentsDisabled, errors.Is, writeAgentError, httptest.ResponseRecorder, and http.StatusForbidden when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bridges/ai/handlematrix.go`:
- Around line 38-41: The new agent gate (ensureAgentTargetAllowed) is only
applied to fresh inbound messages but can be bypassed via edit regeneration; add
the same check at the start of HandleMatrixEdit (and/or inside
regenerateFromEdit) so edits that would re-trigger agent execution first call
oc.ensureAgentTargetAllowed(meta) and, on error, call oc.sendSystemNotice(ctx,
portal, err.Error()) and return nil, agentremote.UnsupportedMessageStatus(err)
to mirror the original gate behavior; reference the existing
ensureAgentTargetAllowed, HandleMatrixEdit, regenerateFromEdit,
sendSystemNotice, and agentremote.UnsupportedMessageStatus symbols to locate and
apply the guard.
In `@bridges/ai/provisioning_agents_test.go`:
- Around line 11-23: The test TestListAgentsForResponseDisabledReturnsEmpty
should assert that the returned items slice is initialized (not nil) in addition
to being empty; after calling listAgentsForResponse in that test, add an
assertion that items != nil (e.g., use t.Fatalf or t.Fatalf-style check) before
or alongside the existing len(items) check to ensure the response encodes an
empty array rather than null.
---
Nitpick comments:
In `@bridges/ai/handleai_test.go`:
- Around line 164-169: The test should assert the sentinel error rather than
only matching text: after calling dispatchInternalMessage in the failing case,
add an assertion using errors.Is(err, errAgentsDisabled) (importing errors if
needed) to ensure the returned error is the expected sentinel; keep the existing
string check if you want to preserve message validation but make errors.Is(err,
errAgentsDisabled) the primary contractual check tied to writeAgentError and
dispatchInternalMessage.
In `@bridges/ai/provisioning_agents_test.go`:
- Around line 37-40: Replace the trivial self-comparison in
TestWriteAgentErrorDisabledMatchesSentinel with a test that wraps
errAgentsDisabled and verifies both errors.Is and writeAgentError behavior:
create a wrapped error (e.g., fmt.Errorf("...%w", errAgentsDisabled)), assert
errors.Is(wrapped, errAgentsDisabled), then call writeAgentError with an
httptest.ResponseRecorder and the wrapped error and assert the recorder.Code
equals http.StatusForbidden (403). Ensure you reference the symbols
TestWriteAgentErrorDisabledMatchesSentinel, errAgentsDisabled, errors.Is,
writeAgentError, httptest.ResponseRecorder, and http.StatusForbidden when making
the change.
🪄 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: 14b9ad4d-8ae4-420d-adee-26eb0060dd2a
📒 Files selected for processing (28)
bridges.manifest.ymlbridges/ai/agent_activity.gobridges/ai/agent_mode.gobridges/ai/chat.gobridges/ai/chat_login_redirect_test.gobridges/ai/config_test.gobridges/ai/connector.gobridges/ai/default_chat_test.gobridges/ai/handleai.gobridges/ai/handleai_test.gobridges/ai/handlematrix.gobridges/ai/heartbeat_config.gobridges/ai/heartbeat_config_test.gobridges/ai/heartbeat_execute.gobridges/ai/integrations_config.gobridges/ai/integrations_example-config.yamlbridges/ai/internal_dispatch.gobridges/ai/provisioning.gobridges/ai/provisioning_agents_test.gobridges/ai/sdk_agent_catalog.gobridges/ai/sdk_agent_catalog_test.gobridges/ai/sessions_tools.gocmd/agentremote/bridges.gocmd/agentremote/commands.gocmd/agentremote/main.gocmd/agentremote/run_bridge.gocmd/internal/bridgeentry/bridgeentry.goconfig.example.yaml
📜 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-agentremote-docker (arm64)
- GitHub Check: build-agentremote-docker (amd64)
- GitHub Check: build-docker
- GitHub Check: Lint
- GitHub Check: build-docker
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-16T09:01:24.464Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 71
File: bridges/ai/connector.go:53-63
Timestamp: 2026-03-16T09:01:24.464Z
Learning: In package ai, the AI connector’s configuration type (Config) defines Bridge as a value field of type BridgeConfig (not a pointer). Therefore, accessing oc.Config.Bridge.CommandPrefix in OpenAIConnector.applyRuntimeDefaults (bridges/ai/connector.go) is safe and does not require a nil-check.
Applied to files:
bridges/ai/sdk_agent_catalog.gobridges/ai/integrations_config.gocmd/agentremote/run_bridge.gocmd/internal/bridgeentry/bridgeentry.gobridges/ai/handleai.gobridges/ai/connector.gobridges/ai/handleai_test.gocmd/agentremote/main.gocmd/agentremote/bridges.gobridges/ai/agent_mode.gobridges/ai/chat.go
🔇 Additional comments (31)
cmd/internal/bridgeentry/bridgeentry.go (1)
27-32: Looks good — newagentbridge definition is consistent and isolated.
Name,Port, andDBNamealign with the new preset wiring and avoid collisions with existing bridge entries.cmd/agentremote/run_bridge.go (1)
26-28: Good parity update for AI and Agent presets.Applying
PortalEventBuffer = 0to both bridge types keeps runtime behavior consistent betweenrun aiandrun agent.bridges/ai/heartbeat_config_test.go (1)
47-60: Nice targeted coverage for disabled-agents heartbeat behavior.This test closes an important regression gap by asserting both
isHeartbeatEnabledForAgentandresolveHeartbeatAgentsbehavior when agents are explicitly disabled.config.example.yaml (1)
202-202: Good config-template addition for discoverability.Including
agents.enabledin the example config makes the mode split explicit for operators.bridges/ai/internal_dispatch.go (1)
34-36: Good early guard in internal dispatch path.Blocking before trimming, context build, and DB writes prevents unintended execution and side effects in disabled-agent mode.
bridges/ai/integrations_example-config.yaml (1)
225-225: LGTM — helpful example update.Adding
agents.enabledto the integration example keeps the docs aligned with runtime behavior.bridges/ai/heartbeat_execute.go (1)
33-34: Behavioral gate is correctly enforced in heartbeat agent resolution.Returning an empty list when agents are disabled cleanly prevents heartbeat runs from being scheduled in that mode.
bridges/ai/config_test.go (1)
81-99: Good coverage for default vs explicit-disable behavior.These tests clearly lock in the intended semantics for
agentsEnabled()and reduce regression risk around pointer/nil config handling.bridges/ai/sessions_tools.go (1)
75-76: Nice unification of portal visibility checks.Using
oc.shouldExcludeVisiblePortal(...)in both paths keeps session listing and label resolution aligned with the new agent-mode gate.Also applies to: 544-545
bridges/ai/sdk_agent_catalog.go (1)
25-27: Consistent gating across catalog entry points.The early exits make disabled-agent mode predictable for default, listing, and resolution paths.
Also applies to: 40-42, 69-71
bridges/ai/handleai.go (1)
265-267: Auto-greeting blocking is correctly enforced.The pre-check and loop-time re-check close the gap for agent-disabled state transitions and prevent unintended greeting dispatches.
Also applies to: 304-307
bridges/ai/integrations_config.go (1)
110-110: Config schema and upgrader are in sync.Adding
agents.enabledto bothAgentsConfigandupgradeConfigis the right migration-safe implementation.Also applies to: 570-570
bridges/ai/agent_activity.go (1)
24-26: Good alignment with agent-mode-aware routing rules.These changes correctly stop blocked-agent activity tracking and ensure default-chat selection uses the same exclusion policy.
Also applies to: 99-99, 104-104
bridges/ai/connector.go (1)
78-80: Validation and error semantics look cleaner.Rejecting agent IDs when disabled and using
ErrInvalidLoginFlowIDmakes behavior both safer and more uniform.Also applies to: 103-103
bridges/ai/heartbeat_config.go (1)
10-11: Heartbeat gating is now consistently enforced.The added checks correctly prevent explicit/default heartbeat enablement when
agents.enabledis false.Also applies to: 82-84
cmd/agentremote/main.go (1)
441-441: Nice: the alias resolves through the registry everywhere it matters.Foreground exec, example-config generation, background start, and remote registration all now go through
runtimeBridgeType(...)/remoteBridgeType(...), which keepsagentas a thin preset overaiinstead of a forked code path.Also applies to: 1156-1156, 1190-1190, 1215-1215
bridges/ai/provisioning.go (3)
225-236: LGTM! Clean sentinel error handling.The
errAgentsDisabledcase is correctly placed first in the switch to ensure it's checked before other agent-related errors. Usingerrors.Is()ensures proper sentinel comparison even if the error is wrapped.
330-333: LGTM! Returning empty slice instead of nil is good for JSON serialization.When agents are disabled, returning
[]*AgentDefinitionContent{}ensures the JSON response contains"agents": []rather than"agents": null, providing a consistent API contract.
370-373: LGTM! Consistent guard placement across agent CRUD handlers.All agent endpoints check
agentsEnabled()early (before request parsing), returning a 403 Forbidden via the sentinel error. This is efficient and consistent.Also applies to: 388-391, 420-423, 458-461
cmd/agentremote/bridges.go (2)
22-40: LGTM! Separate bridge instances with distinct config overrides.Both "ai" and "agent" entries call
aibridge.NewAIConnector()which creates independent instances (confirmed by context snippet showing it allocates new struct and maps each time). TheConfigOverridesapproach cleanly separates the agent-enabled vs agent-disabled configurations.
63-77: LGTM! Defensive fallback in helper functions.Both
remoteBridgeTypeandruntimeBridgeTypecorrectly fall back tolocalBridgeTypewhen the registry entry doesn't exist or the field is empty. This ensures graceful degradation for unknown bridge types.bridges/ai/agent_mode.go (4)
1-9: LGTM! Well-defined sentinel error for feature gating.Using
errors.New()creates a comparable sentinel that works correctly witherrors.Is(). The error message is descriptive and user-facing appropriate.
11-34: LGTM! Nil-safe delegation chain with safe defaults.The
agentsEnabled()methods at each layer (Config → OpenAIConnector → AIClient) correctly default totruewhen any part of the chain is nil, maintaining backwards compatibility. The pointer dereference at line 15 is safe due to the nil checks.
36-52: LGTM! Clean predicate composition.
agentTargetBlockedcorrectly combines the three conditions (non-nil client, agents disabled, portal has agent).shouldExcludeVisiblePortalcleanly composes model-level exclusion with agent blocking.
58-79: LGTM! Correct portal selection with slug-based priority.The
chooseDefaultChatPortallogic correctly uses thehaveSlugflag to gate comparisons againstminIdx, avoiding any issues with the zero-initializedminIdx. Portals with the lowest slug index are preferred, with a fallback to the first eligible non-slug portal.bridges/ai/chat.go (6)
412-414: LGTM! Early guard for agent ghost chat creation.When the ghost ID parses as an agent, checking
agentsEnabled()before proceeding prevents agent chat creation when the feature is disabled. The error is correctly wrapped withmautrix.MForbidden.
431-433: LGTM! Entry guard for resolveAgentIdentifier.Placing the guard at the function entry ensures all code paths that resolve agent identifiers are protected when agents are disabled.
739-741: LGTM! Consistent guards in resolveNewChatTarget.Both explicit (
!ai new agent <id>) and implicit (current room has agent) agent-based chat creation paths are correctly blocked when agents are disabled.Also applies to: 765-767
1108-1108: LGTM! Refactored to use instance methods for default chat selection.The default chat candidate/selection logic now uses
oc.isDefaultChatCandidate()andoc.chooseDefaultChatPortal()instance methods, which internally respect theagentsEnabled()state viashouldExcludeVisiblePortal.Also applies to: 1136-1136, 1150-1150
1191-1210: LGTM! Conditional agent configuration in ensureDefaultChat.When agents are enabled, the Beeper AI agent is configured as the chat target. When disabled, the portal is created as a simple model chat (using
modelUserIDset byinitPortalForChatat line 678). This correctly implements the simple/agent mode split.
1226-1228: LGTM! Guard in ensureExistingChatPortalReady.Using
oc.isDefaultChatCandidate()ensures hidden or agent-blocked portals are rejected consistently.
| if err := oc.ensureAgentTargetAllowed(meta); err != nil { | ||
| oc.sendSystemNotice(ctx, portal, err.Error()) | ||
| return nil, agentremote.UnsupportedMessageStatus(err) | ||
| } |
There was a problem hiding this comment.
Agent-disabled policy can still be bypassed through edit regeneration.
The new gate at Line 38 protects fresh inbound messages, but HandleMatrixEdit/regenerateFromEdit can still trigger execution in an existing agent-target room when agents are disabled. That leaves a policy bypass path.
🔧 Suggested fix (apply in HandleMatrixEdit)
func (oc *AIClient) HandleMatrixEdit(ctx context.Context, edit *bridgev2.MatrixEdit) error {
@@
portal := edit.Portal
if portal == nil {
return errors.New("portal is nil")
}
meta := portalMeta(portal)
+ if err := oc.ensureAgentTargetAllowed(meta); err != nil {
+ oc.sendSystemNotice(ctx, portal, err.Error())
+ return agentremote.UnsupportedMessageStatus(err)
+ }
@@
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/handlematrix.go` around lines 38 - 41, The new agent gate
(ensureAgentTargetAllowed) is only applied to fresh inbound messages but can be
bypassed via edit regeneration; add the same check at the start of
HandleMatrixEdit (and/or inside regenerateFromEdit) so edits that would
re-trigger agent execution first call oc.ensureAgentTargetAllowed(meta) and, on
error, call oc.sendSystemNotice(ctx, portal, err.Error()) and return nil,
agentremote.UnsupportedMessageStatus(err) to mirror the original gate behavior;
reference the existing ensureAgentTargetAllowed, HandleMatrixEdit,
regenerateFromEdit, sendSystemNotice, and agentremote.UnsupportedMessageStatus
symbols to locate and apply the guard.
There was a problem hiding this comment.
its ok for now if existing rooms have this issue becuase there are no existing rooms right now
There was a problem hiding this comment.
@batuhan Understood — since there are no existing agent-target rooms today, the edit-regeneration path poses no practical risk. Happy to revisit this guard in a future PR if rooms start being created.
| func TestListAgentsForResponseDisabledReturnsEmpty(t *testing.T) { | ||
| disabled := false | ||
| client := newCatalogTestClient() | ||
| client.connector.Config.Agents = &AgentsConfig{Enabled: &disabled} | ||
|
|
||
| items, err := listAgentsForResponse(context.Background(), client, NewAgentStoreAdapter(client)) | ||
| if err != nil { | ||
| t.Fatalf("listAgentsForResponse returned error: %v", err) | ||
| } | ||
| if len(items) != 0 { | ||
| t.Fatalf("expected no agents when disabled, got %#v", items) | ||
| } | ||
| } |
There was a problem hiding this comment.
Assert that items is initialized, not just empty.
len(items) == 0 also passes for a nil slice, so this test won’t catch a regression from [] to null in the JSON response. Please assert items != nil as well.
Suggested assertion
items, err := listAgentsForResponse(context.Background(), client, NewAgentStoreAdapter(client))
if err != nil {
t.Fatalf("listAgentsForResponse returned error: %v", err)
}
+ if items == nil {
+ t.Fatal("expected initialized empty slice, got nil")
+ }
if len(items) != 0 {
t.Fatalf("expected no agents when disabled, got %#v", items)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestListAgentsForResponseDisabledReturnsEmpty(t *testing.T) { | |
| disabled := false | |
| client := newCatalogTestClient() | |
| client.connector.Config.Agents = &AgentsConfig{Enabled: &disabled} | |
| items, err := listAgentsForResponse(context.Background(), client, NewAgentStoreAdapter(client)) | |
| if err != nil { | |
| t.Fatalf("listAgentsForResponse returned error: %v", err) | |
| } | |
| if len(items) != 0 { | |
| t.Fatalf("expected no agents when disabled, got %#v", items) | |
| } | |
| } | |
| func TestListAgentsForResponseDisabledReturnsEmpty(t *testing.T) { | |
| disabled := false | |
| client := newCatalogTestClient() | |
| client.connector.Config.Agents = &AgentsConfig{Enabled: &disabled} | |
| items, err := listAgentsForResponse(context.Background(), client, NewAgentStoreAdapter(client)) | |
| if err != nil { | |
| t.Fatalf("listAgentsForResponse returned error: %v", err) | |
| } | |
| if items == nil { | |
| t.Fatal("expected initialized empty slice, got nil") | |
| } | |
| if len(items) != 0 { | |
| t.Fatalf("expected no agents when disabled, got %#v", items) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/ai/provisioning_agents_test.go` around lines 11 - 23, The test
TestListAgentsForResponseDisabledReturnsEmpty should assert that the returned
items slice is initialized (not nil) in addition to being empty; after calling
listAgentsForResponse in that test, add an assertion that items != nil (e.g.,
use t.Fatalf or t.Fatalf-style check) before or alongside the existing
len(items) check to ensure the response encodes an empty array rather than null.
1 participant
Summary
agents.enabledgating to the AI bridge so simple model chats and agent chats can be globally splitagentremote run agentas an AI-bridge preset/alias with agent mode enabled, whileagentremote run aidefaults to simple modeTesting