Merged
Conversation
Replace ephemeral+debounced edit streaming with a unified StreamTransport-based flow and embed stream descriptors in placeholder messages. Key changes: - Introduce StreamTransport APIs and StreamDescriptor flow: Turn and StreamSession now request a shared stream transport, build descriptors, start/finish streams, and publish deltas via Publish/Start/Finish calls. - Propagate com.beeper.stream descriptor into initial/placeholder messages and message parts so streams are discoverable by clients. - Consolidate stream event shape: change internal ephemeral stream event type to com.beeper.llm and adjust envelope building and txn handling accordingly. - Remove legacy debounced_edit/fallback logic and related helpers/tests; simplify StreamSession to manage lifecycle, descriptor caching, and publishing. - Update sdk/turn.go to provide StreamDescriptor, hook up stream transport resolution, and start sessions when sending the initial message. - bridges/ai: pass streamDescriptor into initial stream messages and use shared Streams transport instead of ephemeral/debounced hooks. Additional changes: - Codex: always enable experimental API during initialization, switch sandbox to a simple mode string and adjust policy generation; add unit tests for sandbox behavior. - Remove SendDebouncedStreamEdit helper and obsolete code paths. - Update package imports and add a local replace in go.mod (developer local maunium replacement). These changes simplify streaming logic by centralizing responsibility in the StreamTransport implementation and removing complex fallback/debounce machinery.
Introduce a new dummybridge connector (agent, bridge, config, connector, login, metadata, tests, and example config) and register it in bridges.manifest.yml. Update README commands for local bridge deletion. Refactor AI/Codex streaming and finalization: remove older m.relates_to builder and tests, switch to SDK-driven placeholder messages and FinalEditPayload injection, remove reliance on network message IDs, and add streamingState helpers (currentTurnID, currentReplyTargetEventID). Update related tests and plumbing to use the SDK patterns and simplify stream/portal handling.
Add buffering and async start behavior for streaming parts: StreamSession now enqueues pending parts, can flush them (FlushPending), and publishes buffered parts once a target event ID is resolved. Turn gains an emitPart wrapper, async ensureStreamStarted logic (awaitStreamStart, ensureStreamStartedAsync), stream-start logging, flushPendingStream calls on End/Abort/Error, and room/log helpers. Placeholder messages now include a default Beeper AI payload. Writer.Start now uses ready() instead of valid(). Added unit tests and a test stream transport to validate buffering, async start, placeholder AI seeding, and lazy placeholder send behavior.
Multiple coordinated changes: - SDK: add final_edit helpers (BuildCompactFinalUIMessage, BuildDefaultFinalEditTopLevelExtra, hasMeaningfulFinalUIMessage) to centralize final-edit construction. - Turn/sdk: refactor Turn lifecycle and placeholder handling (applyPlaceholderSendResult), add suppressFinalEdit, thread support for final edits, defaultFinalEdit generation, idle timeout scaffolding, safer concurrency for Start/End flows, and small API adjustments (InitialEventID/NetworkMessageID locking). - Bridges: use SDK helpers in AI bridge finalization and UI compacting; set thread/reply IDs when preparing streaming turns; minor logging nil-check. - OpenClaw: avoid holding StreamMu during turn creation and ensure single state turn set atomically to prevent races. - OpenCode: ensure streamState is returned by mustStreamWriter before using metadata. - DummyBridge: improve lorem text generation (sentence corpus, rng usage, trimming), fix context usage to use turn context, validate option parsing changes, and add unit tests for lorem generator. - Codex: consolidate repetitive test setup into newApprovalTestFixture and simplify tests; minor initializer signature change. - CLI: provide dynamic, sorted available bridge names in error messages. - Docs: note room_id and event_id in encrypted stream envelope routing. Also include various small cleanups: import adjustments, helper renames, and refactors to reduce duplication and improve testability.
Introduce sdk.StreamPartState and sdk.UIStateReplayer to centralize stream part state tracking and UI replay logic. Replace ad-hoc visible/accumulated/timestamp fields across OpenClaw and OpenCode bridges with the new StreamPartState; use StreamPartState.ApplyPart to process parts and propagate timestamps. Add UIStateReplayer to encapsulate streamui.ApplyChunk calls and replace many direct ApplyChunk usages with higher-level replay methods (Text, Reasoning, ToolInput, ToolOutput, Artifact, StepStart/Finish, etc.). Also add login_helpers.ValidateLoginState to share login precondition checks, update part application to respect terminal event handling (PartApplyOptions.HandleTerminalEvents) and treat "error" finish reason as EndWithError, add withFinalEditFinishReason and maps usage to final_edit, and wire citations when emitting artifacts in opencode. Update tests accordingly.
Introduce a generic agentremote.StreamTurnHost to centralize thread-safe stream state management and drain/abort logic. Replace per-bridge streamStates maps and related locking/abort helpers in OpenClaw and OpenCode with StreamTurnHost usage, and update tests accordingly. Remove a number of noisy/demo debug logging calls and the old sdk.Turn log helpers, simplify some session logging, and adjust dummybridge signatures to remove unused params. Overall this unifies stream lifecycle handling and cleans up logging and state-management code paths.
Refactor streaming API to use bridgev2.BeeperStreamPublisher instead of StreamTransport across the codebase. Renamed and adapted functions: SetStreamPublisherFunc / GetStreamPublisher, NewDescriptor/Register/Publish/Unregister on publisher, and updated StreamSession methods to call publisher APIs. Adjusted stream delta key logic to prefer the descriptor.Type (so custom stream types produce the correct "<type>.deltas" key). Added a small connector wrapper (pkg/connector) exposing ai.Config and NewAIConnector. Updated tests to match the new publisher interface and added cases for custom stream types and buffering behavior. Also includes dependency version bumps in go.mod/go.sum.
Rename the `expiry` descriptor field to `expiry_ms` in the streaming MSC and spec to make the units (milliseconds) explicit and update related examples and tables. Also adjust wording from "Shared approval manager + approval-decision parser" to "Shared approval manager and reaction handling" and add a rule forbidding legacy timeline approval decision payloads in favor of owner reactions on the canonical approval notice. Changes applied to docs/matrix-ai-matrix-spec-v1.md and docs/msc/com.beeper.mscXXXX-streaming.md.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a demo dummybridge and CLI entry; replaces debounced/ephemeral AI streaming with a placeholder + publisher-backed stream model; introduces SDK stream state and UI replayer primitives; removes debounced/fallback machinery; and applies widespread refactors and tests across bridges, turns, sessions, gateway clients, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Bridge as Bridge (AI/OpenClaw/OpenCode)
participant SDK as SDK (Turn/Session)
participant Publisher as StreamPublisher
participant Matrix as Matrix/Homeserver
Client->>Bridge: send user message / request
Bridge->>SDK: create Turn, SetPlaceholderMessagePayload
SDK->>Matrix: send placeholder m.room.message (com.beeper.stream descriptor)
Matrix-->>SDK: returns placeholder event_id
SDK->>Publisher: Register / NewDescriptor (room + event_id)
loop stream deltas
Bridge->>SDK: EmitPart (text/reasoning/tool...)
SDK->>SDK: ApplyPart -> StreamPartState
SDK->>Publisher: Publish delta (to_device com.beeper.stream.update)
Publisher->>Matrix: to_device update to subscriber device(s)
end
Bridge->>SDK: End turn
SDK->>SDK: buildFinalEditPayload
SDK->>Matrix: send m.replace edit (finalized content)
Matrix-->>SDK: edit confirmed
SDK->>Publisher: Unregister
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (4)
bridges/dummybridge/agent.go-3-6 (1)
3-6:⚠️ Potential issue | 🟡 MinorFix import ordering to resolve pipeline failure.
The
go-imports-repopre-commit hook failed. The imports should follow standard Go import ordering conventions (standard library, then external packages typically grouped and sorted).Proposed fix
import ( + "maunium.net/go/mautrix/bridgev2/networkid" + bridgesdk "github.com/beeper/agentremote/sdk" - "maunium.net/go/mautrix/bridgev2/networkid" )Note: Run
goimportsor check repository conventions for the expected grouping (third-party vs internal modules).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/dummybridge/agent.go` around lines 3 - 6, The import block in bridges/dummybridge/agent.go is misordered and failing the go-imports-repo pre-commit hook; reorder the imports to follow Go conventions (standard library first, then external third-party modules, then internal/repo packages), e.g. ensure any standard library packages (if present) are listed before third-party packages and that third-party imports like "maunium.net/go/mautrix/bridgev2/networkid" and internal imports like bridgesdk "github.com/beeper/agentremote/sdk" are grouped and sorted; run goimports (or go fmt) to automatically fix formatting and grouping and re-run the pipeline.bridges/ai/streaming_init.go-3-14 (1)
3-14:⚠️ Potential issue | 🟡 MinorFix the import block before merging.
go-imports-repois already failing on this file, so the new import ordering needs to be normalized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/streaming_init.go` around lines 3 - 14, Reorder and normalize the import block so goimports/go fmt passes: group standard library imports (context) first, then a blank line, then third-party modules grouped and sorted (e.g., github.com/rs/zerolog, github.com/openai/openai-go/v3, github.com/beeper/agentremote, bridgesdk "github.com/beeper/agentremote/sdk", maunium.net/... packages), remove any unused imports, and run goimports (or go fmt) to apply the canonical ordering; update the import block that currently lists context and the packages (agentremote, openai-go, zerolog, bridgev2, event, id, bridgesdk) accordingly.bridges/dummybridge/connector.go-69-79 (1)
69-79:⚠️ Potential issue | 🟡 MinorUse
GetLoginFlowsfor dynamic login-flow discovery.The static
LoginFlowsis evaluated at connector construction time, before config is loaded. SinceConfig.DummyBridge.Enabledis nil at that point,dc.enabled()returns true and the DummyBridge flow is advertised regardless of the later loaded config. If a user's config hasdummybridge.enabled: false, the flow appears in discovery but is rejected at login time, creating inconsistency. ImplementGetLoginFlows(supported by the SDK and used in bridges/ai) to make flow advertisement dynamic and dependent on the loaded config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/dummybridge/connector.go` around lines 69 - 79, Replace the static LoginFlows with a dynamic GetLoginFlows implementation so advertised flows reflect runtime config: add a GetLoginFlows(ctx context.Context) ([]bridgev2.LoginFlow, error) on the connector that calls dc.enabled() and returns the login flow only when enabled (using the same ProviderDummyBridge ID/metadata), keep CreateLogin and its agentremote.ValidateSingleLoginFlow check as-is, and ensure DummyBridgeLogin construction still returns when a flow is requested; this prevents advertising the DummyBridge flow before config is loaded.bridges/dummybridge/login.go-59-63 (1)
59-63:⚠️ Potential issue | 🟡 MinorTruncate the display suffix by runes, not bytes.
trimmed[:40]can cut a multi-byte code point in half and leaveremoteNamewith invalid UTF-8.💡 Suggested change
if trimmed := strings.TrimSpace(value); trimmed != "" { - if len(trimmed) > 40 { - trimmed = trimmed[:40] + runes := []rune(trimmed) + if len(runes) > 40 { + trimmed = string(runes[:40]) } remoteName = fmt.Sprintf("%s (%s)", dummyAgentName, trimmed) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/dummybridge/login.go` around lines 59 - 63, The code in the remote name construction (inside the block that sets remoteName using dummyAgentName and trimmed) slices the string by bytes with trimmed[:40], which can split multi-byte runes; replace this byte-slice with a rune-aware truncation: convert trimmed to a []rune, check length, and slice to at most 40 runes before converting back to string and using fmt.Sprintf to set remoteName so UTF-8 remains valid.
🧹 Nitpick comments (7)
pkg/connector/connector.go (1)
5-12: Consider aligning function name with the type it returns.
NewAIConnector()returns*OpenAIConnector, which creates a slight naming inconsistency. IfOpenAIis an implementation detail, this is acceptable; otherwise, consider renaming toNewOpenAIConnector()for consistency.Also, consider adding brief doc comments to the exported types and function for API clarity.
📝 Optional: Add documentation and align naming
package connector import ai "github.com/beeper/agentremote/bridges/ai" +// Config is the configuration for the AI connector. type ( Config = ai.Config + // OpenAIConnector is the connector implementation for OpenAI-compatible APIs. OpenAIConnector = ai.OpenAIConnector ) -func NewAIConnector() *OpenAIConnector { +// NewOpenAIConnector creates a new OpenAI connector instance. +func NewOpenAIConnector() *OpenAIConnector { return ai.NewAIConnector() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connector/connector.go` around lines 5 - 12, Rename the constructor to align with the returned type and add brief doc comments: change NewAIConnector to NewOpenAIConnector (which returns *OpenAIConnector) and add short exported comments for Config, OpenAIConnector, and NewOpenAIConnector to clarify their purpose; update any callers referencing NewAIConnector to NewOpenAIConnector (or alternatively change the return alias to the more generic name if you prefer keeping NewAIConnector) so naming is consistent with the ai.OpenAIConnector symbol.cmd/internal/selfhost/registration.go (1)
176-183: Prefer structured not-found checks here.Line 180 currently treats any error text containing
404/not foundas a missing resource. That is brittle and can suppress unrelated API failures; ifhungryapiorbeeperapiexpose an HTTP status or typed error, use that instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/internal/selfhost/registration.go` around lines 176 - 183, The isRemoteNotFoundError function is doing brittle string matching on err.Error(); change it to detect structured not-found signals first by checking for known typed errors or status codes: assert the error against any exported sentinel errors (e.g., hungryapi.ErrNotFound / beeperapi.ErrNotFound) using errors.Is, and/or type-assert to an interface that exposes an HTTP status (e.g., interface{ StatusCode() int } or { Code() int }) and return true when that status equals 404; only keep the current lowercase substring fallback as a last resort to preserve behavior for unknown clients. Ensure you update isRemoteNotFoundError to try errors.Is(err, <api>.ErrNotFound) and then check a StatusCode()/Code() accessor before falling back to string checks.sdk/stream_replay_test.go (2)
55-56: Assert reasoning content viaTurnPart.Reasoninginstead ofTurnPart.Text.This keeps the test aligned with the structured turn-data contract and avoids relying on field aliasing behavior.
♻️ Proposed test adjustment
- if parts[2].Type != "reasoning" || parts[2].Text != "thinking" { + if parts[2].Type != "reasoning" || parts[2].Reasoning != "thinking" { t.Fatalf("expected replayed reasoning part, got %#v", parts[2]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/stream_replay_test.go` around lines 55 - 56, The test currently asserts the reasoning content from TurnPart.Text; change it to assert via TurnPart.Reasoning instead: in the assertion that checks parts[2] (and retains the Type == "reasoning" check), verify parts[2].Reasoning == "thinking" and update the failure message to reflect that it expected a reasoning part with the correct Reasoning field rather than Text so the test uses the structured turn-data contract.
79-84: The defaults test currently passes an explicit finish reason.If the goal is to validate defaulting behavior, call
Finishwithoutfinish_reasonand keep the same assertion.♻️ Proposed test adjustment
- replayer.Finish("", map[string]any{"finish_reason": "stop"}) + replayer.Finish("", nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/stream_replay_test.go` around lines 79 - 84, The test currently passes an explicit finish reason to replayer.Finish which prevents asserting defaulting behavior; change the call to replayer.Finish so it omits the "finish_reason" entry (call Finish with just the empty string and no metadata map or with a nil/empty map), then keep the existing snapshot/extraction via streamui.SnapshotUIMessage(state) and the metadata["finish_reason"] assertion to verify the default is applied; locate the call to replayer.Finish and the following use of streamui.SnapshotUIMessage and metadata in the test and remove the explicit finish_reason from the Finish invocation.README.md (1)
225-228: Consider includingdummybridgein this reset snippet.Since
dummybridgeis now a registered bridge type, omitting it here can leave local state behind for users who tried the demo bridge.📝 Optional doc tweak
./tools/bridges delete ai ./tools/bridges delete codex +./tools/bridges delete dummybridge ./tools/bridges delete openclaw ./tools/bridges delete opencode🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 225 - 228, Update the reset snippet in README.md to also remove the newly registered demo bridge by adding a line that deletes "dummybridge"; specifically, add "./tools/bridges delete dummybridge" alongside the existing "./tools/bridges delete ai", "./tools/bridges delete codex", "./tools/bridges delete openclaw", and "./tools/bridges delete opencode" commands so local state from the demo bridge is cleaned up.bridges/opencode/backfill_canonical.go (1)
28-31: RedundantInitMapscall.Line 30 calls
state.InitMaps()afterNewUIStateReplayer(&state)on line 28. The replayer constructor already initializes maps when the state is non-nil. This is safe but redundant.♻️ Remove redundant InitMaps call
state := streamui.UIState{TurnID: turnID} replayer := bridgesdk.NewUIStateReplayer(&state) startMeta := buildTurnStartMetadata(&msg, agentID) - state.InitMaps() replayer.Start(startMeta)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/opencode/backfill_canonical.go` around lines 28 - 31, Remove the redundant state.InitMaps() call: NewUIStateReplayer(&state) already initializes the state's maps when a non-nil state is provided, so delete the state.InitMaps() invocation (the one between NewUIStateReplayer(&state) and replayer.Start(startMeta)) to avoid duplicate initialization while keeping the surrounding calls to buildTurnStartMetadata, replayer.Start, and the replayer variable intact.cmd/agentremote/main.go (1)
904-940: Consider removing the deprecated--remoteflag entirely.The
--remoteflag is marked deprecated and its value is explicitly discarded (_ = *remote). If remote deletion always happens now, consider removing the flag definition entirely in a follow-up to avoid user confusion.The current approach of keeping the flag but ignoring it maintains backward compatibility for existing scripts. If this is intentional for a transition period, it's acceptable as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/agentremote/main.go` around lines 904 - 940, Remove the unused deprecated --remote flag from cmdDelete: delete the remote flag definition (the remote := fs.Bool("remote", ...) line) and the no-op discard (_ = *remote) so the function no longer declares or references the flag; also update the flag parser/help by removing any reference to "deprecated: remote deletion always happens" in the newFlagSet usage for cmdDelete (ensure no other references to remote exist in cmdDelete).
🤖 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/response_finalization.go`:
- Around line 556-567: The final edit payload currently drops thread context
because sendFinalAssistantTurnContent only accepts replyToEventID; change
sendFinalAssistantTurnContent to accept the full ReplyTarget (or at least both
ReplyTo and ThreadRoot) and update its callers (the place passing
finalReplyTarget.ReplyTo) to pass the entire finalReplyTarget from
resolveFinalReplyTarget; then when constructing the sdk.FinalEditPayload in
sendFinalAssistantTurnContent (where buildFinalEditTopLevelExtra and
state.turn.SetFinalEditPayload are used), populate both ReplyTo and ThreadRoot
fields from the passed ReplyTarget so thread-rooted replies are preserved.
In `@bridges/codex/client.go`:
- Around line 1558-1571: The sandbox policy returned by buildSandboxPolicy
contains an invalid "readOnlyAccess" key; remove that field from the returned
map so the policy only includes supported keys (type, writableRoots,
networkAccess, excludeTmpdirEnvVar, excludeSlashTmp). Also audit usages of
buildSandboxMode and buildSandboxPolicy (e.g., calls to buildSandboxMode()) to
ensure you don't pass the string mode where the API expects the full sandbox
policy object; if any callers need the full policy, make them call
buildSandboxPolicy(cwd) instead of buildSandboxMode().
In `@bridges/dummybridge/runtime.go`:
- Around line 317-387: parseRandomCommand sets randomCommand.Duration but
runRandom ignores it, so positional duration is a no-op; update runRandom (the
function that executes a *randomCommand) to respect cmd.Duration by stopping the
workload when the elapsed time exceeds cmd.Duration (or by using a context with
timeout derived from cmd.Duration) while still honoring cmd.Actions as an upper
bound — i.e., run until either the requested number of actions completes or the
duration expires; ensure the loop checks elapsed time (or ctx.Done()) between
actions and that default behavior remains unchanged when Duration is zero or not
set.
- Around line 267-315: The parsers currently accept unbounded numeric and list
inputs; add hard caps and validation so user-supplied values cannot blow memory
or spawn excessive work: introduce constants (e.g., MaxChars, MaxTools,
MaxActions, MaxTurns, MaxDelayMs, MaxChunkSize) and enforce them in
parseLoremCommand, parseToolsCommand and in any other parsers (the ranges
mentioned around lines 317-590) by validating the outputs of parsePositiveInt
and the length of toolTokens (and use parseToolSpec) to return a clear error
when a value exceeds the cap instead of accepting it; also validate delay/chunk
ranges used by runRandom and runChaos (and clamp or error for min>max) before
those values propagate to buildLoremText, runRandom, and runChaos to prevent
huge allocations or massive goroutine spawning.
In `@bridges/openclaw/stream.go`:
- Around line 52-67: Concurrent callers can both observe state.turn == nil and
call newSDKStreamTurn(), duplicating setup and racing on shared state; fix by
serializing turn creation per-state: add a creation sentinel (e.g., a bool like
state.creatingTurn) or a sync.Once field on the stream state and use it when
creating a turn so only one caller runs newSDKStreamTurn(), and ensure callers
set state.turn once under oc.streamHost lock (functions/fields involved:
ensureStreamStateLocked, applyStreamPartStateLocked, state.turn,
newSDKStreamTurn, oc.streamHost). Acquire the lock to check/set the sentinel or
call the sync.Once wrapper so the expensive creation happens only once and the
losing callers reuse state.turn without duplicating side effects.
In `@cmd/internal/selfhost/registration.go`:
- Around line 144-159: remoteBridgeDeleted currently assumes appserviceGone=true
when auth.Username is empty and then skips verification, which can falsely
report success; change remoteBridgeDeleted to not default to true or skip
verification: after calling beeperapi.Whoami (in function remoteBridgeDeleted)
use the returned who.User data to derive the appservice presence when
auth.Username is empty (e.g., check who.User.AppServices or similar) or, if
who.User does not contain sufficient info, return an explicit verification error
instead of returning (bridgeGone, appserviceGone, nil); do not short-circuit
verification when auth.Username == ""—either compute appserviceGone from who or
return an error indicating verification could not be performed.
In `@go.mod`:
- Line 27: The go.mod contains a developer-local replace directive "replace
maunium.net/go/mautrix =>
/Users/batuhan/Projects/texts/beeper-workspace/mautrix/go" that must be removed
before merge; delete that replace line from go.mod and either reference the
upstream mautrix module by a version/commit or move the local path override into
a workspace file (go.work) for local development so CI and other developers can
resolve the module.
In `@pkg/matrixevents/matrixevents.go`:
- Line 14: The constant StreamEventMessageType was changed from the existing
on-the-wire value and will break backwards compatibility for older consumers;
restore the original value or implement a compatibility path by either reverting
StreamEventMessageType back to "com.beeper.ai.stream_event" or dual-emit both
event types during rollout (e.g., keep StreamEventMessageType as the original
and add a new constant like StreamEventMessageTypeV2 = event.Type{Type:
"com.beeper.llm", Class: event.EphemeralEventType}) and update producers to emit
both until consumers are migrated; update any code that references
StreamEventMessageType accordingly to ensure both old and new consumers can
receive deltas.
In `@sdk/stream_part_state.go`:
- Around line 35-37: The code is trimming the delta payload
(strings.TrimSpace(stringValue(part["delta"]))) which removes meaningful
whitespace; change it to use the raw stringValue(part["delta"]) (no TrimSpace)
and only skip when that raw value is an empty string (== ""), so
whitespace/newlines are preserved; update both occurrences where
s.visible.WriteString and s.accumulated.WriteString are called (and any other
uses of strings.TrimSpace on part["delta"]) to use the raw value and the
empty-string check instead.
In `@sdk/stream_replay.go`:
- Around line 74-75: The code is trimming message payloads (variables like text,
reasoning, toolInputText) which can alter replayed content; remove the
strings.TrimSpace calls for those payload variables and only normalize/trim IDs
such as partID before validation. Update the conditional checks (e.g., the if
that currently uses partID == "" || text == "") to use the untrimmed payload
values for emptiness/validation and keep trimming/normalizing only for
identifiers (partID); apply the same change to the other occurrences referenced
(the blocks handling reasoning and toolInputText).
In `@sdk/turn_manager.go`:
- Around line 14-16: You added IdleTimeoutMs before KeyFunc in the TurnConfig
struct which changes field order and can break callers using unkeyed composite
literals; to fix, either move IdleTimeoutMs to the end of the TurnConfig
declaration or provide a constructor (e.g., NewTurnConfig) that accepts
parameters and returns a fully initialized TurnConfig, and update public docs to
require keyed literals or the constructor; reference the struct name TurnConfig
and the fields IdleTimeoutMs and KeyFunc so maintainers can locate and apply the
fix.
In `@sdk/turn.go`:
- Around line 964-991: The awaitStreamStart loop retries on every error; change
it to bail out on permanent errors from session.EnsureStarted instead of
retrying forever. Update awaitStreamStart so after calling
t.session.EnsureStarted(t.turnCtx) you check: if err != nil { if errors.Is(err,
session.ErrNoPublisher) || errors.Is(err, session.ErrClosed) ||
isPermanentEnsureStartedError(err) { return } /* otherwise continue retrying */
}. Use errors.Is against the session package's exported sentinel errors (or add
a helper like isPermanentEnsureStartedError to encapsulate checks) so the
goroutine returns immediately for non-stream-capable or closed-session errors
while preserving retries for transient errors.
- Around line 327-337: The session callback closures capture raw fields
(t.initialEventID, t.suppressSend, and t.defaultStreamPublisher) and thus can
race with applyPlaceholderSendResult(); change them to call the thread-safe
accessors instead (use the locked getter methods for target event ID and
suppressSend and the accessor wrapper for stream publisher) so GetTargetEventID,
GetSuppressSend and GetStreamPublisher invoke the methods that acquire t.mu
(e.g., t.getTargetEventID(), t.getSuppressSend(), and the existing safe
defaultStreamPublisher accessor) rather than closing over the raw fields.
- Around line 649-665: The edit event currently adds reply/thread context under
topLevelExtra["m.relates_to"] when t.initialEventID != "" (setting
"m.in_reply_to" and "m.thread" from payload.ReplyTo and payload.ThreadRoot);
remove those nested additions and ensure topLevelExtra["m.relates_to"] only
contains the m.replace relation ({ "rel_type": matrixevents.RelReplace,
"event_id": t.initialEventID.String() }) so the edit relies on the original
event's thread/reply context instead of embedding "m.in_reply_to" or "m.thread".
In `@stream_turn_host.go`:
- Around line 78-79: The DrainAndAbort flow can panic when StreamTurnHost was
constructed with a zero-value StreamTurnHostCallbacks because
callbacks.GetAborter may be nil; update DrainAndAbort to guard before calling by
checking h.callbacks != nil and h.callbacks.GetAborter != nil (or call a safe
wrapper) before invoking GetAborter and appending its result, or alternatively
ensure NewStreamTurnHost assigns a default no-op StreamTurnHostCallbacks so
h.callbacks.GetAborter is always safe; reference h.callbacks, GetAborter,
DrainAndAbort and NewStreamTurnHost when applying the guard or defaulting.
- Around line 34-37: The one-line method definitions for StreamTurnHost[S].Lock
and StreamTurnHost[S].Unlock are causing CI formatting failures; replace the
single-line function bodies with standard multi-line definitions (i.e., put the
opening brace on the first line and the body on the next line) or simply run
gofmt/goimports (or gofmt -s / goimports) over the file to normalize formatting
so the methods Lock() and Unlock() conform to the project's formatting rules.
In `@turns/session.go`:
- Around line 101-121: Start currently aborts when
s.params.GetStreamPublisher(ctx) is nil, which prevents pure SendHook consumers
from progressing; change Start so that if GetStreamPublisher returns nil it only
returns early when no SendHook is registered, i.e. detect the hook (via the
stream descriptor's SendHook or the params API used for hooks) and allow
execution to continue with a nil publisher passed into
tryStart/publishPendingPart; apply the same change to the other Start-like block
mentioned so hook-only callers are not blocked and FlushPending still runs.
- Around line 81-98: The code currently uses descriptorOnce in
StreamSession.Descriptor which permanently records a failed lookup
(descriptorErr) when publisher or roomID aren't ready; replace the sync.Once
pattern with a mutex-guarded "loaded" flag so transient failures aren't cached:
add (or reuse) a mutex (e.g., s.mu) and a boolean (e.g., s.descriptorLoaded),
then in Descriptor lock the mutex, if s.descriptorLoaded return cached values,
otherwise attempt to get publisher via s.params.GetStreamPublisher(ctx) and
roomID and call publisher.NewDescriptor(ctx,...); on success set s.descriptor,
s.descriptorErr=nil, s.descriptorLoaded=true; on failure return the error but do
NOT set descriptorLoaded so subsequent calls can retry; keep using the same
field names (Descriptor, descriptor, descriptorErr, params.GetStreamPublisher,
NewDescriptor) to locate and update the logic.
---
Minor comments:
In `@bridges/ai/streaming_init.go`:
- Around line 3-14: Reorder and normalize the import block so goimports/go fmt
passes: group standard library imports (context) first, then a blank line, then
third-party modules grouped and sorted (e.g., github.com/rs/zerolog,
github.com/openai/openai-go/v3, github.com/beeper/agentremote, bridgesdk
"github.com/beeper/agentremote/sdk", maunium.net/... packages), remove any
unused imports, and run goimports (or go fmt) to apply the canonical ordering;
update the import block that currently lists context and the packages
(agentremote, openai-go, zerolog, bridgev2, event, id, bridgesdk) accordingly.
In `@bridges/dummybridge/agent.go`:
- Around line 3-6: The import block in bridges/dummybridge/agent.go is
misordered and failing the go-imports-repo pre-commit hook; reorder the imports
to follow Go conventions (standard library first, then external third-party
modules, then internal/repo packages), e.g. ensure any standard library packages
(if present) are listed before third-party packages and that third-party imports
like "maunium.net/go/mautrix/bridgev2/networkid" and internal imports like
bridgesdk "github.com/beeper/agentremote/sdk" are grouped and sorted; run
goimports (or go fmt) to automatically fix formatting and grouping and re-run
the pipeline.
In `@bridges/dummybridge/connector.go`:
- Around line 69-79: Replace the static LoginFlows with a dynamic GetLoginFlows
implementation so advertised flows reflect runtime config: add a
GetLoginFlows(ctx context.Context) ([]bridgev2.LoginFlow, error) on the
connector that calls dc.enabled() and returns the login flow only when enabled
(using the same ProviderDummyBridge ID/metadata), keep CreateLogin and its
agentremote.ValidateSingleLoginFlow check as-is, and ensure DummyBridgeLogin
construction still returns when a flow is requested; this prevents advertising
the DummyBridge flow before config is loaded.
In `@bridges/dummybridge/login.go`:
- Around line 59-63: The code in the remote name construction (inside the block
that sets remoteName using dummyAgentName and trimmed) slices the string by
bytes with trimmed[:40], which can split multi-byte runes; replace this
byte-slice with a rune-aware truncation: convert trimmed to a []rune, check
length, and slice to at most 40 runes before converting back to string and using
fmt.Sprintf to set remoteName so UTF-8 remains valid.
---
Nitpick comments:
In `@bridges/opencode/backfill_canonical.go`:
- Around line 28-31: Remove the redundant state.InitMaps() call:
NewUIStateReplayer(&state) already initializes the state's maps when a non-nil
state is provided, so delete the state.InitMaps() invocation (the one between
NewUIStateReplayer(&state) and replayer.Start(startMeta)) to avoid duplicate
initialization while keeping the surrounding calls to buildTurnStartMetadata,
replayer.Start, and the replayer variable intact.
In `@cmd/agentremote/main.go`:
- Around line 904-940: Remove the unused deprecated --remote flag from
cmdDelete: delete the remote flag definition (the remote := fs.Bool("remote",
...) line) and the no-op discard (_ = *remote) so the function no longer
declares or references the flag; also update the flag parser/help by removing
any reference to "deprecated: remote deletion always happens" in the newFlagSet
usage for cmdDelete (ensure no other references to remote exist in cmdDelete).
In `@cmd/internal/selfhost/registration.go`:
- Around line 176-183: The isRemoteNotFoundError function is doing brittle
string matching on err.Error(); change it to detect structured not-found signals
first by checking for known typed errors or status codes: assert the error
against any exported sentinel errors (e.g., hungryapi.ErrNotFound /
beeperapi.ErrNotFound) using errors.Is, and/or type-assert to an interface that
exposes an HTTP status (e.g., interface{ StatusCode() int } or { Code() int })
and return true when that status equals 404; only keep the current lowercase
substring fallback as a last resort to preserve behavior for unknown clients.
Ensure you update isRemoteNotFoundError to try errors.Is(err, <api>.ErrNotFound)
and then check a StatusCode()/Code() accessor before falling back to string
checks.
In `@pkg/connector/connector.go`:
- Around line 5-12: Rename the constructor to align with the returned type and
add brief doc comments: change NewAIConnector to NewOpenAIConnector (which
returns *OpenAIConnector) and add short exported comments for Config,
OpenAIConnector, and NewOpenAIConnector to clarify their purpose; update any
callers referencing NewAIConnector to NewOpenAIConnector (or alternatively
change the return alias to the more generic name if you prefer keeping
NewAIConnector) so naming is consistent with the ai.OpenAIConnector symbol.
In `@README.md`:
- Around line 225-228: Update the reset snippet in README.md to also remove the
newly registered demo bridge by adding a line that deletes "dummybridge";
specifically, add "./tools/bridges delete dummybridge" alongside the existing
"./tools/bridges delete ai", "./tools/bridges delete codex", "./tools/bridges
delete openclaw", and "./tools/bridges delete opencode" commands so local state
from the demo bridge is cleaned up.
In `@sdk/stream_replay_test.go`:
- Around line 55-56: The test currently asserts the reasoning content from
TurnPart.Text; change it to assert via TurnPart.Reasoning instead: in the
assertion that checks parts[2] (and retains the Type == "reasoning" check),
verify parts[2].Reasoning == "thinking" and update the failure message to
reflect that it expected a reasoning part with the correct Reasoning field
rather than Text so the test uses the structured turn-data contract.
- Around line 79-84: The test currently passes an explicit finish reason to
replayer.Finish which prevents asserting defaulting behavior; change the call to
replayer.Finish so it omits the "finish_reason" entry (call Finish with just the
empty string and no metadata map or with a nil/empty map), then keep the
existing snapshot/extraction via streamui.SnapshotUIMessage(state) and the
metadata["finish_reason"] assertion to verify the default is applied; locate the
call to replayer.Finish and the following use of streamui.SnapshotUIMessage and
metadata in the test and remove the explicit finish_reason from the Finish
invocation.
🪄 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: 7ed39ed0-ce2e-4e8b-8f0c-db57a25abe39
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (70)
README.mdbridges.manifest.ymlbridges/ai/msgconv/to_matrix.gobridges/ai/msgconv/to_matrix_test.gobridges/ai/response_finalization.gobridges/ai/response_finalization_test.gobridges/ai/streaming_init.gobridges/ai/streaming_ui_helpers.gobridges/codex/approvals_test.gobridges/codex/client.gobridges/codex/client_test.gobridges/codex/login.gobridges/codex/stream_mapping_test.gobridges/codex/streaming_support.gobridges/dummybridge/agent.gobridges/dummybridge/bridge.gobridges/dummybridge/config.gobridges/dummybridge/connector.gobridges/dummybridge/connector_test.gobridges/dummybridge/example-config.yamlbridges/dummybridge/login.gobridges/dummybridge/login_test.gobridges/dummybridge/metadata.gobridges/dummybridge/runtime.gobridges/dummybridge/runtime_test.gobridges/openclaw/client.gobridges/openclaw/login.gobridges/openclaw/manager.gobridges/openclaw/stream.gobridges/openclaw/stream_test.gobridges/opencode/backfill_canonical.gobridges/opencode/client.gobridges/opencode/host.gobridges/opencode/login.gobridges/opencode/opencode_text_stream.gobridges/opencode/opencode_turn_stream.gobridges/opencode/stream_canonical.gobridges/opencode/stream_canonical_test.gocmd/agentremote/bridges.gocmd/agentremote/commands.gocmd/agentremote/main.gocmd/dummybridge/main.gocmd/internal/bridgeentry/bridgeentry.gocmd/internal/selfhost/registration.godocs/bridge-orchestrator.mddocs/matrix-ai-matrix-spec-v1.mddocs/msc/com.beeper.mscXXXX-ephemeral.mddocs/msc/com.beeper.mscXXXX-streaming.mdgo.modhelpers.gologin_helpers.gopkg/connector/connector.gopkg/matrixevents/matrixevents.gosdk/final_edit.gosdk/part_apply.gosdk/stream_part_state.gosdk/stream_part_state_test.gosdk/stream_replay.gosdk/stream_replay_test.gosdk/turn.gosdk/turn_manager.gosdk/turn_test.gostream_turn_host.goturns/converted_edit.goturns/debounced_edit.goturns/debounced_edit_test.goturns/fallback.goturns/fallback_test.goturns/session.goturns/session_target_test.go
💤 Files with no reviewable changes (7)
- bridges/ai/msgconv/to_matrix_test.go
- bridges/ai/msgconv/to_matrix.go
- helpers.go
- turns/fallback_test.go
- turns/fallback.go
- turns/debounced_edit_test.go
- turns/debounced_edit.go
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-16T09:01:24.464Z
Learnt from: batuhan
Repo: beeper/agentremote PR: 71
File: bridges/ai/connector.go:53-63
Timestamp: 2026-03-16T09:01:24.464Z
Learning: In package ai, the AI connector’s configuration type (Config) defines Bridge as a value field of type BridgeConfig (not a pointer). Therefore, accessing oc.Config.Bridge.CommandPrefix in OpenAIConnector.applyRuntimeDefaults (bridges/ai/connector.go) is safe and does not require a nil-check.
Applied to files:
cmd/agentremote/bridges.gobridges/opencode/login.gopkg/connector/connector.gocmd/dummybridge/main.gobridges/dummybridge/connector_test.gobridges/dummybridge/connector.gobridges/dummybridge/config.gocmd/agentremote/main.gobridges/dummybridge/runtime.gobridges/dummybridge/bridge.go
🧬 Code graph analysis (28)
cmd/agentremote/bridges.go (2)
cmd/internal/bridgeentry/bridgeentry.go (2)
Definition(13-18)DummyBridge(45-50)bridges/dummybridge/connector.go (1)
NewConnector(32-92)
bridges/openclaw/login.go (2)
bridges/opencode/bridge.go (1)
Bridge(74-79)login_helpers.go (1)
ValidateLoginState(13-21)
bridges/ai/streaming_ui_helpers.go (1)
sdk/final_edit.go (1)
BuildCompactFinalUIMessage(12-54)
bridges/opencode/login.go (2)
bridges/opencode/bridge.go (1)
Bridge(74-79)login_helpers.go (1)
ValidateLoginState(13-21)
pkg/connector/connector.go (2)
bridges/codex/config.go (1)
Config(13-17)bridges/opencode/config.go (1)
Config(16-19)
cmd/dummybridge/main.go (3)
cmd/internal/bridgeentry/bridgeentry.go (2)
Run(63-67)DummyBridge(45-50)bridges/dummybridge/connector.go (1)
NewConnector(32-92)cmd/agentremote/main.go (3)
Tag(27-27)Commit(28-28)BuildTime(29-29)
bridges/dummybridge/connector_test.go (2)
bridges/dummybridge/connector.go (1)
NewConnector(32-92)pkg/search/router.go (1)
Search(12-37)
bridges/dummybridge/login_test.go (2)
bridges/dummybridge/login.go (1)
DummyBridgeLogin(20-24)bridges/dummybridge/connector.go (1)
DummyBridgeConnector(20-30)
login_helpers.go (1)
bridges/opencode/bridge.go (1)
Bridge(74-79)
sdk/stream_part_state_test.go (1)
sdk/stream_part_state.go (1)
StreamPartState(8-17)
bridges/dummybridge/agent.go (1)
sdk/agent.go (2)
Agent(59-68)BaseAgentCapabilities(36-45)
bridges/ai/response_finalization.go (2)
sdk/turn.go (1)
FinalEditPayload(43-48)sdk/final_edit.go (1)
BuildDefaultFinalEditTopLevelExtra(58-67)
bridges/dummybridge/connector.go (6)
connector_builder.go (1)
ConnectorBase(32-35)bridges/dummybridge/config.go (1)
Config(16-19)sdk/connector_helpers.go (5)
NewStandardConnectorConfig(123-165)StandardConnectorConfigParams(82-119)ApplyDefaultCommandPrefix(29-33)ApplyBoolDefault(36-42)AcceptProviderLogin(44-59)sdk/types.go (1)
ProviderIdentity(212-216)identifier_helpers.go (2)
SingleLoginFlow(63-68)ValidateSingleLoginFlow(70-75)sdk/connector.go (1)
NewConnectorBase(18-167)
cmd/internal/selfhost/registration.go (1)
cmd/internal/beeperauth/auth.go (1)
Config(27-32)
sdk/stream_replay_test.go (5)
pkg/shared/streamui/emitter.go (1)
UIState(14-36)sdk/stream_replay.go (1)
NewUIStateReplayer(14-19)pkg/shared/citations/citations.go (2)
SourceCitation(11-20)SourceDocument(23-28)pkg/shared/streamui/recorder.go (1)
SnapshotUIMessage(212-217)sdk/turn_data.go (1)
TurnDataFromUIMessage(95-147)
bridges/dummybridge/config.go (2)
cmd/internal/bridgeentry/bridgeentry.go (1)
DummyBridge(45-50)pkg/shared/exa/client.go (1)
Enabled(15-17)
bridges/opencode/backfill_canonical.go (3)
sdk/stream_replay.go (2)
NewUIStateReplayer(14-19)UIStateReplayer(10-12)pkg/shared/streamui/emitter.go (1)
UIState(14-36)pkg/shared/citations/citations.go (2)
SourceCitation(11-20)SourceDocument(23-28)
bridges/openclaw/client.go (2)
stream_turn_host.go (4)
StreamTurnHost(19-23)NewStreamTurnHost(26-31)StreamTurnHostCallbacks(12-15)Aborter(7-9)sdk/stream_part_state.go (1)
StreamPartState(8-17)
cmd/agentremote/main.go (2)
pkg/shared/bridgeutil/process.go (1)
ProcessAliveFromPIDFile(83-93)cmd/internal/cliutil/state.go (1)
ReadMetadata(50-60)
bridges/dummybridge/runtime.go (9)
bridges/dummybridge/connector.go (1)
DummyBridgeConnector(20-30)sdk/conversation.go (1)
Conversation(22-33)sdk/turn.go (1)
Turn(102-145)sdk/writer.go (3)
Writer(33-41)ToolInputOptions(14-19)ToolOutputOptions(22-26)pkg/shared/citations/citations.go (2)
SourceCitation(11-20)SourceDocument(23-28)bridges/dummybridge/metadata.go (1)
MessageMetadata(26-30)sdk/types.go (1)
ApprovalRequest(101-108)pkg/shared/streamui/recorder.go (1)
SnapshotUIMessage(212-217)pkg/shared/streamui/emitter.go (1)
UIState(14-36)
bridges/openclaw/stream.go (3)
sdk/part_apply.go (2)
ApplyStreamPart(24-110)PartApplyOptions(12-20)bridges/openclaw/client.go (1)
OpenClawClient(60-78)pkg/shared/streamui/recorder.go (1)
SnapshotUIMessage(212-217)
bridges/opencode/client.go (3)
stream_turn_host.go (4)
StreamTurnHost(19-23)NewStreamTurnHost(26-31)StreamTurnHostCallbacks(12-15)Aborter(7-9)sdk/turn.go (1)
Turn(102-145)sdk/stream_part_state.go (1)
StreamPartState(8-17)
bridges/openclaw/manager.go (4)
sdk/stream_replay.go (2)
NewUIStateReplayer(14-19)UIStateReplayer(10-12)pkg/shared/stringutil/coalesce.go (1)
TrimDefault(46-52)pkg/shared/jsonutil/jsonutil.go (2)
ToMap(10-27)DeepCloneAny(42-71)pkg/shared/openclawconv/content.go (1)
ExtractMessageText(68-85)
bridges/dummybridge/login.go (4)
bridges/dummybridge/connector.go (1)
DummyBridgeConnector(20-30)login_helpers.go (2)
ValidateLoginState(13-21)CreateAndCompleteLogin(62-88)bridges/dummybridge/config.go (1)
ProviderDummyBridge(11-11)bridges/dummybridge/metadata.go (1)
UserLoginMetadata(10-14)
sdk/turn.go (7)
pkg/matrixevents/matrixevents.go (3)
BeeperAIKey(29-29)StreamEventMessageType(14-14)RelReplace(23-23)identifier_helpers.go (1)
MatrixMessageID(15-17)turns/converted_edit.go (1)
BuildConvertedEdit(9-20)helpers.go (1)
SendEditViaPortal(173-209)turns/session.go (4)
EndReasonFinish(22-22)EndReasonError(24-24)EndReasonDisconnect(23-23)EndReason(19-19)pkg/shared/streamui/recorder.go (2)
ApplyChunk(11-210)SnapshotUIMessage(212-217)sdk/final_edit.go (2)
BuildCompactFinalUIMessage(12-54)BuildDefaultFinalEditTopLevelExtra(58-67)
turns/session_target_test.go (1)
turns/session.go (3)
NewStreamSession(70-75)StreamSessionParams(27-43)EndReasonFinish(22-22)
turns/session.go (1)
pkg/matrixevents/matrixevents.go (4)
StreamEventMessageType(14-14)BuildStreamEventEnvelope(74-105)StreamEventOpts(68-71)BuildStreamEventTxnID(107-113)
sdk/stream_replay.go (3)
pkg/shared/streamui/emitter.go (1)
UIState(14-36)pkg/shared/streamui/recorder.go (1)
ApplyChunk(11-210)pkg/shared/citations/citations.go (2)
SourceCitation(11-20)SourceDocument(23-28)
🪛 Betterleaks (1.1.1)
docs/msc/com.beeper.mscXXXX-streaming.md
[high] 185-185: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 GitHub Actions: Go
bridges/dummybridge/agent.go
[error] 1-1: pre-commit hook 'go-imports-repo' failed (exit code 1). Hook modified Go import ordering.
bridges/ai/streaming_init.go
[error] 1-1: pre-commit hook 'go-imports-repo' failed (exit code 1). Hook modified Go import ordering.
pkg/matrixevents/matrixevents.go
[error] 7-2: go vet / module resolution error at pkg/matrixevents/matrixevents.go:7:2: replacement directory /Users/batuhan/Projects/texts/beeper-workspace/mautrix/go does not exist.
stream_turn_host.go
[error] 31-31: pre-commit hook 'go-imports-repo' failed (exit code 1). Hook modified formatting (extra spacing removed in method signature).
Update maunium.net/go/mautrix dependency to v0.26.5-0.20260322120516-fcf7539a7af5 in go.mod and remove the local replace directive that pointed to a workspace path. Corresponding checksums for the new version were added to go.sum.
Introduce ApprovalResolutionOrigin and track ResolvedBy on approval decisions so the flow can distinguish user vs agent/system resolutions. Mirror remote reactions only for user-origin decisions and add cleanup options to preserve selected placeholder reactions when an agent resolves an approval; update related redact helpers and tests. For OpenClaw, expand session metadata, add HTTP session history endpoint (SessionHistory) with cursor/limit handling and sequence-aware pagination, normalize history limits, and improve Recent/Session history plumbing (including auth handling, URL normalization, and patch param builder). Also wire per-portal approval senders, store approval hints in the manager, and add helpers to merge and emit approval prompts.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bridges/openclaw/manager_test.go (1)
3-11:⚠️ Potential issue | 🟡 MinorRun the repo import formatter on this block before merge.
CI is already failing the
go-imports-repohook on this file, so this import section still isn't in the repository's canonical layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/manager_test.go` around lines 3 - 11, Import ordering in the import block is not formatted to the repository canonical layout; run the repo import formatter (go-imports-repo) to reorder and group the imports so standard library imports (testing, time) come first, a blank line, then third-party imports (github.com/beeper/agentremote, maunium.net/go/mautrix/bridgev2, maunium.net/go/mautrix/bridgev2/database, maunium.net/go/mautrix/bridgev2/networkid), ensuring the import block in manager_test.go matches the repository's canonical layout before merging.
🧹 Nitpick comments (1)
bridges/openclaw/client.go (1)
733-757: Consider removing or using thectxparameter.The
ctxparameter insendNoticeViaPortalis declared but never used within the function body. If context is needed for future logging or cancellation, consider using it; otherwise, removing it would simplify the signature.♻️ Option 1: Remove unused ctx parameter
-func (oc *OpenClawClient) sendNoticeViaPortal(ctx context.Context, portal *bridgev2.Portal, msg string, sender bridgev2.EventSender) { +func (oc *OpenClawClient) sendNoticeViaPortal(portal *bridgev2.Portal, msg string, sender bridgev2.EventSender) {And update the wrapper:
-func (oc *OpenClawClient) sendSystemNoticeViaPortal(ctx context.Context, portal *bridgev2.Portal, msg string) { - oc.sendNoticeViaPortal(ctx, portal, msg, oc.senderForAgent("gateway", false)) +func (oc *OpenClawClient) sendSystemNoticeViaPortal(portal *bridgev2.Portal, msg string) { + oc.sendNoticeViaPortal(portal, msg, oc.senderForAgent("gateway", false))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/client.go` around lines 733 - 757, The ctx parameter in sendNoticeViaPortal is unused; remove it from the function signature and from all callers (e.g., update sendSystemNoticeViaPortal to call sendNoticeViaPortal without passing ctx), or alternatively use ctx for cancellation/logging if intended — ensure you update the method declaration on OpenClawClient (sendNoticeViaPortal) and the wrapper function sendSystemNoticeViaPortal to match the new signature and adjust any other callers to stop passing ctx.
🤖 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/openclaw/gateway_client.go`:
- Around line 1078-1085: The function buildPatchSessionParams currently copies
entries from the patch map onto params after seeding params["key"], allowing a
patch["key"] to overwrite the method argument; change the copy loop to skip any
entry with patchKey == "key" (or otherwise ensure params["key"] is set last so
the method argument wins) so the provided key cannot be clobbered, and add a
unit test that passes a patch containing "key" to assert the returned
params["key"] equals strings.TrimSpace(key) rather than the patch value.
- Around line 555-563: The code sets base.Path using url.PathEscape(sessionKey)
which double-escapes when URL.String() is called; change the assignment so you
do not pre-escape the session key: assign the raw/unescaped
strings.TrimSpace(sessionKey) (or use URL.JoinPath with the unescaped segment)
when building base.Path instead of url.PathEscape(sessionKey), leaving query
construction (query.Set calls, base.RawQuery = query.Encode()) unchanged; update
the line that currently references base.Path = strings.TrimRight(base.Path, "/")
+ "/sessions/" + url.PathEscape(strings.TrimSpace(sessionKey)) + "/history" to
use the unescaped sessionKey or URL.JoinPath so percent signs are not
double-encoded.
- Around line 1068-1076: The httpBearerAuthToken() function currently falls back
to cfg.Password and also DeviceToken is never persisted from Connect(); update
Connect() to save the parsed device token back into c.cfg.DeviceToken (assign
the token after parsing and before returning) so future HTTP calls use it, and
change gatewayWSClient.httpBearerAuthToken() to only return cfg.Token or
cfg.DeviceToken (trimmed) and otherwise return an empty string (remove the
Password fallback) so raw password is never used as a Bearer token.
---
Outside diff comments:
In `@bridges/openclaw/manager_test.go`:
- Around line 3-11: Import ordering in the import block is not formatted to the
repository canonical layout; run the repo import formatter (go-imports-repo) to
reorder and group the imports so standard library imports (testing, time) come
first, a blank line, then third-party imports (github.com/beeper/agentremote,
maunium.net/go/mautrix/bridgev2, maunium.net/go/mautrix/bridgev2/database,
maunium.net/go/mautrix/bridgev2/networkid), ensuring the import block in
manager_test.go matches the repository's canonical layout before merging.
---
Nitpick comments:
In `@bridges/openclaw/client.go`:
- Around line 733-757: The ctx parameter in sendNoticeViaPortal is unused;
remove it from the function signature and from all callers (e.g., update
sendSystemNoticeViaPortal to call sendNoticeViaPortal without passing ctx), or
alternatively use ctx for cancellation/logging if intended — ensure you update
the method declaration on OpenClawClient (sendNoticeViaPortal) and the wrapper
function sendSystemNoticeViaPortal to match the new signature and adjust any
other callers to stop passing ctx.
🪄 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: 06407fd2-3a7b-4f83-ba32-0cebb393da5c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
approval_decision.goapproval_flow.goapproval_flow_test.goapproval_prompt_test.goapproval_reaction_helpers.gobridges/openclaw/client.gobridges/openclaw/events.gobridges/openclaw/gateway_client.gobridges/openclaw/gateway_client_test.gobridges/openclaw/gateway_smoke_test.gobridges/openclaw/manager.gobridges/openclaw/manager_test.gobridges/openclaw/media_test.gobridges/openclaw/metadata.gobridges/openclaw/provisioning.gobridges/opencode/opencode_manager.gogo.mod
✅ Files skipped from review due to trivial changes (2)
- bridges/openclaw/provisioning.go
- go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- bridges/openclaw/manager.go
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (5)
approval_prompt_test.go (1)
approval_decision.go (1)
ApprovalResolutionOriginUser(23-23)
bridges/opencode/opencode_manager.go (1)
approval_decision.go (2)
ApprovalResolutionOriginFromString(45-54)ApprovalResolutionOriginUser(23-23)
approval_flow_test.go (3)
approval_prompt.go (2)
ApprovalPromptRegistration(469-483)ApprovalReactionKeyAllowAlways(23-23)approval_reaction_helpers.go (1)
ApprovalPromptReactionCleanupOptions(132-135)approval_decision.go (3)
ApprovalResolutionOriginUser(23-23)ApprovalDecisionPayload(28-34)ApprovalResolutionOriginAgent(24-24)
approval_flow.go (3)
approval_prompt.go (1)
ApprovalPromptRegistration(469-483)approval_reaction_helpers.go (2)
ApprovalPromptReactionCleanupOptions(132-135)RedactApprovalPromptPlaceholderReactions(180-219)approval_decision.go (3)
ApprovalResolutionOriginAgent(24-24)ApprovalResolutionOriginUser(23-23)ApprovalDecisionPayload(28-34)
bridges/openclaw/client.go (2)
stream_turn_host.go (4)
StreamTurnHost(19-23)NewStreamTurnHost(26-31)StreamTurnHostCallbacks(12-15)Aborter(7-9)sdk/stream_part_state.go (1)
StreamPartState(8-17)
🪛 GitHub Actions: Go
bridges/openclaw/manager_test.go
[error] 1-1: pre-commit hook 'go-imports-repo' failed (exit code 1). Hook modified files to reorder Go imports.
🔇 Additional comments (17)
approval_decision.go (1)
20-54: LGTM!The new
ApprovalResolutionOrigintype and helper functions are well-designed:
- Clear distinction between user and agent origins
- Proper case-insensitive parsing with whitespace trimming
- Safe default to empty string for unknown values
approval_prompt_test.go (1)
152-154: LGTM!Good test coverage for the new
ResolvedByfield, ensuring that direct Matrix reactions are correctly attributed to user-origin resolution.bridges/opencode/opencode_manager.go (1)
846-852: LGTM!The fallback chain for determining resolution origin is well-designed:
- First tries the explicit
resolvedByfield- Falls back to
sourcefor backward compatibility- Defaults to
ApprovalResolutionOriginUserwhen neither is presentThis ensures existing integrations continue to work while supporting the new explicit field.
approval_flow_test.go (2)
66-69: LGTM!Test hook signature correctly updated to match the new
RedactApprovalPromptPlaceholderReactionsinterface.
670-742: LGTM!Thorough test coverage for agent-origin resolution behavior:
- Correctly validates that no user reaction mirroring occurs
- Verifies placeholder cleanup preserves the selected reaction key
- Proper timeout handling for both positive and negative assertions
approval_reaction_helpers.go (2)
132-153: LGTM!The
ApprovalPromptReactionCleanupOptionsstruct andshouldPreserveApprovalReactionhelper are well-implemented:
- Proper null checks and early returns
- Consistent string normalization for sender IDs
- Checks both
EmojiandEmojiIDfields for key matching
180-218: LGTM!The updated
RedactApprovalPromptPlaceholderReactionscorrectly integrates the preservation logic, skipping redaction for reactions that match the cleanup options.approval_flow.go (6)
118-119: LGTM!Test hook signature correctly updated to match the production code path.
450-472: LGTM!The updated
ResolveExternalproperly handles resolution origin:
- Clear doc comment explaining the behavior
- Defaults to
ApprovalResolutionOriginAgentfor external resolutions (correct assumption)- Only mirrors the user reaction when explicitly resolved by user
742-743: LGTM!Both
matchReactionandmatchFallbackReactioncorrectly setResolvedBy: ApprovalResolutionOriginUsersince they handle direct Matrix reactions from the owner.Also applies to: 794-794
1292-1304: LGTM!The
approvalCleanupOptionsfunction correctly computes preservation settings:
- Returns empty options for user-origin resolutions (all placeholders will be cleaned up)
- For agent-origin resolutions, preserves the placeholder matching the selected option
1306-1309: LGTM!The early-return guard in
mirrorRemoteDecisionReactionensures mirroring only occurs for user-originated resolutions, preventing duplicate reactions when the agent/system resolves.
1404-1416: LGTM!The cleanup options are correctly computed and propagated through both the test hook and production code paths.
bridges/openclaw/client.go (4)
77-77: LGTM!The new
streamHostfield properly uses the genericStreamTurnHostwith the correct type parameter, centralizing stream state management.
80-95: LGTM!The refactored
openClawStreamStatestruct properly delegates stream part lifecycle tracking tobridgesdk.StreamPartState, removing redundant fields while retaining OpenClaw-specific metadata (token counts, IDs, etc.).
168-168: LGTM!The
DrainAndAbort("disconnect")call cleanly centralizes stream cleanup, replacing the manual turn drainage and abort logic.
108-115: TheGetAbortercallback correctly returnss.turn(which implementsagentremote.Aborterwith the requiredAbort(reason string)method) and safely handles the nil case. No issues here.
Comment on lines
+1068
to
+1076
| func (c *gatewayWSClient) httpBearerAuthToken() string { | ||
| if token := strings.TrimSpace(c.cfg.Token); token != "" { | ||
| return token | ||
| } | ||
| if deviceToken := strings.TrimSpace(c.cfg.DeviceToken); deviceToken != "" { | ||
| return deviceToken | ||
| } | ||
| return strings.TrimSpace(c.cfg.Password) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "gateway_client.go" -type fRepository: beeper/agentremote
Length of output: 98
🏁 Script executed:
git ls-files | grep -E "openclaw|gateway"Repository: beeper/agentremote
Length of output: 1246
🏁 Script executed:
cat -n bridges/openclaw/gateway_client.go | head -100Repository: beeper/agentremote
Length of output: 4735
🏁 Script executed:
cat -n bridges/openclaw/gateway_client.go | sed -n '1050,1100p'Repository: beeper/agentremote
Length of output: 1793
🏁 Script executed:
cat -n bridges/openclaw/gateway_client.go | sed -n '900,950p'Repository: beeper/agentremote
Length of output: 1483
🏁 Script executed:
rg "Connect.*context.Context" bridges/openclaw/gateway_client.go -A 30Repository: beeper/agentremote
Length of output: 1958
🏁 Script executed:
rg "httpBearerAuthToken" bridges/openclaw/gateway_client.goRepository: beeper/agentremote
Length of output: 178
🏁 Script executed:
cat -n bridges/openclaw/gateway_client.go | sed -n '700,850p'Repository: beeper/agentremote
Length of output: 5696
🏁 Script executed:
rg "httpBearerAuthToken" bridges/openclaw/gateway_client.go -B 3 -A 3Repository: beeper/agentremote
Length of output: 516
🏁 Script executed:
cat -n bridges/openclaw/gateway_client.go | sed -n '500,600p'Repository: beeper/agentremote
Length of output: 3942
🏁 Script executed:
cat -n bridges/openclaw/gateway_client.go | sed -n '250,400p'Repository: beeper/agentremote
Length of output: 5415
🏁 Script executed:
rg "parseHelloDeviceToken" bridges/openclaw/gateway_client.go -B 5 -A 5Repository: beeper/agentremote
Length of output: 520
🏁 Script executed:
rg "deviceToken :=" bridges/openclaw/gateway_client.go -B 3 -A 3Repository: beeper/agentremote
Length of output: 792
🏁 Script executed:
cat -n bridges/openclaw/gateway_client.go | sed -n '420,500p'Repository: beeper/agentremote
Length of output: 2749
🏁 Script executed:
rg "func \(c \*gatewayWSClient\) Connect" bridges/openclaw/gateway_client.go -A 60Repository: beeper/agentremote
Length of output: 1839
🏁 Script executed:
rg "SessionHistory" bridges/openclaw/ -B 2 -A 2Repository: beeper/agentremote
Length of output: 4601
🏁 Script executed:
rg "\.SessionHistory" bridges/openclaw/ -B 1 -A 1Repository: beeper/agentremote
Length of output: 1719
🏁 Script executed:
grep -r "httpBearerAuthToken" bridges/openclaw/gateway_client_test.goRepository: beeper/agentremote
Length of output: 44
🏁 Script executed:
cat -n bridges/openclaw/gateway_client_test.go | head -150Repository: beeper/agentremote
Length of output: 6195
🏁 Script executed:
rg "buildConnectParams" bridges/openclaw/gateway_client.go -A 40 | head -50Repository: beeper/agentremote
Length of output: 1585
🏁 Script executed:
cat -n bridges/openclaw/gateway_client_test.go | sed -n '102,142p'Repository: beeper/agentremote
Length of output: 1878
🏁 Script executed:
rg "func \(c \*gatewayWSClient\) buildConnectParams" bridges/openclaw/gateway_client.go -A 50Repository: beeper/agentremote
Length of output: 1755
🏁 Script executed:
cat -n bridges/openclaw/gateway_client_test.go | sed -n '144,200p'Repository: beeper/agentremote
Length of output: 881
🏁 Script executed:
rg "Password" bridges/openclaw/gateway_client.go | head -20Repository: beeper/agentremote
Length of output: 257
🏁 Script executed:
cat -n bridges/openclaw/gateway_client.go | sed -n '960,1010p'Repository: beeper/agentremote
Length of output: 1866
🏁 Script executed:
rg "Password.*string" bridges/openclaw/gateway_client.goRepository: beeper/agentremote
Length of output: 81
🏁 Script executed:
rg "NewGateway" bridges/openclaw/gateway_client.go -A 10Repository: beeper/agentremote
Length of output: 44
🏁 Script executed:
rg "gatewayWSClient" bridges/openclaw/gateway_client.go | head -20Repository: beeper/agentremote
Length of output: 1702
🏁 Script executed:
rg "gatewayWSClient\(" bridges/openclaw/ -B 2 -A 2Repository: beeper/agentremote
Length of output: 44
🏁 Script executed:
cat -n bridges/openclaw/gateway_client.go | sed -n '402,420p'Repository: beeper/agentremote
Length of output: 834
Remove the Password fallback from httpBearerAuthToken() and persist the websocket device token.
After login via websocket, the connection receives a device token that should be used for HTTP API authentication. Currently, the device token is parsed and returned from Connect() but never stored. This causes subsequent SessionHistory() calls to fall back to Password as the Bearer token, exposing the raw password credential in HTTP headers. The fix is to store the parsed device token back to the config during Connect() and remove the password fallback from httpBearerAuthToken(), replacing it with an empty string.
🔐 Proposed fix
func (c *gatewayWSClient) Connect(ctx context.Context) (string, error) {
...
deviceToken := parseHelloDeviceToken(res.Payload)
+ if deviceToken != "" {
+ c.cfg.DeviceToken = deviceToken
+ }
c.readStarted.Store(true)
go c.readLoop()
go c.pingLoop()
return deviceToken, nil
} func (c *gatewayWSClient) httpBearerAuthToken() string {
if token := strings.TrimSpace(c.cfg.Token); token != "" {
return token
}
if deviceToken := strings.TrimSpace(c.cfg.DeviceToken); deviceToken != "" {
return deviceToken
}
- return strings.TrimSpace(c.cfg.Password)
+ return ""
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bridges/openclaw/gateway_client.go` around lines 1068 - 1076, The
httpBearerAuthToken() function currently falls back to cfg.Password and also
DeviceToken is never persisted from Connect(); update Connect() to save the
parsed device token back into c.cfg.DeviceToken (assign the token after parsing
and before returning) so future HTTP calls use it, and change
gatewayWSClient.httpBearerAuthToken() to only return cfg.Token or
cfg.DeviceToken (trimmed) and otherwise return an empty string (remove the
Password fallback) so raw password is never used as a Bearer token.
Multiple enhancements and fixes across bridges and runtime: - agentremote: add ApprovalFlow.PendingIDs to list pending approval IDs. - bridges/codex: remove readOnlyAccess from sandbox policy and update tests accordingly. - bridges/dummybridge: introduce many demo limits (chars, steps, tools, collections, delays, durations, etc.), add validation helpers, enforce limits during parsing and runtime, and add tests (oversized input rejection and runRandom duration expiry). Also add an advancing runtime helper for tests. - bridges/openclaw (client/gateway/manager/events): - Improve gateway hello parsing and expose Hello() on ws client; parse device token via parseGatewayHello. - Add compatibility reporting (openClawGatewayCompatibilityReport) and validation of required methods/events; ProbeSessionHistory to probe the history HTTP endpoint. - Implement SessionHistory HTTP handling with better decoding and error semantics (including not_found handling) and new helper methods for building/performing requests. - Add ListPendingApprovals RPC wrapper and GetBackfillMaxBatchCount stub. - Manager: add history cache (TTL, max entries), functions to load/cache/prefetch backward history pages, invalidate cache, seed background backfill tasks, sorted session listing, and rehydrate pending approvals from the gateway (including expiration of local approvals). Wire compatibility checks and background backfill seeding into Start/Stop. - Ensure portal metadata defaults for BackgroundBackfillStatus in events. - Other: updates to various tests and small supporting functions (cloning history payloads, helper validators, etc.). These changes add validation and robustness for demo commands, improve OpenClaw gateway compatibility handling and history/backfill performance, and strengthen approval synchronization with upstream gateway.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
turns/session.go (1)
279-315:⚠️ Potential issue | 🟠 MajorLet started sessions fall back to the stored target event ID.
Start(ctx, targetEventID)recordss.targetEventID, but this helper only re-reads params. If a caller starts the session beforeGetTargetEventIDbegins returning that value,FlushPending()and laterEmitPart()calls keep seeing""and the queue never drains.Suggested change
func (s *StreamSession) currentTargetEventID(ctx context.Context) (id.EventID, error) { if s == nil { return "", context.Canceled } if s.params.GetTargetEventID != nil { if eventID := s.params.GetTargetEventID(); eventID != "" { return eventID, nil } } + s.streamMu.Lock() + startedTarget := s.targetEventID + started := s.streamStarted + s.streamMu.Unlock() + if started && startedTarget != "" { + return startedTarget, nil + } target := StreamTarget{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@turns/session.go` around lines 279 - 315, currentTargetEventID should fall back to the session's stored s.targetEventID set by Start when params.GetTargetEventID doesn't yet return a value; update StreamSession.currentTargetEventID to consult s.targetEventID (after nil/empty checks on s.params.GetTargetEventID) and return it if non-empty before proceeding to ResolveTargetEventID or treating target as absent, ensuring you preserve existing locking and resolvedTargetIDs cache behavior used by FlushPending/EmitPart.bridges/openclaw/manager.go (1)
733-738:⚠️ Potential issue | 🟠 MajorInvalidate cached history when a session reset succeeds.
After Line 734 succeeds, the old pages stay in
historyCacheuntil TTL expiry because cache invalidation only happens on chat events. That means a just-reset room can still backfill cleared transcript entries for up to 45 seconds.🩹 Proposed fix
case "reset": if err := gateway.ResetSession(ctx, sessionKey); err != nil { return true, err } + m.invalidateHistoryCache(sessionKey) m.client.sendSystemNoticeViaPortal(ctx, msg.Portal, "OpenClaw session reset.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/manager.go` around lines 733 - 738, After a successful gateway.ResetSession(ctx, sessionKey) call in the "reset" case, invalidate the cached history for that session so old pages cannot be backfilled; call the historyCache invalidation method (e.g., historyCache.Delete(sessionKey) or m.historyCache.Delete(sessionKey) depending on where it's defined) immediately after ResetSession returns and before sending the system notice via m.client.sendSystemNoticeViaPortal(ctx, msg.Portal, ...).
♻️ Duplicate comments (4)
cmd/internal/selfhost/registration.go (1)
94-119:⚠️ Potential issue | 🔴 CriticalFail before any delete call when username discovery is unavailable or malformed.
This branch still isn't fail-closed. If
beeperWhoamireturns(nil, nil), Line 97 panics; if it returns an error or an empty username, the hungry appservice delete is skipped but Line 113 still deletes the Beeper bridge, which can orphan the remote appservice before verification runs.💡 Proposed fix
func DeleteRemoteBridge(ctx context.Context, auth beeperauth.Config, saveAuth func(beeperauth.Config) error, beeperName string) error { if auth.Username == "" { who, err := beeperWhoami(auth.Domain, auth.Token) - if err == nil { - auth.Username = who.UserInfo.Username - if saveAuth != nil { - if err := saveAuth(auth); err != nil { - fmt.Fprintf(os.Stderr, "warning: failed to save auth config: %v\n", err) - } - } + if err != nil { + return fmt.Errorf("failed to resolve username for remote bridge deletion: %w", err) + } + if who == nil || who.UserInfo.Username == "" { + return fmt.Errorf("failed to resolve username for remote bridge deletion: username unavailable") + } + auth.Username = who.UserInfo.Username + if saveAuth != nil { + if err := saveAuth(auth); err != nil { + fmt.Fprintf(os.Stderr, "warning: failed to save auth config: %v\n", err) + } } } - if auth.Username != "" { - hc := hungryNewClient(auth.Domain, auth.Username, auth.Token) - deleteCtx, cancel := context.WithTimeout(ctx, 30*time.Second) - defer cancel() - if err := hc.DeleteAppService(deleteCtx, beeperName); err != nil && !isRemoteNotFoundError(err) { - fmt.Fprintf(os.Stderr, "warning: failed to delete appservice: %v\n", err) - } + hc := hungryNewClient(auth.Domain, auth.Username, auth.Token) + deleteCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + if err := hc.DeleteAppService(deleteCtx, beeperName); err != nil && !isRemoteNotFoundError(err) { + fmt.Fprintf(os.Stderr, "warning: failed to delete appservice: %v\n", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/internal/selfhost/registration.go` around lines 94 - 119, Ensure we fail-fast when username discovery fails or returns malformed data: after calling beeperWhoami(auth.Domain, auth.Token) in the block that populates auth.Username, check for both a non-nil error and whether the returned who is nil or who.UserInfo.Username is empty; if any of these conditions occur, return an error instead of proceeding. Only proceed to create the hungry client (hungryNewClient) and call hc.DeleteAppService when auth.Username is reliably set; do not call beeperapi.DeleteBridge or waitForRemoteBridgeDeletion if username discovery failed. Add clear error returns describing the failed username discovery so the function exits before any deletion calls.bridges/openclaw/gateway_client.go (3)
649-649:⚠️ Potential issue | 🟠 MajorDon't pre-escape
sessionKeybefore assigningURL.Path.Line 649 writes
url.PathEscape(...)intoURL.Path.url.URL.String()escapes path segments itself, so keys likeagent:main:testbecomeagent%253Amain%253Atest, and the history request goes to the wrong endpoint.🩹 Proposed fix
- base.Path = strings.TrimRight(base.Path, "/") + "/sessions/" + url.PathEscape(strings.TrimSpace(sessionKey)) + "/history" + base.Path = strings.TrimRight(base.Path, "/") + "/sessions/" + strings.TrimSpace(sessionKey) + "/history"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/gateway_client.go` at line 649, The code is pre-escaping sessionKey into URL.Path causing double-escaping (e.g., "agent:main:test" -> "agent%253Amain%253Atest"); update the assignment that sets base.Path so it uses the raw, trimmed sessionKey (sessionKey after strings.TrimSpace) appended to "/sessions/<key>/history" without calling url.PathEscape, letting url.URL.String() perform proper escaping; locate the statement that assigns base.Path (the current line using url.PathEscape) and remove the url.PathEscape call so URL.Path contains the unescaped segment.
503-507:⚠️ Potential issue | 🟠 MajorPersist the device token and stop using
cfg.Passwordas a Bearer token.Lines 503-507 parse the device token but never write it back into the live client config, so Lines 1174-1181 fall through to
cfg.Passwordfor every HTTP history/probe call on this connection. That leaks the raw password intoAuthorizationheaders and can also make compatibility probing fail against gateways that expect the device token instead.🔐 Proposed fix
deviceToken := parseHelloDeviceToken(res.Payload) + if deviceToken != "" { + c.cfg.DeviceToken = deviceToken + } c.readStarted.Store(true) go c.readLoop() go c.pingLoop() return deviceToken, nil } @@ func (c *gatewayWSClient) httpBearerAuthToken() string { if token := strings.TrimSpace(c.cfg.Token); token != "" { return token } if deviceToken := strings.TrimSpace(c.cfg.DeviceToken); deviceToken != "" { return deviceToken } - return strings.TrimSpace(c.cfg.Password) + return "" }Also applies to: 1174-1181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/gateway_client.go` around lines 503 - 507, After parsing the device token with parseHelloDeviceToken, persist it into the client's live config (e.g., set c.cfg.DeviceToken or c.cfg.BearerToken / a dedicated c.deviceToken field) instead of leaving only cfg.Password; then update the HTTP auth header construction used by the history/probe requests (the code currently falling back to cfg.Password for Authorization) to prefer the persisted device token field and only fallback to cfg.Password if no device token is available, ensuring the raw password is not sent as a Bearer token.
1184-1189:⚠️ Potential issue | 🟠 MajorKeep the function argument authoritative for
key.Lines 1186-1188 seed
params["key"]and then let the patch body overwrite it. A caller can accidentally or maliciously retargetsessions.patchby including akeyentry inpatch.🩹 Proposed fix
func buildPatchSessionParams(key string, patch map[string]any) map[string]any { params := make(map[string]any, len(patch)+1) params["key"] = strings.TrimSpace(key) for patchKey, patchValue := range patch { + if patchKey == "key" { + continue + } params[patchKey] = patchValue } return params }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/gateway_client.go` around lines 1184 - 1189, The current buildPatchSessionParams function seeds params["key"] from the function argument then copies entries from patch, allowing patch["key"] to overwrite it; change the implementation so the function argument is authoritative by either copying patch into params first and then setting params["key"] = strings.TrimSpace(key), or by skipping any patchKey == "key" when copying; update buildPatchSessionParams accordingly so sessions.patch cannot be retargeted by a key in the patch map.
🧹 Nitpick comments (3)
bridges/dummybridge/runtime_test.go (2)
332-380: Consider extracting the terminal state assertion into a helper.Both
TestRunLoremErrorSetsTerminalErrorStateandTestRunLoremAbortSetsTerminalAbortStateshare identical logic for extracting and asserting the terminal state fromUIMessage. A shared helper would reduce duplication:♻️ Suggested helper extraction
func assertTerminalState(t *testing.T, turn *bridgesdk.Turn, expectedType string) { t.Helper() ui := turn.UIState().UIMessage metadata, _ := ui["metadata"].(map[string]any) terminal, _ := metadata["beeper_terminal_state"].(map[string]any) if terminal["type"] != expectedType { t.Fatalf("expected %s terminal state, got %#v", expectedType, terminal) } }Then the tests simplify to:
- ui := turn.UIState().UIMessage - metadata, _ := ui["metadata"].(map[string]any) - terminal, _ := metadata["beeper_terminal_state"].(map[string]any) - if terminal["type"] != "error" { - t.Fatalf("expected error terminal state, got %#v", terminal) - } + assertTerminalState(t, turn, "error")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/dummybridge/runtime_test.go` around lines 332 - 380, Extract the duplicated UI terminal-state check into a t.Helper named assertTerminalState and call it from TestRunLoremErrorSetsTerminalErrorState and TestRunLoremAbortSetsTerminalAbortState; implement assertTerminalState(t *testing.T, turn *bridgesdk.Turn, expectedType string) to call t.Helper(), read ui := turn.UIState().UIMessage, get metadata := ui["metadata"].(map[string]any) and terminal := metadata["beeper_terminal_state"].(map[string]any) and fail with t.Fatalf if terminal["type"] != expectedType, then replace the duplicated extraction/assertion blocks in runLorem tests with calls to assertTerminalState(t, turn, "error") and assertTerminalState(t, turn, "abort").
57-63: Consider adding a brief comment documenting the nil parameters.The function passes
nilforloginandportal, and an emptyEventSender{}. While this works for the current test scenarios, a brief comment explaining that these are intentionally nil (because the tested code paths don't access them) would help future maintainers understand the constraints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/dummybridge/runtime_test.go` around lines 57 - 63, Add a brief comment in newTestTurn explaining why nil is passed for the login and portal parameters and why an empty bridgev2.EventSender{} is used (i.e., these fields are intentionally nil/empty because the test's code paths don't access them), so future maintainers understand the constraint; place the comment near the call to bridgesdk.NewConversation (or above newTestTurn) and reference the intent for conv.StartTurn and dummySDKAgent usage.sdk/stream_part_state.go (1)
104-108: Consider nil receiver checks on accessors for consistency.
ApplyPartandapplyPartTimestampguard against nil receivers, but accessor methods likeVisibleText()andAccumulatedText()will panic if called on a nil*StreamPartState. If nil receivers are expected in calling code, consider adding guards or documenting the requirement.Example nil-safe accessor
-func (s *StreamPartState) VisibleText() string { return s.visible.String() } +func (s *StreamPartState) VisibleText() string { + if s == nil { + return "" + } + return s.visible.String() +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/stream_part_state.go` around lines 104 - 108, Accessor methods VisibleText, AccumulatedText and LastVisibleText can panic on a nil *StreamPartState receiver (ApplyPart and applyPartTimestamp already guard against nil); update these accessors on the StreamPartState type to check for a nil receiver (s == nil) and return a safe zero value (e.g., empty string) instead of dereferencing, or alternatively add a clear comment documenting that callers must not pass nil—make the change for VisibleText, AccumulatedText and LastVisibleText consistently to match ApplyPart/applyPartTimestamp behavior.
🤖 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/openclaw/gateway_client.go`:
- Around line 617-627: The probe currently treats any 404 with an empty
gatewaySessionHistoryResponse as compatible; instead, when statusCode ==
http.StatusNotFound decode the response into the errPayload struct and only set
report.HistoryEndpointOK = true if errPayload.Error.Type == "not_found" (handle
JSON decode errors as non-matching). Keep the existing fallback for checking
history (gatewaySessionHistoryResponse and history.Messages) but do not accept
generic 404s—use the errPayload.Error.Type check to explicitly detect the
"not_found" semantic before returning report.
In `@bridges/openclaw/manager.go`:
- Around line 769-781: The branches that call markBackgroundBackfillError (e.g.,
after loadAllHistoryMessages and the other failing branches around 787-790)
return before persisting the portal state, so the "failed"
BackgroundBackfillError never gets saved; update those error paths to call
Portal.Save() (via the same save flow used at the end of the method) immediately
after markBackgroundBackfillError (before returning) so failures are persisted —
locate usages around loadAllHistoryMessages and the other early-return branches
and add the save step after markBackgroundBackfillError to ensure the portal
state is written.
- Around line 128-145: The compatibility whitelist is missing runtime RPCs the
manager actually uses; add "exec.approval.resolve" to
openClawRequiredGatewayMethods (since DeliverDecision calls it) and add
"agent.wait" to openClawRequiredGatewayMethods (used during run recovery) so the
fail-fast check correctly rejects gateways that lack these RPCs; update the
openClawRequiredGatewayMethods slice accordingly and ensure references to
DeliverDecision and the run recovery code paths remain consistent.
- Around line 1165-1179: The loop fetching pages via gateway.SessionHistory uses
cursor and NextCursor but doesn't detect when NextCursor stops advancing; modify
the loop in manager.go (around cursor, pages, gateway.SessionHistory,
history.HasMore, history.NextCursor) to track the previous cursor (e.g.,
prevCursor) and after receiving history check if
strings.TrimSpace(history.NextCursor) == strings.TrimSpace(prevCursor) (or equal
to current cursor) and if so break the loop to avoid infinite spinning; also
update prevCursor (or cursor) only when NextCursor advances and keep the
existing HasMore and empty-NextCursor checks.
In `@cmd/internal/selfhost/registration.go`:
- Around line 149-158: remoteBridgeDeleted currently initializes bridgeGone=true
and treats a nil who (from beeperWhoami) as "gone"; change it to treat nil who
as unverifiable by initializing bridgeGone=false (or returning an error) and
explicitly handling who==nil: if who == nil return false, false,
fmt.Errorf("unable to verify remote bridge deletion: beeperWhoami returned nil")
(or set bridgeGone=false and let caller retry). Update the function
remoteBridgeDeleted (and the local variables bridgeGone and who and the
beeperWhoami call) so only a non-nil who with an absent beeperName in
who.User.Bridges sets bridgeGone=true.
In `@stream_turn_host.go`:
- Around line 74-83: The code currently calls the bridge callback
h.callbacks.GetAborter while holding h.mu (inside the loop over h.states), which
can deadlock or block; change the logic to first lock h.mu, snapshot the
relevant states (e.g., copy non-nil entries from h.states into a local slice or
collect pointers/IDs), then unlock h.mu and only then iterate that snapshot to
call h.callbacks.GetAborter and append returned Aborter objects to aborters;
ensure you still skip nil states and nil Aborter results and preserve the
original semantics of the Aborter collection.
In `@turns/session.go`:
- Around line 148-156: The stream is being marked started before the publisher
has actually been registered, causing the alreadyStarted fast path to skip
registration later; update the Start/tryStart logic so that s.streamStarted is
only set true after publisher registration succeeds: ensure that when
publisherAvailable is true but roomID == "" you do not call s.tryStart in a way
that flips streamStarted, and change tryStart/Register flow so Register (or the
successful Descriptor() result) is a prerequisite for setting streamStarted;
specifically modify calls around s.Descriptor, s.tryStart, and the code path
that sets streamStarted so retries with the same targetEventID will still
attempt Register once roomID becomes available.
---
Outside diff comments:
In `@bridges/openclaw/manager.go`:
- Around line 733-738: After a successful gateway.ResetSession(ctx, sessionKey)
call in the "reset" case, invalidate the cached history for that session so old
pages cannot be backfilled; call the historyCache invalidation method (e.g.,
historyCache.Delete(sessionKey) or m.historyCache.Delete(sessionKey) depending
on where it's defined) immediately after ResetSession returns and before sending
the system notice via m.client.sendSystemNoticeViaPortal(ctx, msg.Portal, ...).
In `@turns/session.go`:
- Around line 279-315: currentTargetEventID should fall back to the session's
stored s.targetEventID set by Start when params.GetTargetEventID doesn't yet
return a value; update StreamSession.currentTargetEventID to consult
s.targetEventID (after nil/empty checks on s.params.GetTargetEventID) and return
it if non-empty before proceeding to ResolveTargetEventID or treating target as
absent, ensuring you preserve existing locking and resolvedTargetIDs cache
behavior used by FlushPending/EmitPart.
---
Duplicate comments:
In `@bridges/openclaw/gateway_client.go`:
- Line 649: The code is pre-escaping sessionKey into URL.Path causing
double-escaping (e.g., "agent:main:test" -> "agent%253Amain%253Atest"); update
the assignment that sets base.Path so it uses the raw, trimmed sessionKey
(sessionKey after strings.TrimSpace) appended to "/sessions/<key>/history"
without calling url.PathEscape, letting url.URL.String() perform proper
escaping; locate the statement that assigns base.Path (the current line using
url.PathEscape) and remove the url.PathEscape call so URL.Path contains the
unescaped segment.
- Around line 503-507: After parsing the device token with
parseHelloDeviceToken, persist it into the client's live config (e.g., set
c.cfg.DeviceToken or c.cfg.BearerToken / a dedicated c.deviceToken field)
instead of leaving only cfg.Password; then update the HTTP auth header
construction used by the history/probe requests (the code currently falling back
to cfg.Password for Authorization) to prefer the persisted device token field
and only fallback to cfg.Password if no device token is available, ensuring the
raw password is not sent as a Bearer token.
- Around line 1184-1189: The current buildPatchSessionParams function seeds
params["key"] from the function argument then copies entries from patch,
allowing patch["key"] to overwrite it; change the implementation so the function
argument is authoritative by either copying patch into params first and then
setting params["key"] = strings.TrimSpace(key), or by skipping any patchKey ==
"key" when copying; update buildPatchSessionParams accordingly so sessions.patch
cannot be retargeted by a key in the patch map.
In `@cmd/internal/selfhost/registration.go`:
- Around line 94-119: Ensure we fail-fast when username discovery fails or
returns malformed data: after calling beeperWhoami(auth.Domain, auth.Token) in
the block that populates auth.Username, check for both a non-nil error and
whether the returned who is nil or who.UserInfo.Username is empty; if any of
these conditions occur, return an error instead of proceeding. Only proceed to
create the hungry client (hungryNewClient) and call hc.DeleteAppService when
auth.Username is reliably set; do not call beeperapi.DeleteBridge or
waitForRemoteBridgeDeletion if username discovery failed. Add clear error
returns describing the failed username discovery so the function exits before
any deletion calls.
---
Nitpick comments:
In `@bridges/dummybridge/runtime_test.go`:
- Around line 332-380: Extract the duplicated UI terminal-state check into a
t.Helper named assertTerminalState and call it from
TestRunLoremErrorSetsTerminalErrorState and
TestRunLoremAbortSetsTerminalAbortState; implement assertTerminalState(t
*testing.T, turn *bridgesdk.Turn, expectedType string) to call t.Helper(), read
ui := turn.UIState().UIMessage, get metadata := ui["metadata"].(map[string]any)
and terminal := metadata["beeper_terminal_state"].(map[string]any) and fail with
t.Fatalf if terminal["type"] != expectedType, then replace the duplicated
extraction/assertion blocks in runLorem tests with calls to
assertTerminalState(t, turn, "error") and assertTerminalState(t, turn, "abort").
- Around line 57-63: Add a brief comment in newTestTurn explaining why nil is
passed for the login and portal parameters and why an empty
bridgev2.EventSender{} is used (i.e., these fields are intentionally nil/empty
because the test's code paths don't access them), so future maintainers
understand the constraint; place the comment near the call to
bridgesdk.NewConversation (or above newTestTurn) and reference the intent for
conv.StartTurn and dummySDKAgent usage.
In `@sdk/stream_part_state.go`:
- Around line 104-108: Accessor methods VisibleText, AccumulatedText and
LastVisibleText can panic on a nil *StreamPartState receiver (ApplyPart and
applyPartTimestamp already guard against nil); update these accessors on the
StreamPartState type to check for a nil receiver (s == nil) and return a safe
zero value (e.g., empty string) instead of dereferencing, or alternatively add a
clear comment documenting that callers must not pass nil—make the change for
VisibleText, AccumulatedText and LastVisibleText consistently to match
ApplyPart/applyPartTimestamp behavior.
🪄 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: fb51a607-70d5-4c48-a105-5f60c3930aa1
📒 Files selected for processing (24)
approval_flow.gobridges/codex/client.gobridges/codex/client_test.gobridges/dummybridge/runtime.gobridges/dummybridge/runtime_test.gobridges/openclaw/client.gobridges/openclaw/events.gobridges/openclaw/gateway_client.gobridges/openclaw/manager.gobridges/openclaw/metadata.gobridges/openclaw/status.gobridges/openclaw/stream.gobridges/openclaw/stream_test.gocmd/internal/selfhost/registration.gocmd/internal/selfhost/registration_test.gosdk/stream_part_state.gosdk/stream_part_state_test.gosdk/stream_replay.gosdk/stream_replay_test.gosdk/turn.gosdk/turn_test.gostream_turn_host.goturns/session.goturns/session_target_test.go
✅ Files skipped from review due to trivial changes (6)
- sdk/stream_part_state_test.go
- bridges/codex/client_test.go
- sdk/stream_replay_test.go
- sdk/turn_test.go
- bridges/dummybridge/runtime.go
- bridges/openclaw/stream.go
🚧 Files skipped from review as they are similar to previous changes (6)
- bridges/openclaw/stream_test.go
- bridges/openclaw/metadata.go
- bridges/openclaw/client.go
- sdk/turn.go
- approval_flow.go
- sdk/stream_replay.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-docker
- GitHub Check: Lint
- GitHub Check: Lint
- GitHub Check: build-docker
🧰 Additional context used
🧬 Code graph analysis (4)
bridges/dummybridge/runtime_test.go (5)
sdk/types.go (3)
ToolApprovalResponse(94-98)ApprovalRequest(101-108)ApprovalHandle(111-115)sdk/turn.go (1)
Turn(103-146)bridges/dummybridge/config.go (1)
Config(16-19)sdk/runtime.go (1)
NewConversation(68-78)pkg/shared/streamui/emitter.go (1)
UIState(14-36)
turns/session_target_test.go (1)
turns/session.go (4)
NewStreamSession(78-83)StreamSessionParams(35-51)EndReasonFinish(23-23)ErrNoPublisher(30-30)
bridges/openclaw/manager.go (3)
approval_decision.go (2)
ApprovalResolutionOriginAgent(24-24)ApprovalResolutionOriginFromString(45-54)bridges/openclaw/metadata.go (1)
PortalMetadata(26-91)sdk/stream_replay.go (2)
NewUIStateReplayer(14-19)UIStateReplayer(10-12)
turns/session.go (1)
pkg/matrixevents/matrixevents.go (4)
StreamEventMessageType(14-14)BuildStreamEventEnvelope(74-105)StreamEventOpts(68-71)BuildStreamEventTxnID(107-113)
🔇 Additional comments (15)
stream_turn_host.go (1)
87-89: Good lock boundary before aborting turns.Calling
Aborton Lines 87-89 after releasingh.muis the right pattern to prevent lock contention and re-entrant deadlocks.bridges/dummybridge/runtime_test.go (8)
1-15: LGTM!Imports are appropriate for the test functionality. The use of
math/randwith explicit seeding is acceptable for deterministic test scenarios.
17-31: LGTM!The mock correctly implements the
ApprovalHandleinterface with configurable approval state for testing denial scenarios.
33-55: LGTM!Good test doubles for controlling timing behavior. The
advancingRuntimepattern effectively simulates time progression for duration-based tests.
65-87: LGTM!Helper functions provide clean abstractions for part lookup with appropriate nil-return semantics.
89-163: LGTM!Comprehensive command parsing tests with good coverage of edge cases. The table-driven test for oversized inputs effectively validates all demo input limits.
165-215: LGTM!Good snapshot-based verification of emitted artifacts. The test correctly validates both the presence of expected parts and the absence of transient data in the persistent snapshot.
217-252: LGTM!Clean test for the approval denial flow. The mock handler setup is straightforward and the assertion on
"output-denied"state validates the expected behavior.
278-305: LGTM!Effective use of
advancingRuntimeto verify duration-based stopping. The test correctly sets up a scenario where the delay exceeds the duration, ensuring only one sleep call occurs.sdk/stream_part_state.go (4)
34-54: Whitespace preservation for deltas is correctly implemented.The previous review concern about
TrimSpacecorrupting delta payloads has been addressed. The code now correctly uses the rawstringValue(part["delta"])and only skips truly empty strings.
80-102: Timestamp precedence logic is correct.The method correctly applies minimum semantics for start/token times and maximum semantics for completion time, ensuring accurate tracking across potentially out-of-order part deliveries.
154-167: Helper functions are clean and correctly implemented.
timestampMillisandtrimDefaultencapsulate fallback logic nicely, keeping the main methods readable.
19-78: ✓stringValuehelper is defined insdk/turn_data.go(lines 251-254) and is properly accessible throughout thesdkpackage. No action needed.cmd/internal/selfhost/registration_test.go (1)
14-39: Good regression coverage for the fail-closed path.This cleanly locks in the intended behavior and the
t.Cleanuprestoration keeps the global test seam contained.turns/session_target_test.go (1)
12-324: Good regression coverage for the new stream lifecycle.The new publisher double plus the delayed-target, delayed-publisher, and hook-only cases give solid coverage for the queue/flush paths most likely to regress in this refactor.
Propagate reply/thread targets through finalization and into the SDK payloads. Replace the previous replyTo pointer with a ReplyTarget argument in sendFinalAssistantTurnContent and add buildFinalEditPayload to construct FinalEditPayloads that include ReplyTo and ThreadRoot. In the SDK, add buildPayloadRelatesTo and apply it when building final edits so edited content carries proper relates_to (thread + in-reply-to) relations. Also refactor gateway client hello handling into applyHelloPayload (persisting deviceToken into client config) and ensure buildPatchSessionParams does not allow patch to override the method key. Add unit tests covering the new payload builder, relates_to preservation, patch key reservation, and hello payload device token persistence. Minor import adjustments and small cleanup included.
Guard against nil state when generating link previews and normalize many error strings in dummybridge (remove trailing periods for consistency). Remove unused gatewayHistoryResponse struct, fix variable declaration in openclaw manager pagination to avoid shadowing, and remove unused atomic/runtimeFallback and Turn.roomID to clean up unused code and fix potential compiler issues.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
turns/session.go (1)
176-184:⚠️ Potential issue | 🟠 Major
streamStartedis set even when publisher registration is skipped.When
publisheris available butroomIDis still empty (e.g., room not yet synced), registration is skipped at line 177, yetstreamStartedis unconditionally set totrueat line 182. A subsequentStart()call with the sametargetEventIDwill hit the early-return at line 173-174, permanently skipping registration.Consider only setting
streamStarted = truewhen either:
- Registration succeeded (
publisher != nil && roomID != "" && descriptor != nil), or- Hook-only mode is active (no publisher configured)
🐛 Proposed fix
func (s *StreamSession) tryStart( ctx context.Context, publisher bridgev2.BeeperStreamPublisher, roomID id.RoomID, targetEventID id.EventID, descriptor *event.BeeperStreamInfo, ) (alreadyStarted bool, pendingCount int, err error) { s.streamMu.Lock() defer s.streamMu.Unlock() pendingCount = len(s.pendingParts) if s.streamStarted && s.targetEventID == targetEventID { return true, pendingCount, nil } + registered := false if publisher != nil && roomID != "" && descriptor != nil { err = publisher.Register(ctx, roomID, targetEventID, descriptor) if err != nil { return false, pendingCount, err } + registered = true + } + // Only mark started if registration succeeded or hook-only mode (no publisher) + if registered || publisher == nil { + s.streamStarted = true + s.targetEventID = targetEventID } - s.streamStarted = true - s.targetEventID = targetEventID return false, pendingCount, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@turns/session.go` around lines 176 - 184, The code currently sets s.streamStarted = true unconditionally even when publisher registration was skipped; update the logic in the Start (or the function containing the shown block) so s.streamStarted is only set when registration actually happened or when we are in hook-only mode: set s.streamStarted = true if (publisher != nil && roomID != "" && descriptor != nil && Register succeeded) OR if publisher == nil (hook-only mode), otherwise leave s.streamStarted false so a future Start() can attempt registration once roomID/descriptor are available; reference the existing variables/conditions publisher, roomID, descriptor, the Register call, and s.streamStarted to locate and change the assignment accordingly.bridges/ai/response_finalization.go (1)
102-102:⚠️ Potential issue | 🟠 MajorPropagate one
ReplyTargetthrough the whole finalization flow.
buildFinalEditPayloadnow takes all relation metadata fromreplyTarget, but Lines 102 and 344 passReplyTarget{}and the overflow path still emits continuations withstate.replyTarget. That means simple/heartbeat edits can lose their reply/thread context, and a long natural-mode reply can split across different targets after directive retargeting.Suggested fix
- oc.sendFinalAssistantTurnContent(ctx, portal, state, meta, cleanedRaw, rendered, ReplyTarget{}, "simple") + oc.sendFinalAssistantTurnContent(ctx, portal, state, meta, cleanedRaw, rendered, state.replyTarget, "simple") … - oc.sendFinalAssistantTurnContent(ctx, portal, state, meta, cleaned, rendered, ReplyTarget{}, "heartbeat") + oc.sendFinalAssistantTurnContent(ctx, portal, state, meta, cleaned, rendered, state.replyTarget, "heartbeat") … - oc.sendContinuationMessage(ctx, portal, chunk, state.replyTarget, state.nextMessageTiming()) + oc.sendContinuationMessage(ctx, portal, chunk, replyTarget, state.nextMessageTiming())Also applies to: 344-344, 530-542, 546-583
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/ai/response_finalization.go` at line 102, The finalization flow is dropping reply/thread context by constructing empty ReplyTarget{} in calls like oc.sendFinalAssistantTurnContent and elsewhere; update the flow to propagate a single ReplyTarget (use the existing state.replyTarget or a local replyTarget variable) through buildFinalEditPayload, sendFinalAssistantTurnContent, and any continuation/overflow emission paths so every simple/heartbeat edit and long-mode continuation uses the same ReplyTarget; adjust function signatures (e.g., sendFinalAssistantTurnContent, buildFinalEditPayload) and call sites to accept and forward that ReplyTarget instead of creating ReplyTarget{} and replace occurrences that emit continuations from state.replyTarget with the propagated replyTarget.
🧹 Nitpick comments (2)
turns/session.go (1)
279-282: Consider usingErrClosedinstead ofcontext.Canceledfor nil receiver.For consistency with other nil-receiver checks in this file that return
ErrClosed, consider returningErrClosedhere as well.♻️ Suggested change
func (s *StreamSession) currentTargetEventID(ctx context.Context) (id.EventID, error) { if s == nil { - return "", context.Canceled + return "", ErrClosed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@turns/session.go` around lines 279 - 282, The nil-receiver check in StreamSession.currentTargetEventID returns context.Canceled but other nil checks in this file use ErrClosed; change the early-return to return ErrClosed instead of context.Canceled. Locate the currentTargetEventID method on StreamSession and replace the return value for the s == nil branch with ErrClosed so it matches the file's nil-receiver convention.bridges/openclaw/manager.go (1)
1078-1108: Simple LRU-style eviction for history cache.The cache eviction scans all entries to find the oldest by
CreatedAt. While O(n), this is acceptable for a small cache (max 128 entries). Consider a proper LRU structure if the cache grows larger in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bridges/openclaw/manager.go` around lines 1078 - 1108, storeBackwardHistoryPage currently implements a simple eviction by scanning m.historyCache for the oldest CreatedAt when the size exceeds openClawHistoryCacheMaxEntries; keep this behavior but make the intent explicit and safe: ensure storeBackwardHistoryPage on adding to m.historyCache (in function storeBackwardHistoryPage) locks m.mu, clones the history via cloneGatewaySessionHistory, sets CreatedAt/ExpiresAt using openClawHistoryCacheTTL, then if len(m.historyCache) > openClawHistoryCacheMaxEntries iterate m.historyCache to find the oldest entry by CreatedAt and delete it; keep the found flag logic to avoid deleting when empty. This preserves the O(n) eviction for the small fixed-size cache while making the eviction condition and locking explicit for future refactor to an LRU structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@bridges/ai/response_finalization.go`:
- Line 102: The finalization flow is dropping reply/thread context by
constructing empty ReplyTarget{} in calls like oc.sendFinalAssistantTurnContent
and elsewhere; update the flow to propagate a single ReplyTarget (use the
existing state.replyTarget or a local replyTarget variable) through
buildFinalEditPayload, sendFinalAssistantTurnContent, and any
continuation/overflow emission paths so every simple/heartbeat edit and
long-mode continuation uses the same ReplyTarget; adjust function signatures
(e.g., sendFinalAssistantTurnContent, buildFinalEditPayload) and call sites to
accept and forward that ReplyTarget instead of creating ReplyTarget{} and
replace occurrences that emit continuations from state.replyTarget with the
propagated replyTarget.
In `@turns/session.go`:
- Around line 176-184: The code currently sets s.streamStarted = true
unconditionally even when publisher registration was skipped; update the logic
in the Start (or the function containing the shown block) so s.streamStarted is
only set when registration actually happened or when we are in hook-only mode:
set s.streamStarted = true if (publisher != nil && roomID != "" && descriptor !=
nil && Register succeeded) OR if publisher == nil (hook-only mode), otherwise
leave s.streamStarted false so a future Start() can attempt registration once
roomID/descriptor are available; reference the existing variables/conditions
publisher, roomID, descriptor, the Register call, and s.streamStarted to locate
and change the assignment accordingly.
---
Nitpick comments:
In `@bridges/openclaw/manager.go`:
- Around line 1078-1108: storeBackwardHistoryPage currently implements a simple
eviction by scanning m.historyCache for the oldest CreatedAt when the size
exceeds openClawHistoryCacheMaxEntries; keep this behavior but make the intent
explicit and safe: ensure storeBackwardHistoryPage on adding to m.historyCache
(in function storeBackwardHistoryPage) locks m.mu, clones the history via
cloneGatewaySessionHistory, sets CreatedAt/ExpiresAt using
openClawHistoryCacheTTL, then if len(m.historyCache) >
openClawHistoryCacheMaxEntries iterate m.historyCache to find the oldest entry
by CreatedAt and delete it; keep the found flag logic to avoid deleting when
empty. This preserves the O(n) eviction for the small fixed-size cache while
making the eviction condition and locking explicit for future refactor to an LRU
structure.
In `@turns/session.go`:
- Around line 279-282: The nil-receiver check in
StreamSession.currentTargetEventID returns context.Canceled but other nil checks
in this file use ErrClosed; change the early-return to return ErrClosed instead
of context.Canceled. Locate the currentTargetEventID method on StreamSession and
replace the return value for the s == nil branch with ErrClosed so it matches
the file's nil-receiver convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2671660b-956f-4a57-a9dd-ee8859702d87
📒 Files selected for processing (13)
bridges/ai/response_finalization.gobridges/ai/response_finalization_test.gobridges/ai/streaming_init.gobridges/dummybridge/agent.gobridges/dummybridge/runtime.gobridges/openclaw/gateway_client.gobridges/openclaw/gateway_client_test.gobridges/openclaw/manager.gobridges/openclaw/manager_test.gosdk/conversation.gosdk/turn.gosdk/turn_test.goturns/session.go
💤 Files with no reviewable changes (1)
- sdk/conversation.go
✅ Files skipped from review due to trivial changes (2)
- bridges/dummybridge/agent.go
- bridges/openclaw/gateway_client_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- bridges/openclaw/manager_test.go
- bridges/dummybridge/runtime.go
- sdk/turn.go
- bridges/ai/streaming_init.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: Lint
- GitHub Check: build-docker
- GitHub Check: build-docker
- GitHub Check: Lint
🧰 Additional context used
🧬 Code graph analysis (3)
sdk/turn_test.go (7)
sdk/turn.go (3)
PlaceholderMessagePayload(38-42)FinalEditPayload(44-49)Turn(103-146)turns/session.go (2)
NewStreamSession(78-83)StreamSessionParams(35-51)sdk/writer.go (1)
Writer(33-41)pkg/shared/citations/citations.go (1)
SourceCitation(11-20)sdk/runtime.go (1)
NewConversation(68-78)sdk/turn_manager.go (1)
TurnConfig(10-22)pkg/shared/streamui/emitter.go (1)
UIState(14-36)
bridges/ai/response_finalization.go (3)
sdk/turn.go (1)
FinalEditPayload(44-49)pkg/shared/citations/citations.go (1)
SourceCitation(11-20)sdk/final_edit.go (1)
BuildDefaultFinalEditTopLevelExtra(58-67)
turns/session.go (1)
pkg/matrixevents/matrixevents.go (4)
StreamEventMessageType(14-14)BuildStreamEventEnvelope(74-105)StreamEventOpts(68-71)BuildStreamEventTxnID(107-113)
🔇 Additional comments (25)
turns/session.go (5)
89-124: LGTM! Descriptor caching now handles transient failures correctly.The mutex-guarded
descriptorLoadedflag pattern properly allows retries on transient failures (e.g., publisher not ready) while caching successful results. The double-checked locking is correctly implemented.
255-277: LGTM!The flush loop correctly handles errors by requeueing failed parts at the front, and the separate
flushMuproperly serializes flush operations while allowing concurrent enqueuing.
373-404: LGTM!The delta-key selection logic correctly prioritizes the descriptor type over the default stream type, and the hook-first pattern properly allows transport overrides to consume events before falling back to the publisher.
413-446: LGTM!The variadic key-value logging helper safely handles pairs with proper type dispatch and a fallback to
Interfacefor unsupported types.
201-226: LGTM!The
Endmethod correctly uses atomic CAS for idempotent teardown, properly guards state reads with the mutex, and ensuresClearTurnGateis called via defer.sdk/turn_test.go (8)
22-41: LGTM!Clean mock implementation of
BeeperStreamPublisherthat captures registration state for test assertions.
317-367: LGTM!The test correctly validates that stream parts are buffered until target resolution completes. The polling pattern with a 500ms deadline is reasonable for async behavior verification.
369-443: LGTM!Comprehensive tests for final edit construction covering both
m.replacerelation targeting and thread/reply relation preservation.
445-468: LGTM!The test correctly verifies that
awaitStreamStartterminates promptly when encountering a permanent error (missing publisher).
583-618: LGTM!The idle timeout tests effectively verify both the timeout-triggered abort and the timer reset behavior on writer activity.
663-686: LGTM!Thorough test validating that
Writer().Start()triggers exactly one placeholder send and correctly updates all related turn state (started flag, UI state, network message ID).
688-697: LGTM!Clean test helper that allows callers to assert on
turn.endedstate with appropriate timeout handling.
470-581: LGTM!Excellent test coverage for final edit body defaulting rules across different turn types (visible text, artifact-only, metadata-only, error, suppressed).
bridges/openclaw/gateway_client.go (5)
606-617: Probe still doesn't validatenot_founderror type.The
errPayloadstruct is declared but never populated or used for checking the error type. The code accepts any 404 with emptyMessagesas compatible, but should only accept 404s with an expliciterror.type == "not_found"to avoid false positives from generic proxy 404s.🩹 Proposed fix
if statusCode == http.StatusNotFound { - var errPayload struct { - Error struct { - Type string `json:"type,omitempty"` - } `json:"error,omitempty"` - } - if history != nil && len(history.Messages) == 0 { + if strings.Contains(strings.ToLower(report.HistoryEndpointError), "not_found") { report.HistoryEndpointOK = true return report } - _ = errPayload }
1163-1171: Password fallback inhttpBearerAuthToken()exposes raw credentials.This method falls back to returning the raw password as a Bearer token when neither
TokennorDeviceTokenare set. This is a security concern as the password credential should not be used as a Bearer token in HTTP headers.🔐 Proposed fix
func (c *gatewayWSClient) httpBearerAuthToken() string { if token := strings.TrimSpace(c.cfg.Token); token != "" { return token } if deviceToken := strings.TrimSpace(c.cfg.DeviceToken); deviceToken != "" { return deviceToken } - return strings.TrimSpace(c.cfg.Password) + return "" }
674-676: Good fallback fromItemstoMessages.The fallback logic correctly populates
MessagesfromItemswhenMessagesis empty, supporting both response formats from the gateway.
1185-1197: Device token persistence now implemented.The
applyHelloPayloadfunction correctly persists the device token toc.cfg.DeviceToken, addressing the previous concern about HTTP API authentication using the device token received during websocket connect.
520-530: Defensive cloning inHello()accessor looks good.The accessor properly clones the
Features.MethodsandFeatures.Eventsslices to prevent callers from mutating the internal state.bridges/openclaw/manager.go (7)
128-145: Missing required gateway methods for runtime operations.The
openClawRequiredGatewayMethodslist is missingexec.approval.resolve(used byDeliverDecisionat line 110) andagent.wait(used bywaitForRunCompletionat line 2343). A gateway missing these methods will pass startup validation but fail during normal operation.🩹 Proposed fix
openClawRequiredGatewayMethods = []string{ "sessions.list", "sessions.patch", "sessions.resolve", "chat.send", "chat.abort", "agents.list", "models.list", "agent.identity.get", "exec.approval.list", + "exec.approval.resolve", + "agent.wait", }
769-791: Background backfill errors not persisted before returning.Both error branches call
markBackgroundBackfillErrorwhich setsmeta.BackgroundBackfillStatus = "failed", but then return before reachingportal.Save(ctx)at line 821. The failed status is never persisted, leaving the portal stuck in the previous state.🩹 Proposed fix
if params.Forward || params.AnchorMessage != nil || cursorMode == openClawForwardHistoryCursorPrefix { allMessages, loadErr := m.loadAllHistoryMessages(ctx, gateway, meta.OpenClawSessionKey) if loadErr != nil { m.markBackgroundBackfillError(params.Portal, meta, params.Task, loadErr) + if params.Task != nil && params.Portal != nil { + _ = params.Portal.Save(ctx) + } return nil, loadErr } @@ } else { history, historyErr := m.loadBackwardHistoryPage(ctx, gateway, meta.OpenClawSessionKey, normalizeHistoryLimit(params.Count), formatOpenClawBackwardCursor(cursorSeq), params.Task == nil) if historyErr != nil { m.markBackgroundBackfillError(params.Portal, meta, params.Task, historyErr) + if params.Task != nil && params.Portal != nil { + _ = params.Portal.Save(ctx) + } return nil, historyErr }
1164-1180: Infinite loop risk ifNextCursorstops advancing.The loop assumes each page returns a new cursor. If the gateway replies with
HasMore=truebut the sameNextCursor, this loop will spin indefinitely until context cancellation.🩹 Proposed fix
func (m *openClawManager) loadAllHistoryMessages(ctx context.Context, gateway *gatewayWSClient, sessionKey string) ([]map[string]any, error) { cursor := "" pages := make([][]map[string]any, 0, 4) for { history, err := gateway.SessionHistory(ctx, sessionKey, openClawMaxHistoryPageLimit, cursor) if err != nil { return nil, err } if history == nil || len(history.Messages) == 0 { break } pages = append(pages, history.Messages) - if !history.HasMore || strings.TrimSpace(history.NextCursor) == "" { + nextCursor := strings.TrimSpace(history.NextCursor) + if !history.HasMore || nextCursor == "" || nextCursor == cursor { break } - cursor = history.NextCursor + cursor = nextCursor }
249-273: Session sync now fully replaces the map.The updated
syncSessionscorrectly replaces the entirem.sessionsmap with a fresh one, preventing stale session entries from persisting.
316-341: Good approval rehydration on startup.The
rehydratePendingApprovalsfunction correctly syncs local approval state with the gateway on reconnect, expiring any locally-tracked approvals that no longer exist upstream.
2666-2734: Clean refactor to useUIStateReplayer.The history chunk application now uses
bridgesdk.NewUIStateReplayer(state)consistently, improving code clarity and alignment with the SDK's replay API.
1736-1756: Polling loop with bounded timeout looks reasonable.The
sendApprovalPromptWhenReadyfunction waits up to 350ms for approval context to be attached before sending the prompt. The timer is properly stopped on context cancellation, and the bounded deadline prevents indefinite waiting.
Several fixes and tests across components: - bridges/dummybridge: add assertTerminalState helper and use it in tests to reduce duplication. - bridges/openclaw: accept semantic `not_found` errors from history endpoint, add error payload field, avoid double-escaping session path, and robustly handle repeating cursors; add tests for history probing and cursor loop prevention. Invalidate history cache and persist portal state after fetch errors/fetches. - openclaw manager: include additional required gateway methods (exec.approval.resolve, agent.wait). - stream host/session: collect Aborters outside lock to avoid deadlocks and add test. Make StreamPartState accessors nil-safe and add corresponding tests. - turns/session: require room ID when a publisher is present, ensure proper register/retry flow and use started target event as fallback; add tests and adjust publisher test helper to track register calls. - cmd/selfhost registration: improve error handling for username discovery and nil responses and update test expectations. These changes fix correctness issues (deadlocks, looped history fetches, URL/path escaping), add defensive checks, and expand test coverage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.