Use agentremote namespace; add typed login errors#76
Conversation
Rename ai-bridge identifiers and artifacts to the agentremote namespace and introduce typed login/response errors. Key changes: rename applyAIBridgeInfo -> applyAgentRemoteBridgeInfo, update protocol/step IDs and temp dir/user-agent prefixes, change MCP client name, and replace generic errors with agentremote.NewLoginRespError / WrapLoginRespError across AI, Codex, OpenClaw and dummybridge. Adds unit tests for AI, Codex and OpenClaw login flows and error mapping, plus new login test files. Also updates LICENSE copyright holder to "agentremote contributors".
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughRebranding from "ai-bridge" to "agentremote" across code, adding standardized login error helpers and converting many login/error paths to HTTP-aware Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bridges/openclaw/login.go (1)
367-370:⚠️ Potential issue | 🟠 MajorWrap transport errors in the RespError contract.
preflightGatewayLoginreturns errors fromConnect()(websocket failures likefmt.Errorf("dial gateway websocket: %w", err)) andListSessions()that are not*gatewayRPCErrorinstances. These pass throughmapOpenClawLoginErroras raw errors because theerrors.As(err, &rpcErr)check fails, breaking the typedbridgev2.RespErrorcontract on a very common failure path (network/transport issues).Suggested fix
func mapOpenClawLoginError(err error) error { var rpcErr *gatewayRPCError if !errors.As(err, &rpcErr) { - return err + return agentremote.WrapLoginRespError(err, http.StatusInternalServerError, "OPENCLAW", "GATEWAY_REQUEST_FAILED") }
🧹 Nitpick comments (2)
bridges/opencode/login.go (2)
157-157: Extract the completion step ID into a constant.Line 157 and Line 176 repeat the same renamed identifier. Since this PR is largely a namespace migration, keeping that value in one constant will make the next rename safer.
♻️ Proposed refactor
const ( FlowOpenCodeRemote = "opencode_remote" FlowOpenCodeManaged = "opencode_managed" openCodeLoginStepRemoteCredentials = "com.beeper.agentremote.opencode.enter_remote_credentials" openCodeLoginStepManagedCredentials = "com.beeper.agentremote.opencode.enter_managed_credentials" + openCodeLoginStepComplete = "com.beeper.agentremote.opencode.complete" defaultOpenCodeUsername = "opencode" )- "com.beeper.agentremote.opencode.complete", + openCodeLoginStepComplete,- "com.beeper.agentremote.opencode.complete", + openCodeLoginStepComplete,Also applies to: 176-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/opencode/login.go` at line 157, Extract the repeated literal "com.beeper.agentremote.opencode.complete" into a single constant (e.g., const OpencodeCompletionStep = "com.beeper.agentremote.opencode.complete") and replace the two inline occurrences with that constant; update any usages in this file (search for the string in bridges/opencode/login.go) to reference OpencodeCompletionStep so future renames only need to change the constant.
188-188: Please add focused coverage for the newOPENCODEreason codes.Line 188, Line 261, and Line 279-Line 295 change the user-visible login error contract. A small table test around invalid URL, bad binary path, missing default path, inaccessible default path, and non-directory default path would make these new mappings much harder to regress.
Also applies to: 261-261, 279-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/opencode/login.go` at line 188, Add focused table-driven tests that assert the new OPENCODE login error mappings produced via agentremote.WrapLoginRespError return the correct HTTP status and reason/code strings; cover scenarios: invalid URL (maps to "INVALID_URL"), bad binary path, missing default path, inaccessible default path, and non-directory default path. Locate the error-returning logic in the login handling function in login.go where WrapLoginRespError("OPENCODE", ...) is used and write tests that call that function (or its internal validation helpers) with inputs for each scenario, then assert the wrapped error contains the expected status code and the OPENCODE reason code string. Ensure tests are table-driven (name, input, expected status, expected reason/code) and include assertions on both the HTTP status and the agentremote error metadata to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bridges/codex/client.go`:
- Around line 361-363: The cleanup guard only checks filepath.Base(clean) for
"agentremote-codex-" and misses fallback dirs like "/tmp/agentremote-codex/…";
update the guard in the deletion loop to inspect all path components (split
clean by filepath.Separator) and allow deletion if any segment either equals
"agentremote-codex" or has the "agentremote-codex-" prefix (instead of only
checking filepath.Base(clean)). Modify the code that currently uses
filepath.Base(clean) and strings.HasPrefix to iterate segments and use
strings.HasPrefix(seg, "agentremote-codex-") || seg == "agentremote-codex" so
fallback paths created in bridges/codex/login.go are also purged.
In `@bridges/codex/login_test.go`:
- Around line 17-18: Replace the hard-coded "zsh" command in the test setup with
a guaranteed executable obtained via os.Executable() so exec.LookPath(cmd)
succeeds consistently; specifically update the test instance where Connector:
&CodexConnector{Config: Config{Codex: &CodexConfig{Command: "zsh"}}} is created
(and the similar instances at the other occurrences) to call os.Executable() and
use its returned path for CodexConfig.Command, handling the error from
os.Executable() in the test setup, so Start and SubmitUserInput exercise the
intended error paths reliably.
In `@bridges/codex/login.go`:
- Around line 503-506: The fast-timeout branch that returns errCodexTimedOut
when overallTimeout <= 0 should first cancel the pending login attempt to avoid
leaving the child codex process and CODEX_HOME behind; update the block around
overallTimeout/ cl.waitUntil to call cl.cancelLoginAttempt(true) (or the
appropriate cancelLoginAttempt method on the login object) before returning
errCodexTimedOut so the child process and temp directory are cleaned up.
In `@pkg/agents/toolpolicy/policy_test.go`:
- Around line 58-59: Add "beeper_send_feedback" to the test's assertions so the
GroupAgentRemote membership is fully covered: update the mustNotContain slice
(the negative assertion for group:openclaw) and the corresponding mustContain
slice (the positive assertion for group:agentremote) to include
"beeper_send_feedback", and ensure any other lists in the same test block
(around the mustContain/mustNotContain variables) are updated similarly so the
test fails if beeper_send_feedback is accidentally removed from GroupAgentRemote
or leaked into GroupOpenClaw; reference the mustNotContain and mustContain
variables and GroupAgentRemote in policy.go when making the change.
In `@pkg/agents/toolpolicy/policy.go`:
- Around line 38-39: The config rename removed the legacy key "group:ai-bridge",
causing ExpandToolGroups to not match older allow/deny configs; restore a
compatibility alias (e.g., add a GroupAIBridge constant or include
"group:ai-bridge" in the same map where GroupAgentRemote and GroupFS are
defined) so that ExpandToolGroups and ToolGroups will still expand legacy keys
until a migration/upgrader rewrites configs.
---
Nitpick comments:
In `@bridges/opencode/login.go`:
- Line 157: Extract the repeated literal
"com.beeper.agentremote.opencode.complete" into a single constant (e.g., const
OpencodeCompletionStep = "com.beeper.agentremote.opencode.complete") and replace
the two inline occurrences with that constant; update any usages in this file
(search for the string in bridges/opencode/login.go) to reference
OpencodeCompletionStep so future renames only need to change the constant.
- Line 188: Add focused table-driven tests that assert the new OPENCODE login
error mappings produced via agentremote.WrapLoginRespError return the correct
HTTP status and reason/code strings; cover scenarios: invalid URL (maps to
"INVALID_URL"), bad binary path, missing default path, inaccessible default
path, and non-directory default path. Locate the error-returning logic in the
login handling function in login.go where WrapLoginRespError("OPENCODE", ...) is
used and write tests that call that function (or its internal validation
helpers) with inputs for each scenario, then assert the wrapped error contains
the expected status code and the OPENCODE reason code string. Ensure tests are
table-driven (name, input, expected status, expected reason/code) and include
assertions on both the HTTP status and the agentremote error metadata to prevent
regressions.
🪄 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: 4ca02650-ba47-4f98-b895-195e0a344312
📒 Files selected for processing (38)
LICENSEbridges/ai/bridge_info.gobridges/ai/bridge_info_test.gobridges/ai/client.gobridges/ai/constructors.gobridges/ai/errors.gobridges/ai/errors_test.gobridges/ai/login.gobridges/ai/login_test.gobridges/ai/mcp_client.gobridges/ai/media_understanding_cli.gobridges/ai/media_understanding_runner.gobridges/ai/tool_approvals_policy.gobridges/ai/tool_policy_chain.gobridges/ai/tool_policy_chain_test.gobridges/ai/tools_beeper_feedback.gobridges/codex/client.gobridges/codex/constructors.gobridges/codex/login.gobridges/codex/login_test.gobridges/dummybridge/login.gobridges/openclaw/login.gobridges/openclaw/login_test.gobridges/opencode/connector.gobridges/opencode/login.gobridges/opencode/login_test.goconnector_builder.gohelpers.gohelpers_test.goidentifier_helpers.gologin_errors.gologin_helpers.gologin_helpers_test.gopkg/agents/toolpolicy/policy.gopkg/agents/toolpolicy/policy_test.gopkg/connector/integrations_example-config.yamlpkg/runtime/inbound_meta.gosdk/connector.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). (4)
- GitHub Check: build-agentremote-docker (arm64)
- 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/tool_policy_chain_test.gobridges/ai/tool_policy_chain.gobridges/ai/mcp_client.gobridges/ai/media_understanding_cli.gobridges/ai/bridge_info.gobridges/ai/bridge_info_test.gobridges/ai/constructors.gosdk/connector.gobridges/codex/constructors.gohelpers_test.gohelpers.goconnector_builder.go
🔇 Additional comments (37)
LICENSE (1)
3-4: Copyright attribution update looks correct.Line 3 and Line 4 align with the rebranding and preserve valid MIT license structure.
bridges/ai/mcp_client.go (1)
119-119: Namespace rename is correctly applied in MCP client metadata.
Name: "agentremote"aligns with the rebranding objective and keeps the MCP client identity consistent.bridges/ai/media_understanding_cli.go (1)
29-29: Namespace prefix update looks correct and safe.This temp-dir prefix rename is consistent with the PR’s
agentremoterebranding and keeps existing cleanup behavior intact.bridges/ai/media_understanding_runner.go (1)
603-603: Temp directory prefix rename is consistent.Good update to the
agentremotenamespace while preserving the existing temp-dir lifecycle and behavior.pkg/runtime/inbound_meta.go (2)
22-22: LGTM! Consistent rebranding.The text change from "ai-bridge" to "agentremote" aligns with the schema identifier update and maintains consistency in the user-facing documentation.
11-11: The schema identifier migration is complete — no references to the old identifier remain in the codebase.The change from
ai-bridge.inbound_meta.v1tocom.beeper.agentremote.inbound_meta.v1is a breaking change, but all internal code references have been updated. The new schema identifier is properly in place at line 11.bridges/ai/tool_approvals_policy.go (1)
23-23: Comment update looks goodLine 23 is a clean terminology update and keeps approval behavior unchanged.
bridges/ai/tools_beeper_feedback.go (1)
31-32: Branding rename is consistent and safeThe
agentremotetext prefix anduser_agentupdate align with the migration and preserve existing behavior.Also applies to: 49-49
bridges/ai/client.go (1)
471-471: OpenRouter referer rename looks correctLine 471 cleanly updates the app referer to the new namespace without affecting header construction logic.
helpers.go (1)
337-345: Helper rename is clean and behavior-preserving
ApplyAgentRemoteBridgeInfokeeps the same protocol/room-type mutation logic and matches the namespace migration.helpers_test.go (1)
32-35: Test rename/update is correctThe test now targets
ApplyAgentRemoteBridgeInfoand still validates the same behavior.bridges/ai/errors.go (1)
33-33: ErrCode namespace migration is consistentThe updated
COM.BEEPER.AGENTREMOTE.AI.*values are coherent and preserve the existing error payload shape.Also applies to: 38-38, 43-43
bridges/ai/errors_test.go (1)
12-22: Good regression guard for ErrCode namespaceThis test correctly protects the new
COM.BEEPER.AGENTREMOTE.AI.*error-code contract.identifier_helpers.go (1)
71-78: LGTM!The refactored
ValidateSingleLoginFlownow properly distinguishes between an invalid flow ID (returning the sentinelbridgev2.ErrInvalidLoginFlowID) and a disabled flow (returning a typedRespErrorwith HTTP 403). This separation improves error handling clarity for consumers.bridges/opencode/login_test.go (2)
58-68: LGTM!Good test coverage for the invalid flow rejection path, correctly asserting
bridgev2.ErrInvalidLoginFlowIDusingerrors.Is.
70-106: LGTM!Comprehensive test coverage for the error mapping in
buildRemoteInstancesandresolveManagedOpenCodeDirectory. The tests properly validate the typedRespErrorresponses and their error codes.login_errors.go (1)
1-52: LGTM!Well-designed error code generation utilities. The
sanitizeLoginErrorCodePartfunction properly normalizes input by uppercasing and converting various separators to underscores. TheLoginErrorCodefunction correctly filters empty parts, ensuring clean error codes. BothNewLoginRespErrorandWrapLoginRespErrorprovide consistent error construction.login_helpers.go (1)
13-21: LGTM!The
ValidateLoginStatefunction now returns typedRespErrorvalues with HTTP 500 status for internal configuration issues. This aligns well with the new error handling pattern and provides clear, structured error codes for debugging.bridges/dummybridge/login.go (2)
14-14: LGTM!The step ID constants are correctly migrated to the
com.beeper.agentremote.dummybridge.*namespace, consistent with the PR's broader renaming effort.Also applies to: 76-76
79-81: LGTM!The error handling now uses
agentremote.WrapLoginRespErrorto return a typedRespErrorwith a structured error code. Thefmt.Errorfwrapper preserves the original error context in the message.login_helpers_test.go (1)
10-47: LGTM!Comprehensive unit tests that validate both
ValidateLoginStateandValidateSingleLoginFlowreturn the expected typed errors with correct HTTP status codes and error codes. The use oferrors.Asanderrors.Isfollows Go best practices for error assertion.bridges/ai/login.go (4)
43-47: LGTM!Well-defined package-level error variables using
agentremote.NewLoginRespError. This pattern allows for reusable error definitions with appropriate HTTP status codes and structured error codes.
97-97: LGTM!Correctly returns
bridgev2.ErrInvalidLoginFlowIDfor unrecognized flow IDs in bothStartandSubmitUserInput, replacing the previous formatted error message.Also applies to: 165-165
232-232: LGTM!Step IDs successfully migrated from
io.ai-bridge.openai.*tocom.beeper.agentremote.openai.*namespace.Also applies to: 307-307
240-299: LGTM!The
finishLoginmethod now uses typed errors consistently:
- Returns pre-defined errors for missing user context and relogin metadata
- Creates new
RespErrorfor provider mismatch with a descriptive message- Uses
WrapLoginRespErrorfor clone and create failures, preserving the underlying error contextThe HTTP status codes are semantically appropriate (400 for client errors, 500 for server errors).
bridges/ai/login_test.go (1)
13-68: LGTM!Comprehensive unit tests covering the key error paths in the
OpenAILoginflow:
- Invalid flow ID returns
bridgev2.ErrInvalidLoginFlowID- Relogin target mismatch and managed Beeper relogin return appropriate typed errors
- Provider mismatch during finish returns
PROVIDER_MISMATCHerrorThe tests properly validate both error types and error codes.
bridges/ai/tool_policy_chain_test.go (1)
9-9: Namespace comment update is correct.Comment wording now matches the
agentremotenaming, and test behavior remains unchanged.bridges/ai/tool_policy_chain.go (1)
103-103: Tool-policy comment rename looks good.The updated
agentremotewording is consistent with the policy-group migration and does not alter behavior.connector_builder.go (1)
140-140: Bridge-info helper rename is correctly wired.Guard conditions are unchanged, and the new helper name aligns with the namespace migration.
sdk/connector.go (1)
141-141: SDK bridge-info call update is consistent.The call-site now uses the renamed API while keeping the same safety checks and semantics.
pkg/connector/integrations_example-config.yaml (1)
216-217: Example tool-policy group rename is accurate.The sample now matches the
group:agentremotenaming used by current tool-policy groups.bridges/opencode/connector.go (1)
89-89: Typed disabled-login response is a solid improvement.Returning a structured login response error here is consistent with the new error model and keeps behavior clear for clients.
bridges/ai/bridge_info.go (1)
30-35: Bridge-info helper rename is correctly applied.The local helper and downstream call now use
agentremotenaming with unchanged logic flow.bridges/ai/constructors.go (1)
75-75: Constructor call-site rename is consistent.
FillBridgeInfonow routes through the renamed helper correctly, preserving existing behavior.bridges/opencode/login.go (3)
6-6: TypedOPENCODEerrors and renamed step IDs look consistent.The
net/httpimport, the newagentremote.NewLoginRespError(...)sentinels, and thecom.beeper.agentremote.*step IDs fit together cleanly in this file.Also applies to: 23-24, 31-32
111-111: Using the shared invalid-flow sentinel here is a good change.Returning
bridgev2.ErrInvalidLoginFlowIDat Line 111 and Line 132 keepsOpenCodeLogin.StartandOpenCodeLogin.SubmitUserInputaligned with shared upstream error handling.Also applies to: 132-132
161-161: Wrapping create/update failures as typed 500s is a solid improvement.Line 161 and Line 180 now return a stable login-response shape instead of bubbling raw persistence or bridge errors to callers.
Also applies to: 180-180
Introduce a managed-path check for Codex temp roots (isManagedCodexTempDirPath) and use it when purging temp dirs; add unit tests for that helper. Ensure Codex login cancels pending attempts on immediate timeout and expand tests to run a helper process to assert cancellation and codexHome cleanup. Refactor OpenCode login: replace hardcoded step name with a constant, make the default managed directory resolver overridable for tests, and consolidate validation error mapping tests into a table-driven test. Factor AgentRemote extra tools into a single slice and add GroupAIBridge as an alias of GroupAgentRemote; update policy tests to include the new tool and verify the alias mapping.
1 participant
Rename ai-bridge identifiers and artifacts to the agentremote namespace and introduce typed login/response errors. Key changes: rename applyAIBridgeInfo -> applyAgentRemoteBridgeInfo, update protocol/step IDs and temp dir/user-agent prefixes, change MCP client name, and replace generic errors with agentremote.NewLoginRespError / WrapLoginRespError across AI, Codex, OpenClaw and dummybridge. Adds unit tests for AI, Codex and OpenClaw login flows and error mapping, plus new login test files. Also updates LICENSE copyright holder to "agentremote contributors".