Add OpenClaw bridge runtime and provisioning support#7
Open
batuhan wants to merge 57 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new OpenClaw Beeper plugin/package with connector/runtime/CLI, extensive bridge dispatch/provisioning/types updates, streaming/approval helpers, persistent registry/rooms, and native Go AI-run lifecycle/streams, with tests and configs across packages. ChangesOpenClaw Beeper Channel + Bridge Integration
Sequence Diagram(s)(skipped) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
added 6 commits
May 27, 2026 02:40
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/openclaw/src/backfill.ts (1)
278-293:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
"agent"transcript roles as agent-authored messages.The new runtime normalizes assistant history entries to
role: "agent", but this mapper only treats"assistant"and"tool"as agent output. Those records will be backfilled as human-authoredm.noticeevents instead of agent messages.Suggested fix
function normalizeHistoryMessage(message: OpenClawChatHistoryMessage, index: number): OpenClawBackfillMessage { const role = typeof message.role === "string" ? message.role : "assistant"; const text = contentText(message.content); + const isAgentSender = role === "assistant" || role === "agent" || role === "tool"; + const isAgentText = role === "assistant" || role === "agent"; const normalized: OpenClawBackfillMessage = { content: { body: text || JSON.stringify(message.content ?? message), - msgtype: role === "assistant" ? "m.text" : "m.notice", + msgtype: isAgentText ? "m.text" : "m.notice", "com.beeper.openclaw.backfill": { messageSeq: message.messageSeq ?? index, role, }, }, id: typeof message.id === "string" ? message.id : `history_${index}`, role, - sender: role === "assistant" || role === "tool" ? "agent" : role === "system" ? "system" : "human", + sender: isAgentSender ? "agent" : role === "system" ? "system" : "human", seq: typeof message.messageSeq === "number" ? message.messageSeq : index, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openclaw/src/backfill.ts` around lines 278 - 293, The mapper normalizeHistoryMessage currently treats only "assistant" and "tool" as agent outputs; update it to also consider "agent" as an agent-authored role so those entries are emitted as agent messages. Specifically, in normalizeHistoryMessage adjust the msgtype calculation (now msgtype should be "m.text" for role === "assistant" || role === "agent" || role === "tool") and update the sender assignment to treat role === "agent" the same as "assistant" or "tool" (i.e., set sender to "agent"); preserve existing fallback behavior for "system" and human roles and keep seq/id logic unchanged.
♻️ Duplicate comments (1)
packages/openclaw/src/rooms.ts (1)
90-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse config-aware domain when creating session rooms.
createSessionRoomstill defaults tomatrixDomainFromHomeserver(config.homeserver), which bypassesconfig.homeserverDomainand can generate the service-bot MXID on the wrong domain.🔧 Proposed fix
- const domain = options.domain ?? matrixDomainFromHomeserver(config.homeserver); + const domain = options.domain ?? matrixDomainFromConfig(config);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openclaw/src/rooms.ts` at line 90, The domain fallback in createSessionRoom uses matrixDomainFromHomeserver(config.homeserver) which ignores config.homeserverDomain; change the domain resolution (the const domain in createSessionRoom) to prefer options.domain, then config.homeserverDomain, and only finally call matrixDomainFromHomeserver(config.homeserver) so the service-bot MXID is created on the configured homeserver domain.
🧹 Nitpick comments (1)
packages/openclaw/src/openclaw-runtime.test.ts (1)
154-183: ⚡ Quick winWrap global runtime registration in
try/finally.These tests clean up
setBeeperChannelRuntimeForHost(..., undefined)only on the happy path. A failing assertion can leak global state into later tests.🧪 Suggested pattern
setBeeperChannelRuntimeForHost(hostRuntime, new BeeperChannelRuntime({ client: { beeper: { aiRuns: createTestBeeperAIRuns(), streams: beeperStreams }, media: { upload: vi.fn() }, } as never, userId: "`@sh-openclaw-bot`:example", })); -const runtime = new OpenClawPluginRuntimeAdapter({ - config: createDefaultConfig({ dataDir: "/tmp/openclaw" }), - transport: createOpenClawHostRuntimeAdapter(hostRuntime), -}); - -// assertions... - -setBeeperChannelRuntimeForHost(hostRuntime, undefined); +try { + const runtime = new OpenClawPluginRuntimeAdapter({ + config: createDefaultConfig({ dataDir: "/tmp/openclaw" }), + transport: createOpenClawHostRuntimeAdapter(hostRuntime), + }); + + // assertions... +} finally { + setBeeperChannelRuntimeForHost(hostRuntime, undefined); +}Also applies to: 312-394, 443-483, 550-596
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openclaw/src/openclaw-runtime.test.ts` around lines 154 - 183, The test registers a global runtime via setBeeperChannelRuntimeForHost(hostRuntime, new BeeperChannelRuntime(...)) but only clears it on the happy path; wrap the registration and test actions in a try/finally so cleanup always runs. Specifically, after calling setBeeperChannelRuntimeForHost(...) and before awaiting runtime.sendMessage / assertions (in the test that constructs OpenClawPluginRuntimeAdapter and calls runtime.sendMessage), add a try block for the sendMessage call and all expects, and move the setBeeperChannelRuntimeForHost(hostRuntime, undefined) call into the finally block to guarantee global state is reset even if assertions fail; apply the same pattern to the other similar test regions mentioned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bridge/src/bridge.ts`:
- Around line 1756-1760: The helper `#matchingRemoteTarget` currently ignores the
partId and always returns by index, which causes multipart edits/removals to
target the wrong SentEvent; update `#matchingRemoteTarget`(SentEvent[] existing,
partId, index) to first search existing for an entry whose partId equals the
provided partId and return that SentEvent if found, otherwise fall back to
existing[index] or existing[0]; apply the same change to the other occurrence
that uses the same index-only fallback (the similar logic around the earlier
occurrence referenced in the comment) so both places consistently prefer partId
matching before index fallback.
In `@packages/openclaw/src/approval.ts`:
- Around line 60-67: defaultBeeperApprovalActions currently advertises
"allow_session" and "allow_room" but the resolver
(toOpenClawApprovalResolvePayload) collapses those into always-approve, so the
UI promises a narrower scope than is actually granted; change
defaultBeeperApprovalActions to only expose decisions that the resolver can
preserve (e.g., replace "allow_session" and "allow_room" with a single
"allow_always" and keep "allow_once" and "deny"), update the produced
id/title/reactionKey strings accordingly (use approvalReactionKey and
approvalActionTitle helpers with the new decision names) so UI labels match the
actual serialized decisions and no click will be misrepresented as a broader
grant.
In `@packages/openclaw/src/beeper-channel-config.schema.json`:
- Around line 69-71: The backfillLimit JSON schema currently allows fractional
and negative numbers; update the backfillLimit property (in
beeper-channel-config.schema.json) to enforce non-negative integers by changing
its type from "number" to "integer" and adding "minimum": 0 so only integers >=
0 are valid; keep the existing description unchanged.
In `@packages/openclaw/src/beeper-channel-runtime.ts`:
- Around line 70-95: sendText() and sendMedia() accept threadRoot but never
serialize it into the Matrix event, so messages intended for threads are sent as
top-level events; update sendText (before calling
`#queueRemoteText/withReplyRelation`) and sendMedia (before calling
`#queueRemoteMedia`) to attach an m.thread relation when options.threadRoot is
present (e.g., merge { "m.relates_to": { "m.thread": { "event_id":
options.threadRoot } } } into the content or into the media payload), ensuring
the thread relation is combined with any existing reply relation produced by
withReplyRelation so both reply and thread relations are preserved.
In `@packages/openclaw/src/bridge-agent.test.ts`:
- Line 53: The test calls runtimeWith without the required responses option;
update the test where runtime is constructed (the runtime variable using
runtimeWith) to include a responses property (e.g. responses: {} or the specific
mock response map needed for this test) so it matches the runtimeWith(options: {
responses: Record<string, unknown> }) signature; modify the runtimeWith
invocation (runtimeWith({...})) to pass responses accordingly.
In `@packages/openclaw/src/cli.test.ts`:
- Around line 179-183: The test hardcodes "/tmp/pickle-openclaw-empty" which can
cause flakes; modify the test to create a unique temporary directory (using
os.tmpdir() + fs.mkdtempSync or fs.mkdtemp) and pass that path to runCli instead
of the hardcoded string, referencing the existing test that calls
runCli(["whoami", "--data-dir", ...], io) and using captureIO() for IO;
optionally remove/cleanup the temp dir after the test to avoid residue.
In `@packages/openclaw/src/connector.ts`:
- Around line 204-223: loadUserLogin creates a runtime via
this.#runtimeFactory(this.config) then hands that runtime (indirectly) to
OpenClawNetworkAPI, but `#sendTurn` calls this.#runtimeFactory again on every turn
which can produce a different runtime; change the implementation so the runtime
created in loadUserLogin is reused by `#sendTurn` (e.g., store the runtime
instance created in loadUserLogin on the connector instance and have `#sendTurn`
use that stored runtime instead of calling `#runtimeFactory` again), ensuring
runtime.transport and OpenClawHostRuntimeAdapter logic continues to work as
before.
In `@packages/openclaw/src/integration.test.ts`:
- Around line 135-138: The current assertion checks for a specific payload and
can miss calls to "exec.approval.resolve" with extra fields; change the
assertion to assert on the method name only by using a wildcard for the payload:
replace the
expect(transport.request).not.toHaveBeenCalledWith("exec.approval.resolve", {
... }) check with
expect(transport.request).not.toHaveBeenCalledWith("exec.approval.resolve",
expect.any(Object)) (or expect.anything() if payload may be null/undefined) so
transport.request is never called with the "exec.approval.resolve" method
regardless of payload; update the test around transport.request and keep
reference to the method string "exec.approval.resolve" and the transport.request
mock.
In `@packages/openclaw/src/matrix-parser.ts`:
- Around line 36-39: The code currently always strips Matrix reply fallback
which removes leading '>' lines from ordinary blockquotes; change the logic to
only strip reply fallback when the event is actually a reply by detecting reply
status (use the result of extractMatrixReplyFallback(effectiveText) — e.g.,
check fallback.isReply or fallback.repliedTo — or fall back to checking
messageContent?.m.relates_to?.m.in_reply_to?.event_id). Only call
stripLeadingMatrixMention and stripMatrixHtmlReplyFallback when that
reply-detection is true; otherwise leave body and formattedBody as the original
(stringValue(effectiveText) or stringValue(msg?.event.html)) so non-reply
blockquotes are preserved.
In `@packages/openclaw/src/openclaw-runtime.ts`:
- Around line 348-363: The current events() implementation returns early and
only wires one host hook, dropping other sources (onAgentEvent,
onSessionTranscriptUpdate, runtime.events()/subscribe(), and
this.#localEvents.events). Change it to always collect all applicable
async-iterables: if this.#runtime.events is an object and has onAgentEvent
include agentRuntimeEvents(this.#runtime.events.onAgentEvent, filter); if it has
onSessionTranscriptUpdate include
transcriptUpdateEvents(this.#runtime.events.onSessionTranscriptUpdate, filter);
if this.#runtime.events is a function use it (events = this.#runtime.events)
else fallback to this.#runtime.subscribe and include events(filter) when
present; always include this.#localEvents.events(filter). Finally return
mergeEvents([...allCollectedStreams]) and only fall back to
this.#localEvents.events(filter) when no host streams exist.
- Around line 1520-1523: nextVisibleText currently appends a replacement delta
onto the old buffer (returning previous + delta) when the payload rewrites text;
instead, when the new text is a replacement (i.e., next does not
startWith(previous)), reset the visible-text buffer by returning next directly.
Update the function nextVisibleText to return next in the replacement branch
(instead of previous + delta), keeping the existing checks for empty delta and
the normal extension case (next.startsWith(previous)) intact.
In `@packages/openclaw/src/setup.ts`:
- Around line 186-204: The media and payload send wrappers (media and payload in
setup.ts) do not forward the replyToId through to beeperOutboundAdapter—ensure
that when calling beeperOutboundAdapter.sendMedia and
beeperOutboundAdapter.sendPayload from the media and payload async functions you
include the replyToId (and threadId) fields from ctx in the forwarded context
object; update both the media wrapper and the payload wrapper (and any path
where payload routes through media) to pass ctx.replyToId (and ctx.threadId) so
replied media/payload messages are sent as replies rather than top-level posts.
- Around line 487-500: The advertised actions omit "mark_unread" though
handleAction implements it; update the action set and support checks to include
"mark_unread": add "mark_unread" to the beeperMessageToolActions array (used by
describeMessageTool) and include it in the supportsAction predicate inside
beeperMessageActions (and repeat the same change for the other identical block
later in the file). Ensure you update both describeMessageTool()’s actions list
and supportsAction() to recognize "mark_unread" so callers that pre-flight-check
capabilities won’t reject the action.
- Around line 1094-1105: The finally block in startBeeperGatewayAccount (which
awaits waitForAbort(ctx.abortSignal)) can be skipped because
stopBeeperGatewayAccount stops the bridge but never resolves/aborts
ctx.abortSignal; update the stop path to signal the start wait so cleanup runs:
in stopBeeperGatewayAccount / stopAccount invoke the AbortController tied to
ctx.abortSignal (e.g., ctx.abortController?.abort() or otherwise trigger the
same abort signal) or otherwise resolve the waitForAbort promise before/after
stopping the bridge; ensure you reference and use the same ctx object
(ctx.abortSignal / ctx.abortController) so startedBridges.delete(key) and the
host/runtime cleanup in the startBeeperGatewayAccount finally block always
execute.
In `@packages/pickle/native/internal/core/beeper_ai_run.go`:
- Around line 63-67: When beginning a new Beeper AI run, the code currently
unconditionally overwrites c.beeperAIRuns[run.RunID], dropping in-flight data;
modify the Begin logic around aistream.NewRun / aistream.NewWriter /
writer.Start so it first checks c.beeperAIRuns for an existing entry for
run.RunID and if present rejects the duplicate (return an error or the existing
snapshot via c.marshalBeeperAIRunSnapshot and outboundEventsFromAGUI) instead of
replacing the map entry; ensure you only call NewWriter/Start and insert into
c.beeperAIRuns when no existing runID is found and preserve beeperAIRunState for
concurrent/retry calls.
In `@packages/pickle/native/internal/core/core.go`:
- Line 26: The beeperAIRuns map is not cleared on shutdown; modify the core
shutdown logic in handleClose (in core.go) to reset or reinitialize the
beeperAIRuns field (e.g., assign an empty map or nil) to avoid leaking stale run
state across re-inits; ensure you do this for the same cleanup path referenced
at both occurrences around beeperAIRuns so any existing entries are dropped
during close.
---
Outside diff comments:
In `@packages/openclaw/src/backfill.ts`:
- Around line 278-293: The mapper normalizeHistoryMessage currently treats only
"assistant" and "tool" as agent outputs; update it to also consider "agent" as
an agent-authored role so those entries are emitted as agent messages.
Specifically, in normalizeHistoryMessage adjust the msgtype calculation (now
msgtype should be "m.text" for role === "assistant" || role === "agent" || role
=== "tool") and update the sender assignment to treat role === "agent" the same
as "assistant" or "tool" (i.e., set sender to "agent"); preserve existing
fallback behavior for "system" and human roles and keep seq/id logic unchanged.
---
Duplicate comments:
In `@packages/openclaw/src/rooms.ts`:
- Line 90: The domain fallback in createSessionRoom uses
matrixDomainFromHomeserver(config.homeserver) which ignores
config.homeserverDomain; change the domain resolution (the const domain in
createSessionRoom) to prefer options.domain, then config.homeserverDomain, and
only finally call matrixDomainFromHomeserver(config.homeserver) so the
service-bot MXID is created on the configured homeserver domain.
---
Nitpick comments:
In `@packages/openclaw/src/openclaw-runtime.test.ts`:
- Around line 154-183: The test registers a global runtime via
setBeeperChannelRuntimeForHost(hostRuntime, new BeeperChannelRuntime(...)) but
only clears it on the happy path; wrap the registration and test actions in a
try/finally so cleanup always runs. Specifically, after calling
setBeeperChannelRuntimeForHost(...) and before awaiting runtime.sendMessage /
assertions (in the test that constructs OpenClawPluginRuntimeAdapter and calls
runtime.sendMessage), add a try block for the sendMessage call and all expects,
and move the setBeeperChannelRuntimeForHost(hostRuntime, undefined) call into
the finally block to guarantee global state is reset even if assertions fail;
apply the same pattern to the other similar test regions mentioned.
🪄 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: 5b545f82-e5c0-4fb1-9022-8a2a7a19e9b1
⛔ Files ignored due to path filters (2)
packages/pickle/native/go.sumis excluded by!**/*.sumpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (89)
CONTRIBUTING.mdPLAN_OPENCLAW.mdpackage.jsonpackages/bridge/src/appservice-websocket.test.tspackages/bridge/src/appservice-websocket.tspackages/bridge/src/beeper.test.tspackages/bridge/src/beeper.tspackages/bridge/src/bridge.test.tspackages/bridge/src/bridge.tspackages/bridge/src/provisioning.test.tspackages/bridge/src/provisioning.tspackages/bridge/src/store.test.tspackages/bridge/src/store.tspackages/bridge/src/types.tspackages/openclaw/.npmignorepackages/openclaw/README.mdpackages/openclaw/openclaw.plugin.jsonpackages/openclaw/package.jsonpackages/openclaw/scripts/copy-runtime-assets.mjspackages/openclaw/scripts/sync-manifest-schema.mjspackages/openclaw/src/approval.test.tspackages/openclaw/src/approval.tspackages/openclaw/src/appservice.test.tspackages/openclaw/src/appservice.tspackages/openclaw/src/backfill.test.tspackages/openclaw/src/backfill.tspackages/openclaw/src/beeper-channel-config.schema.jsonpackages/openclaw/src/beeper-channel-runtime.test.tspackages/openclaw/src/beeper-channel-runtime.tspackages/openclaw/src/beeper-setup.test.tspackages/openclaw/src/beeper-setup.tspackages/openclaw/src/beeper-stream.test.tspackages/openclaw/src/beeper-stream.tspackages/openclaw/src/beeper-turn-events.tspackages/openclaw/src/bridge-agent.test.tspackages/openclaw/src/bridge-agent.tspackages/openclaw/src/cli.test.tspackages/openclaw/src/cli.tspackages/openclaw/src/config.test.tspackages/openclaw/src/config.tspackages/openclaw/src/connector.test.tspackages/openclaw/src/connector.tspackages/openclaw/src/ids.tspackages/openclaw/src/integration.test.tspackages/openclaw/src/matrix-parser.tspackages/openclaw/src/openclaw-extension.test.tspackages/openclaw/src/openclaw-extension.tspackages/openclaw/src/openclaw-identity.tspackages/openclaw/src/openclaw-runtime.test.tspackages/openclaw/src/openclaw-runtime.tspackages/openclaw/src/protocol-coverage.test.tspackages/openclaw/src/protocol-coverage.tspackages/openclaw/src/registration.test.tspackages/openclaw/src/registration.tspackages/openclaw/src/registry.test.tspackages/openclaw/src/registry.tspackages/openclaw/src/rooms.test.tspackages/openclaw/src/rooms.tspackages/openclaw/src/setup-entry.tspackages/openclaw/src/setup.test.tspackages/openclaw/src/setup.tspackages/openclaw/src/types.tspackages/openclaw/tsdown.config.tspackages/pickle/native/go.modpackages/pickle/native/internal/core/appservice.gopackages/pickle/native/internal/core/appservice_test.gopackages/pickle/native/internal/core/beeper_ai_run.gopackages/pickle/native/internal/core/beeper_ai_run_test.gopackages/pickle/native/internal/core/core.gopackages/pickle/native/internal/core/messages.gopackages/pickle/native/internal/core/operations.gopackages/pickle/native/internal/core/persistent_crypto_load.gopackages/pickle/native/internal/core/persistent_crypto_methods.gopackages/pickle/native/internal/core/persistent_crypto_snapshot.gopackages/pickle/native/internal/core/persistent_crypto_store.gopackages/pickle/package.jsonpackages/pickle/src/beeper/auth.test.tspackages/pickle/src/beeper/auth.tspackages/pickle/src/client-types.tspackages/pickle/src/client.test.tspackages/pickle/src/client.tspackages/pickle/src/generated-runtime-operations.tspackages/pickle/src/generated-runtime-types.tspackages/pickle/src/index.tspackages/pickle/src/runtime-types.tspackages/pickle/src/streams/beeper-message.tspackages/state-file/src/index.test.tspackages/state-file/src/index.tsscripts/audit-package-surface.mjs
💤 Files with no reviewable changes (3)
- packages/pickle/native/internal/core/persistent_crypto_snapshot.go
- packages/pickle/native/internal/core/persistent_crypto_load.go
- packages/pickle/native/internal/core/persistent_crypto_store.go
✅ Files skipped from review due to trivial changes (4)
- CONTRIBUTING.md
- packages/openclaw/.npmignore
- packages/pickle/src/generated-runtime-types.ts
- packages/openclaw/src/protocol-coverage.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/openclaw/README.md
- packages/openclaw/src/registration.test.ts
- packages/openclaw/src/setup-entry.ts
- packages/openclaw/openclaw.plugin.json
- packages/openclaw/tsdown.config.ts
- packages/openclaw/src/beeper-setup.test.ts
- packages/openclaw/src/protocol-coverage.test.ts
- packages/openclaw/src/registry.test.ts
- packages/openclaw/src/registry.ts
- packages/openclaw/src/types.ts
- packages/openclaw/src/rooms.test.ts
- packages/pickle/src/beeper/auth.test.ts
- packages/openclaw/src/beeper-setup.ts
📜 Review details
🧰 Additional context used
🪛 LanguageTool
PLAN_OPENCLAW.md
[style] ~57-~57: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...dinator plus Go-backed AI run bridge. - Replace connector-local /help, /tools, `/mo...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~57-~57: Ensure spelling is correct
Context: ... with OpenClaw SDK command and approval surfaces. - Keep the Pickle bridge/appservice me...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ...oval surfaces. - Keep the Pickle bridge/appservice mechanics for Matrix transport, portals...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ...or Matrix transport, portals, contacts, appservice registration, media, reactions, receipt...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 OpenGrep (1.22.0)
packages/openclaw/src/matrix-parser.ts
[ERROR] 136-136: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 149-149: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
packages/openclaw/src/openclaw-runtime.ts
[ERROR] 1650-1650: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (43)
packages/bridge/src/store.ts (1)
141-147: LGTM!packages/openclaw/scripts/sync-manifest-schema.mjs (1)
1-17: LGTM!packages/openclaw/scripts/copy-runtime-assets.mjs (1)
1-19: LGTM!packages/state-file/src/index.test.ts (1)
1-1: LGTM!Also applies to: 30-41
packages/openclaw/src/beeper-channel-config.schema.json (1)
1-68: LGTM!Also applies to: 72-88
packages/pickle/package.json (1)
66-67: LGTM!packages/pickle/native/internal/core/operations.go (1)
72-81: LGTM!packages/bridge/src/beeper.test.ts (1)
53-54: LGTM!Also applies to: 114-137
packages/pickle/native/go.mod (1)
6-8: LGTM!Also applies to: 16-18, 23-27, 29-31
packages/pickle/src/index.ts (1)
38-43: LGTM!packages/openclaw/src/openclaw-identity.ts (1)
5-33: LGTM!packages/state-file/src/index.ts (1)
1-2: LGTM!Also applies to: 61-64, 77-83
packages/openclaw/src/ids.ts (1)
1-9: LGTM!package.json (1)
18-18: LGTM!packages/pickle/src/client.ts (1)
89-95: LGTM!packages/pickle/native/internal/core/persistent_crypto_methods.go (1)
74-74: LGTM!packages/openclaw/src/openclaw-extension.ts (1)
1-3: LGTM!Also applies to: 5-22, 26-28
packages/openclaw/src/approval.test.ts (1)
3-4: LGTM!Also applies to: 35-142, 203-245
packages/bridge/src/store.test.ts (1)
1-29: LGTM!packages/bridge/src/types.ts (1)
163-165: LGTM!Also applies to: 653-656, 894-902, 1044-1046, 1102-1102
packages/openclaw/src/openclaw-extension.test.ts (1)
3-3: LGTM!Also applies to: 20-27, 35-44, 48-65, 80-90, 95-98, 112-119, 131-136, 147-165, 167-200, 204-228
packages/openclaw/src/config.test.ts (1)
5-7: LGTM!Also applies to: 9-17, 22-25, 27-32, 58-72, 74-87, 93-93
packages/pickle/src/client.test.ts (1)
918-919: LGTM!Also applies to: 960-1000, 1043-1047, 1081-1082, 1095-1096
packages/bridge/src/beeper.ts (1)
115-117: LGTM!Also applies to: 125-126, 130-130, 139-139
packages/pickle/src/runtime-types.ts (1)
28-28: LGTM!Also applies to: 31-33, 37-37, 46-46, 58-58
packages/pickle/src/beeper/auth.ts (1)
134-140: LGTM!packages/openclaw/src/appservice.test.ts (1)
5-7: LGTM!Also applies to: 29-29, 31-33, 54-86, 88-128, 130-255, 276-298
packages/bridge/src/provisioning.ts (1)
14-16: LGTM!Also applies to: 28-32, 62-70, 181-206, 283-326
packages/pickle/native/internal/core/beeper_ai_run_test.go (1)
11-193: LGTM!packages/openclaw/package.json (1)
175-178: Verifyopenclawpublish output bundles@beeper/pickle*despite moving them todevDependencies.
@beeper/pickle*are inpackages/openclaw/package.jsondevDependenciesonly, and the source uses runtime value imports/re-exports from:
@beeper/pickle-bridge(e.g.,src/connector.ts,src/beeper-channel-runtime.ts)@beeper/pickle-ag-ui(e.g.,src/beeper-turn-events.ts)
packages/openclaw/tsdown.config.tssetsdeps.alwaysBundle: [/^@beeper\//], so the build is intended to bundle@beeper/*intodist. However,packages/openclaw/distisn’t present in this snapshot, so the “no bare imports left indist/*.mjs” part can’t be checked here—re-run after building and confirmdistcontains no remaining@beeper/pickle*bare imports (or move required packages todependencies/peerDependenciesif any remain).packages/openclaw/src/beeper-stream.test.ts (1)
3-353: LGTM!packages/bridge/src/bridge.test.ts (1)
36-1423: LGTM!packages/pickle/src/client-types.ts (1)
81-159: LGTM!packages/openclaw/src/cli.ts (1)
4-189: LGTM!packages/openclaw/src/beeper-channel-runtime.test.ts (1)
1-285: LGTM!packages/pickle/src/generated-runtime-operations.ts (1)
5-5: LGTM!Also applies to: 19-20, 25-25, 32-32, 44-44, 132-136, 310-328
packages/openclaw/src/bridge-agent.ts (1)
2-2: LGTM!Also applies to: 7-7, 23-24, 28-30, 33-33, 52-56, 59-59, 79-80
packages/pickle/src/streams/beeper-message.ts (1)
50-50: LGTM!Also applies to: 75-82, 89-90, 108-108, 125-125, 167-167, 177-177, 255-257, 263-267, 269-276, 303-303, 310-310, 320-320, 339-359, 393-395
packages/bridge/src/appservice-websocket.test.ts (1)
19-19: LGTM!Also applies to: 25-25, 58-58, 66-73
packages/openclaw/src/registration.ts (1)
13-17: LGTM!Also applies to: 26-29, 33-35, 38-45, 48-49, 52-53, 62-62, 68-83
packages/openclaw/src/backfill.test.ts (1)
1-3: LGTM!Also applies to: 7-7, 17-17, 39-50, 55-56, 82-82, 95-97, 104-104, 153-159, 165-178, 190-193, 222-224, 234-241, 329-363, 365-395, 397-517, 520-531
packages/pickle/native/internal/core/appservice_test.go (1)
13-13: LGTM!Also applies to: 43-51, 109-168, 275-333, 335-419, 545-560
packages/openclaw/src/appservice.ts (1)
1-9: LGTM!Also applies to: 11-16, 24-24, 30-30, 40-55, 63-67, 71-110, 112-138, 158-158, 162-185, 193-199, 201-241
Comment on lines
+69
to
+71
| "backfillLimit": { | ||
| "type": "number", | ||
| "description": "Maximum OpenClaw messages to backfill per imported session." |
There was a problem hiding this comment.
Constrain backfillLimit to non-negative integers.
backfillLimit currently accepts fractional and negative values. For a message-count limit, schema should enforce integer >= 0 to prevent invalid runtime inputs.
Suggested schema change
- "backfillLimit": {
- "type": "number",
- "description": "Maximum OpenClaw messages to backfill per imported session."
- },
+ "backfillLimit": {
+ "type": "integer",
+ "minimum": 0,
+ "description": "Maximum OpenClaw messages to backfill per imported session."
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "backfillLimit": { | |
| "type": "number", | |
| "description": "Maximum OpenClaw messages to backfill per imported session." | |
| "backfillLimit": { | |
| "type": "integer", | |
| "minimum": 0, | |
| "description": "Maximum OpenClaw messages to backfill per imported session." |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/openclaw/src/beeper-channel-config.schema.json` around lines 69 -
71, The backfillLimit JSON schema currently allows fractional and negative
numbers; update the backfillLimit property (in
beeper-channel-config.schema.json) to enforce non-negative integers by changing
its type from "number" to "integer" and adding "minimum": 0 so only integers >=
0 are valid; keep the existing description unchanged.
Member
Author
|
@indent review |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 2: The root package.json currently duplicates the workspace package
identity by having "name": "`@beeper/openclaw`" (same as
packages/openclaw/package.json); remove or change the root "name" to a distinct
value (or drop it entirely) and ensure "private": true remains, so pnpm
workspace selection isn’t ambiguous; leave the existing "scripts.test:go"
delegating to "pnpm --filter `@beeper/pickle` test:go" unchanged.
🪄 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: 6cb480b5-b73c-4f11-b391-cca54d7c8e45
📒 Files selected for processing (5)
package.jsonpackages/openclaw/README.mdpackages/openclaw/package.jsonpackages/openclaw/scripts/copy-runtime-assets.mjspackages/openclaw/src/openclaw-extension.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/openclaw/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/openclaw/scripts/copy-runtime-assets.mjs
- packages/openclaw/src/openclaw-extension.test.ts
📜 Review details
🔇 Additional comments (2)
package.json (1)
18-18: LGTM!packages/openclaw/package.json (1)
2-2: LGTM!Also applies to: 147-148
|
Review posted on the banner above. Highlights: Functional
Latent
Nit
|
|
Re-checked after Resolved
Updated
Still valid (unchanged in these commits)
No new issues introduced by the refactor that I can confirm. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/bridge/src/types.ts (1)
511-559:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDon't require
roomStateuntilRuntimeBridgeimplements it.This new required field is the direct cause of the current typecheck failure:
RuntimeBridgeno longer satisfiesPickleBridge. Add theroomStateimplementation in the same PR or keep this property optional until the runtime is wired up.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bridge/src/types.ts` around lines 511 - 559, The new required readonly roomState on the PickleBridge interface is breaking RuntimeBridge; make roomState optional until the runtime implements it by changing the declaration to readonly roomState?: BridgeRoomStateAPI so existing RuntimeBridge types pass, or alternatively implement BridgeRoomStateAPI (get and set) on the RuntimeBridge class (methods matching BridgeRoomStateAPI) in this PR and remove the optionality; update any consumers that assume roomState is non-null accordingly.packages/pickle/native/internal/core/messages.go (1)
118-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist the thread root with the stream state.
A stream started in a thread forgets its
ThreadRootEventIDafter registration. Later non-direct carrier events are sent with only anm.reference, so live updates escape the thread and appear as top-level room messages. Store the thread root here and reapply it when publishing carrier events.Also applies to: 166-172
♻️ Duplicate comments (2)
packages/bridge/src/bridge.ts (1)
1822-1832:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMatch edit targets by
partId, not index fallback.This still routes multipart edits through
#matchingRemoteTarget(...), which ignorespartIdand picks by index. If part ordering changes, the wrong Matrix event gets edited/redacted.🛠️ Minimal fix
- `#matchingRemoteTarget`(existing: SentEvent[], partId: string | undefined, index: number): SentEvent | undefined { - if (partId) { - return existing[index] ?? existing[0]; - } - return existing[index] ?? existing[0]; + `#matchingRemoteTarget`( + existing: { db: Message[]; sent: SentEvent[] }, + partId: string | undefined, + index: number, + ): SentEvent | undefined { + if (partId) { + const partIndex = existing.db.findIndex((message) => message.partId === partId); + if (partIndex >= 0) return existing.sent[partIndex]; + } + return existing.sent[index] ?? existing.sent[0]; } @@ - return this.#matchingRemoteTarget(existing.sent, partId, index); + return this.#matchingRemoteTarget(existing, partId, index);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bridge/src/bridge.ts` around lines 1822 - 1832, The converted edit routing falls back to `#matchingRemoteTarget` which ignores partId and may pick by index; update the `#convertedEditPartTarget` method to prefer finding a SentEvent in existing.sent whose raw/metadata contains the same partId (e.g., sent.raw?.partId or sent.raw?.metadata?.partId) or whose eventId equals partId, and return that SentEvent; only if no match is found, fall back to using existing.sent[index] or existing.sent[0] as the last resort. Keep references to the method name `#convertedEditPartTarget` and the helper `#matchingRemoteTarget` so reviewers can locate the change.packages/openclaw/src/registry.test.ts (1)
8-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClean up the temporary registry directory.
This test still leaves the
mkdtemp(...)directory behind on every run. Please wrap the body intry/finallyand removedirso repeated local/CI runs do not accumulate temp files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openclaw/src/registry.test.ts` around lines 8 - 39, Wrap the test body for the "persists agent contacts, session bindings, and dedupe keys" case in a try/finally so the temporary directory created by mkdtemp (variable dir) is always removed; after the existing await registry.save() and assertions (or in finally) remove the dir (e.g., fs.rmSync(dir, { recursive: true, force: true }) or await fs.promises.rm(dir, { recursive: true, force: true })) to ensure no temp files remain, keeping the rest of the test (path, registry, loaded, assertions) unchanged.
🧹 Nitpick comments (2)
packages/openclaw/src/beeper-channel-config.schema.json (1)
3-14: ⚡ Quick winReject unknown channel config keys.
With
"additionalProperties": true, typos like"beeprEnv"will still validate here and then be silently ignored bycreateDefaultConfig, which falls back to defaults instead of surfacing the mistake. Tightening this schema makes config errors fail fast.Suggested change
- "additionalProperties": true, + "additionalProperties": false,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openclaw/src/beeper-channel-config.schema.json` around lines 3 - 14, The schema currently allows unknown keys because "additionalProperties" is true; change it to "additionalProperties": false so unknown channel config keys (e.g., typos like "beeprEnv") fail validation; keep the existing "properties" block for "enabled" and "beeperEnv" intact and optionally add a "required" array if you want to enforce presence of specific keys, but the essential fix is flipping additionalProperties to false to reject unrecognized keys.packages/openclaw/src/connector.ts (1)
718-724: 💤 Low value
outboundActivityPatchappears unused.The function is defined but never called in this file.
inboundActivityPatch()is used at line 331, but there's no corresponding call tooutboundActivityPatch(). If outbound activity tracking is intended, the function should be wired in; otherwise, consider removing dead code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openclaw/src/connector.ts` around lines 718 - 724, outboundActivityPatch is defined but never used; either wire it into the outbound-event code path (mirror where inboundActivityPatch() is invoked — e.g., call outboundActivityPatch() from the same caller that updates outbound events such as the send/dispatch/transport emit handler that updates activity timestamps) so the OpenClawBridgeActivityPatch fields (lastOutboundAt/lastTransportActivityAt) get updated, or remove the unused outboundActivityPatch function and its OpenClawBridgeActivityPatch-only helper if outbound tracking is not required; reference the outboundActivityPatch and inboundActivityPatch functions and the code locations that update activity timestamps to guide the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bridge/src/beeper-stream.ts`:
- Around line 217-235: The catch currently wraps both event-processing and the
finalization call in runBeeperTurnStream so a failure inside
options.stream.finalize() triggers a second terminal write with EVENT_RUN_ERROR;
restructure by separating concerns: wrap the for-await loop and the map/publish
logic (toAsyncIterable, options.mapEvent, options.stream.publishMany) in a
try/catch that on error writes a terminalPart RUN_ERROR via
options.stream.finalize(...) and rethrows, then perform the normal finalize call
(options.stream.finalize(options.finishReason ? { finishReason:
options.finishReason } : {})) in its own try/without the RUN_ERROR fallback so
any finalize failures are propagated without issuing a second terminal write;
ensure you do not call finalize twice for the same runId/turnId.
---
Outside diff comments:
In `@packages/bridge/src/types.ts`:
- Around line 511-559: The new required readonly roomState on the PickleBridge
interface is breaking RuntimeBridge; make roomState optional until the runtime
implements it by changing the declaration to readonly roomState?:
BridgeRoomStateAPI so existing RuntimeBridge types pass, or alternatively
implement BridgeRoomStateAPI (get and set) on the RuntimeBridge class (methods
matching BridgeRoomStateAPI) in this PR and remove the optionality; update any
consumers that assume roomState is non-null accordingly.
---
Duplicate comments:
In `@packages/bridge/src/bridge.ts`:
- Around line 1822-1832: The converted edit routing falls back to
`#matchingRemoteTarget` which ignores partId and may pick by index; update the
`#convertedEditPartTarget` method to prefer finding a SentEvent in existing.sent
whose raw/metadata contains the same partId (e.g., sent.raw?.partId or
sent.raw?.metadata?.partId) or whose eventId equals partId, and return that
SentEvent; only if no match is found, fall back to using existing.sent[index] or
existing.sent[0] as the last resort. Keep references to the method name
`#convertedEditPartTarget` and the helper `#matchingRemoteTarget` so reviewers can
locate the change.
In `@packages/openclaw/src/registry.test.ts`:
- Around line 8-39: Wrap the test body for the "persists agent contacts, session
bindings, and dedupe keys" case in a try/finally so the temporary directory
created by mkdtemp (variable dir) is always removed; after the existing await
registry.save() and assertions (or in finally) remove the dir (e.g.,
fs.rmSync(dir, { recursive: true, force: true }) or await fs.promises.rm(dir, {
recursive: true, force: true })) to ensure no temp files remain, keeping the
rest of the test (path, registry, loaded, assertions) unchanged.
---
Nitpick comments:
In `@packages/openclaw/src/beeper-channel-config.schema.json`:
- Around line 3-14: The schema currently allows unknown keys because
"additionalProperties" is true; change it to "additionalProperties": false so
unknown channel config keys (e.g., typos like "beeprEnv") fail validation; keep
the existing "properties" block for "enabled" and "beeperEnv" intact and
optionally add a "required" array if you want to enforce presence of specific
keys, but the essential fix is flipping additionalProperties to false to reject
unrecognized keys.
In `@packages/openclaw/src/connector.ts`:
- Around line 718-724: outboundActivityPatch is defined but never used; either
wire it into the outbound-event code path (mirror where inboundActivityPatch()
is invoked — e.g., call outboundActivityPatch() from the same caller that
updates outbound events such as the send/dispatch/transport emit handler that
updates activity timestamps) so the OpenClawBridgeActivityPatch fields
(lastOutboundAt/lastTransportActivityAt) get updated, or remove the unused
outboundActivityPatch function and its OpenClawBridgeActivityPatch-only helper
if outbound tracking is not required; reference the outboundActivityPatch and
inboundActivityPatch functions and the code locations that update activity
timestamps to guide the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 041c42a6-fdda-418a-961b-13759fddf15c
⛔ Files ignored due to path filters (1)
packages/pickle/native/go.sumis excluded by!**/*.sum
📒 Files selected for processing (64)
AGENTS.mdpackage.jsonpackages/bridge/package.jsonpackages/bridge/src/beeper-stream.test.tspackages/bridge/src/beeper-stream.tspackages/bridge/src/bridge.test.tspackages/bridge/src/bridge.tspackages/bridge/src/events.tspackages/bridge/src/index.tspackages/bridge/src/media-message.test.tspackages/bridge/src/media-message.tspackages/bridge/src/types.tspackages/bridge/tsdown.config.tspackages/openclaw/README.mdpackages/openclaw/openclaw.plugin.jsonpackages/openclaw/package.jsonpackages/openclaw/src/appservice.test.tspackages/openclaw/src/appservice.tspackages/openclaw/src/beeper-channel-config.schema.jsonpackages/openclaw/src/beeper-channel-runtime.test.tspackages/openclaw/src/beeper-channel-runtime.tspackages/openclaw/src/beeper-setup.test.tspackages/openclaw/src/beeper-setup.tspackages/openclaw/src/beeper-turn-events.tspackages/openclaw/src/bridge-agent.test.tspackages/openclaw/src/bridge-agent.tspackages/openclaw/src/cli.test.tspackages/openclaw/src/cli.tspackages/openclaw/src/config.test.tspackages/openclaw/src/config.tspackages/openclaw/src/connector.test.tspackages/openclaw/src/connector.tspackages/openclaw/src/integration.test.tspackages/openclaw/src/openclaw-extension.test.tspackages/openclaw/src/openclaw-runtime.test.tspackages/openclaw/src/openclaw-runtime.tspackages/openclaw/src/protocol-coverage.tspackages/openclaw/src/registration.test.tspackages/openclaw/src/registration.tspackages/openclaw/src/registry.test.tspackages/openclaw/src/registry.tspackages/openclaw/src/rooms.test.tspackages/openclaw/src/rooms.tspackages/openclaw/src/setup.test.tspackages/openclaw/src/setup.tspackages/openclaw/src/types.tspackages/openclaw/tsdown.config.tspackages/openclaw/vitest.config.tspackages/pickle/native/go.modpackages/pickle/native/internal/core/appservice_test.gopackages/pickle/native/internal/core/beeper_ai_run.gopackages/pickle/native/internal/core/beeper_ai_run_test.gopackages/pickle/native/internal/core/core.gopackages/pickle/native/internal/core/messages.gopackages/pickle/native/internal/core/operations.gopackages/pickle/src/client-types.tspackages/pickle/src/client.test.tspackages/pickle/src/client.tspackages/pickle/src/generated-runtime-operations.tspackages/pickle/src/generated-runtime-types.tspackages/pickle/src/index.tspackages/pickle/src/runtime-types.tsscripts/openclaw-crabpot-full-test.mjstsconfig.base.json
💤 Files with no reviewable changes (1)
- packages/openclaw/src/registration.ts
✅ Files skipped from review due to trivial changes (7)
- packages/bridge/tsdown.config.ts
- AGENTS.md
- packages/bridge/package.json
- package.json
- packages/pickle/native/go.mod
- packages/pickle/src/generated-runtime-types.ts
- packages/pickle/src/generated-runtime-operations.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/pickle/src/client.ts
- packages/pickle/native/internal/core/operations.go
- packages/openclaw/tsdown.config.ts
- packages/openclaw/src/rooms.test.ts
- packages/openclaw/vitest.config.ts
- packages/pickle/src/index.ts
- packages/pickle/src/runtime-types.ts
- packages/openclaw/src/protocol-coverage.ts
- packages/pickle/src/client-types.ts
- packages/pickle/native/internal/core/core.go
- packages/openclaw/src/openclaw-extension.test.ts
- packages/openclaw/src/bridge-agent.test.ts
- packages/openclaw/src/registration.test.ts
- packages/openclaw/package.json
- packages/bridge/src/bridge.test.ts
- packages/pickle/src/client.test.ts
- packages/openclaw/src/registry.ts
- packages/openclaw/src/integration.test.ts
- packages/openclaw/src/cli.ts
- packages/openclaw/src/openclaw-runtime.test.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: beeper/pickle
Timestamp: 2026-06-02T03:11:10.919Z
Learning: Use Crabpot for OpenClaw plugin and integration compatibility testing, including plugin API seams, channel registration, lifecycle hooks, provider capabilities, cold imports, workspace planning, and static execution policy
Learnt from: CR
Repo: beeper/pickle
Timestamp: 2026-06-02T03:11:10.919Z
Learning: Maintain expected sibling directory structure with checkouts at /Projects/pickle, /Projects/crabpot, and /Projects/openclaw
Learnt from: CR
Repo: beeper/pickle
Timestamp: 2026-06-02T03:11:10.919Z
Learning: Run full Pickle and OpenClaw plugin compatibility suite using `npm run full-test` before deployment
Learnt from: CR
Repo: beeper/pickle
Timestamp: 2026-06-02T03:11:10.919Z
Learning: Do not run opt-in isolated execution commands (with CRABPOT_EXECUTE_ISOLATED=1) unless the task explicitly requires side effects
🪛 GitHub Actions: CI / 0_Typecheck, test, build, and package.txt
packages/bridge/src/index.ts
[error] 1-1: pnpm -r typecheck failed at packages/bridge with command 'tsc --noEmit'.
packages/bridge/src/media-message.ts
[error] 1-1: pnpm -r typecheck failed at packages/bridge with command 'tsc --noEmit'.
packages/bridge/src/events.ts
[error] 1-1: pnpm -r typecheck failed at packages/bridge with command 'tsc --noEmit'.
packages/bridge/src/beeper-stream.ts
[error] 1-1: pnpm -r typecheck failed at packages/bridge with command 'tsc --noEmit'.
packages/bridge/src/media-message.test.ts
[error] 1-1: pnpm -r typecheck failed at packages/bridge with command 'tsc --noEmit'.
packages/bridge/src/beeper-stream.test.ts
[error] 1-1: pnpm -r typecheck failed at packages/bridge with command 'tsc --noEmit'.
packages/bridge/src/bridge.ts
[error] 113-113: TypeScript (tsc) error TS2741: Property 'roomState' is missing in type 'RuntimeBridge' but required in type 'PickleBridge'.
[error] 1-1: pnpm -r typecheck failed at packages/bridge with command 'tsc --noEmit'.
packages/bridge/src/types.ts
[error] 1-1: pnpm -r typecheck failed at packages/bridge with command 'tsc --noEmit'.
🪛 GitHub Actions: CI / Typecheck, test, build, and package
packages/bridge/src/bridge.ts
[error] 113-113: TypeScript (tsc --noEmit) failed: TS2741: Property 'roomState' is missing in type 'RuntimeBridge' but required in type 'PickleBridge'.
🪛 LanguageTool
packages/openclaw/README.md
[grammar] ~18-~18: Ensure spelling is correct
Context: ...d login available when needed. - Beeper appservice registration for the OpenClaw bridge. -...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~20-~20: Ensure spelling is correct
Context: ... and ClawHub install metadata. - Pickle bridgev2-style transport for Matrix portals, med...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (22)
packages/bridge/src/index.ts (1)
25-25: LGTM!Also applies to: 47-47, 59-61
packages/bridge/src/media-message.ts (1)
23-66: LGTM!packages/bridge/src/events.ts (1)
56-79: LGTM!packages/bridge/src/media-message.test.ts (1)
4-44: LGTM!packages/openclaw/src/beeper-channel-runtime.ts (1)
76-89:threadRootis still ignored for outbound sends.
sendText()andsendMedia()both acceptthreadRoot, but neither serializes anm.threadrelation, so threaded sends still land as top-level messages.Also applies to: 91-101
packages/openclaw/src/cli.test.ts (1)
211-215: Use a unique temp directory in the empty-statewhoamitest.The hardcoded
/tmp/pickle-openclaw-emptypath can still pick up residue from another run and make this case flaky.packages/openclaw/src/connector.ts (2)
212-232: ⚡ Quick winReuse the loaded runtime instance for
sendTurn.
loadUserLogin()creates a runtime via#runtimeFactory(this.config), but#sendTurncalls the factory again on every turn. If a customruntimeFactoryreturns a fresh adapter per call, dispatch gets split away from the runtime the agent is tracking, which can desynchronize event streams and lifecycle handling.The default factory at lines 125-128 returns the same cached
runtimeinstance, but a custom factory wouldn't have this guarantee.
264-273: LGTM!packages/openclaw/src/setup.ts (5)
184-211: ⚡ Quick winMedia sends still drop
replyToId.
sendMediadoesn't accept or forwardreplyToIdfrom the context. Thesend.mediawrapper at lines 145-153 includesreplyToIdin its signature, but it's lost when callingsendMedia. Replied media messages will be sent as top-level posts instead of replies.
446-459: ⚡ Quick win
mark_unreadnot exposed in advertised action set.
handleAction()at lines 489-494 implementsmark_unread, butbeeperMessageToolActions(line 446) andsupportsAction()(lines 458-459) omit it. Callers checking capabilities first will reject the action before it reaches the handler.
1163-1172: ⚖️ Poor tradeoff
stopBeeperGatewayAccountcannot signal a running bridge to stop.When a bridge is running and waiting at
waitForAbort(ctx.abortSignal)(line 1072),stopBeeperGatewayAccountstops the bridge object but doesn't abort the signal. Thefinallyblock instartBeeperGatewayAccountOncewon't run until the signal is aborted by the lifecycle manager. If the caller expectsstopAccount()to fully shut down the bridge synchronously, this may leave the startup task hanging.
1004-1026: LGTM!
939-998: LGTM!packages/openclaw/src/beeper-setup.test.ts (1)
1-174: LGTM!packages/openclaw/src/types.ts (1)
1-67: LGTM!packages/openclaw/src/beeper-setup.ts (1)
1-187: LGTM!packages/openclaw/src/appservice.ts (1)
1-100: LGTM!packages/openclaw/src/bridge-agent.ts (1)
1-141: LGTM!packages/openclaw/src/rooms.ts (1)
64-78: UsematrixDomainFromConfig()increateSessionRoom()too.This path still derives the Matrix domain from
config.homeserver, so the service-bot MXID is wrong wheneverhomeserverDomainintentionally differs from the URL hostname.packages/openclaw/src/openclaw-runtime.ts (2)
367-382: Still only one event source is wired here.The early returns still drop other host event streams, and
#localEventsis still skipped entirely whenruntime.eventsis a function orsubscribe()path is used.
2252-2255: Reset the visible-text buffer on replacements.This still appends replacement text onto the previous buffer, so a rewrite like
"hello" -> "hi"leaves corrupted state for later deltas and finalization.packages/pickle/native/internal/core/beeper_ai_run.go (1)
398-417: Reject duplicate or empty run IDs before replacing run state.
beginBeeperAIRun()still unconditionally overwritesc.beeperAIRuns[run.RunID], so a retry or emptyrunIdcan clobber an in-flight run and drop its accumulated stream state.
|
Re-checked after Resolved
Updated
Still valid (unchanged in these commits)
CodeRabbit's two outstanding finds ( |
![indent[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
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.
Summary
openclawpackage with CLI, runtime, bridge-agent, registry, setup, approval, and event-mapping flows.pickleappservice/auth path to align with the new OpenClaw bridge integration.Testing
packages/openclawandpackages/bridge.packages/pickle/native/internal/core.