Add OpenClaw bridge runtime and provisioning support#7
Open
batuhan wants to merge 44 commits into
Open
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds the OpenClaw Beeper plugin/package, connector, runtime adapters, streaming/approval/backfill/registry/rooms, and CLI. Expands bridge Matrix dispatch and provisioning. Introduces Go AI-run lifecycle operations and updates client/types/tests across packages. ChangesOpenClaw Beeper channel and bridge/runtime changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 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: 4
🧹 Nitpick comments (5)
packages/openclaw/src/backfill.test.ts (1)
165-165: ⚡ Quick winUse per-test temporary registry paths for isolation.
Lines 165, 220, 269, and 307 use fixed
/tmp/*.jsonfiles. Re-runs/watch mode can inherit stale state and make these tests flaky. Prefermkdtemp+ uniqueregistry.jsonper test case.♻️ Suggested pattern
+import { mkdtemp } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; ... -const registry = new OpenClawBridgeRegistry("/tmp/openclaw-backfill-test.json"); +const registryDir = await mkdtemp(join(tmpdir(), "openclaw-backfill-")); +const registry = new OpenClawBridgeRegistry(join(registryDir, "registry.json"));Also applies to: 220-220, 269-269, 307-307
🤖 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.test.ts` at line 165, Replace fixed temp file paths used when constructing OpenClawBridgeRegistry in the tests (e.g., the registry variable created with new OpenClawBridgeRegistry("/tmp/openclaw-backfill-test.json")) with a per-test unique path generated via a temporary directory (use mkdtemp or equivalent) and a deterministic filename like "registry.json" inside that dir, and ensure cleanup after the test; update all occurrences referenced in this file (the registry instantiations around the tests at lines that create registry) so each test gets an isolated registry file.packages/openclaw/src/bridge-agent.ts (1)
113-122: 💤 Low valueEvent normalization logic may duplicate payload data.
When
event.eventandevent.payloadare both present (line 114-116), the result includes both the spreadpayloadfields andpayload: event.payload. This creates a structure where the same data appears twice under different keys, which may cause confusion or increased memory usage in long-running streams.Consider whether the
payloadproperty in the result is intentional or if only the spread fields are needed.🤖 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/bridge-agent.ts` around lines 113 - 122, The normalization in openClawEventFromGateway duplicates payload when both event.event and event.payload are present by spreading event.payload and also adding payload: event.payload; update openClawEventFromGateway so it does one or the other: either spread the payload fields into the returned object and omit the payload property, or return an object with type and a single payload property (no spreading). Ensure the returned shape is consistent with downstream consumers (adjust callers if they expect payload vs flattened fields) and keep the branch for when only payload exists unchanged.packages/openclaw/src/backfill.ts (1)
103-156: ⚖️ Poor tradeoffConsider adding error handling for individual session backfill failures.
The loop at lines 111-154 processes sessions sequentially, but if one session fails (e.g.,
buildBackfillImportthrows), the entire backfill operation will abort and subsequent sessions won't be processed.Consider wrapping individual session processing in try-catch to log failures and continue with remaining sessions, or document that partial failures are intentional.
🤖 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 103 - 156, The backfillAllOpenClawSessions loop currently lets any thrown error (e.g., from buildBackfillImport, options.bridge.createPortal, or options.bridge.backfillPortal) abort the whole operation; wrap the per-session processing inside a try-catch so one failing session is logged and skipped while the loop continues. Specifically, in backfillAllOpenClawSessions surround the body that creates the agent/portal, calls options.bridge.createPortal, buildBackfillImport, options.registry.upsertBinding, and options.bridge.backfillPortal with try { ... } catch (err) { processLogger.error(...) or options.runtime.logger.error(...); skipped.push(session); continue; } and ensure any resources (like created portals or partial bindings) are accounted for or rolled back if needed; keep existing behavior of pushing successful sessions into importedSessions and portals into portals.packages/openclaw/src/cli.ts (1)
398-410: 💤 Low valueType cast of
io.stderrmay fail for minimal streams.The
CliIO.stderrtype isPick<typeof process.stderr, "write">, butcreateInterfaceexpects a fullNodeJS.WritableStreamfor the output parameter. If a minimal mock is passed (only implementingwrite), this cast could cause runtime errors when readline tries to use other stream methods.Consider using
process.stderras a fallback or documenting thatstderrmust be a full writable stream whenstdinis provided.🤖 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/cli.ts` around lines 398 - 410, The code passes io.stderr to createInterface by casting to NodeJS.WritableStream which can fail for minimal mocks (CliIO.stderr). Change the call around createInterface to detect whether io.stderr implements the full writable stream surface (e.g., has write and on/emit methods) and if not fall back to process.stderr; update the createInterface invocation (the rl creation that uses input and output) to use that fallback so readline always gets a proper NodeJS.WritableStream, and ensure io.stdin/io.stderr checks happen before rl.question/rl.close.packages/openclaw/src/setup.ts (1)
443-481: ⚖️ Poor tradeoffGateway start/stop may race on rapid abort.
In
startBeeperGatewayAccount, the bridge is added tostartedBridgesat line 462, thenwaitForAbortis called. IfabortSignalis already aborted when the function is called,waitForAbortresolves immediately (line 668), butbridge.stop?.()is still called in the finally block. This is correct behavior.However, if
stopBeeperGatewayAccountis called concurrently whilestartBeeperGatewayAccountis in the finally block, both could attempt to stop the same bridge and delete from the map. Consider adding a guard or using a more explicit state machine if concurrent stop calls are expected.🤖 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/setup.ts` around lines 443 - 481, startBeeperGatewayAccount can race with stopBeeperGatewayAccount causing both to call bridge.stop and delete startedBridges; to fix, make stop idempotent by tying deletion/stop to the exact instance you started: when you set startedBridges with key = gatewayAccountKey(ctx.accountId) store the bridge object (StartedBeeperBridge) and in the finally block only call startedBridges.delete(key) and await bridge.stop() if startedBridges.get(key) === bridge (i.e., compare by reference) — likewise update stopBeeperGatewayAccount to read the bridge from startedBridges, delete it (or mark it) and call stop only for that retrieved instance; alternatively use a per-bridge mutex/boolean flag on the stored entry to ensure stop is executed once.
🤖 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/openclaw/README.md`:
- Line 25: Update the README text to capitalize "Mac" where it refers to Apple's
platform: change "mac app" to "Mac app" (and any other occurrences like the one
noted around line 70) so all references to Apple's macOS/Mac apps use "Mac" with
an uppercase M.
In `@packages/openclaw/src/registry.test.ts`:
- Around line 8-46: The test creates a temp dir via mkdtemp and never removes
it; update the test that uses mkdtemp/dir/path/OpenClawBridgeRegistry to delete
the temporary directory when done (use a finally block or afterEach/teardown) so
the temp folder is removed even on failures; ensure you call the appropriate
async removal (e.g., fs.rm/fs.rmdir with recursive/force) on dir after
registry.save() and after the loaded assertions to prevent accumulating temp
files.
In `@packages/openclaw/src/registry.ts`:
- Around line 37-39: The temp filename for atomic saves can collide when
multiple save() calls run in the same process; change the tmp name generation in
the save routine to include a per-operation unique token (e.g., add
crypto.randomUUID() or a timestamp+random suffix) so each save uses its own temp
file (use the same this.path and this.#data write/rename flow but build tmp as
`${this.path}.${process.pid}.${uniqueToken}.tmp` and import/use
crypto.randomUUID() or a safe fallback).
In `@packages/openclaw/src/rooms.ts`:
- Around line 18-27: The helper functions (agentGhostUserId, userGhostUserId,
serviceBotUserId and the other similar helpers around lines 86-100) currently
always compute the domain with matrixDomainFromHomeserver(config.homeserver) and
therefore ignore an explicit config.homeserverDomain; update each function to
prefer config.homeserverDomain when present and only fall back to
matrixDomainFromHomeserver(config.homeserver) otherwise (e.g., use domain =
config.homeserverDomain ?? matrixDomainFromHomeserver(config.homeserver)); make
this change for agentGhostUserId, userGhostUserId, serviceBotUserId and the
other ghost/service MXID helpers so generated MXIDs use the configured
homeserverDomain when provided.
---
Nitpick comments:
In `@packages/openclaw/src/backfill.test.ts`:
- Line 165: Replace fixed temp file paths used when constructing
OpenClawBridgeRegistry in the tests (e.g., the registry variable created with
new OpenClawBridgeRegistry("/tmp/openclaw-backfill-test.json")) with a per-test
unique path generated via a temporary directory (use mkdtemp or equivalent) and
a deterministic filename like "registry.json" inside that dir, and ensure
cleanup after the test; update all occurrences referenced in this file (the
registry instantiations around the tests at lines that create registry) so each
test gets an isolated registry file.
In `@packages/openclaw/src/backfill.ts`:
- Around line 103-156: The backfillAllOpenClawSessions loop currently lets any
thrown error (e.g., from buildBackfillImport, options.bridge.createPortal, or
options.bridge.backfillPortal) abort the whole operation; wrap the per-session
processing inside a try-catch so one failing session is logged and skipped while
the loop continues. Specifically, in backfillAllOpenClawSessions surround the
body that creates the agent/portal, calls options.bridge.createPortal,
buildBackfillImport, options.registry.upsertBinding, and
options.bridge.backfillPortal with try { ... } catch (err) {
processLogger.error(...) or options.runtime.logger.error(...);
skipped.push(session); continue; } and ensure any resources (like created
portals or partial bindings) are accounted for or rolled back if needed; keep
existing behavior of pushing successful sessions into importedSessions and
portals into portals.
In `@packages/openclaw/src/bridge-agent.ts`:
- Around line 113-122: The normalization in openClawEventFromGateway duplicates
payload when both event.event and event.payload are present by spreading
event.payload and also adding payload: event.payload; update
openClawEventFromGateway so it does one or the other: either spread the payload
fields into the returned object and omit the payload property, or return an
object with type and a single payload property (no spreading). Ensure the
returned shape is consistent with downstream consumers (adjust callers if they
expect payload vs flattened fields) and keep the branch for when only payload
exists unchanged.
In `@packages/openclaw/src/cli.ts`:
- Around line 398-410: The code passes io.stderr to createInterface by casting
to NodeJS.WritableStream which can fail for minimal mocks (CliIO.stderr). Change
the call around createInterface to detect whether io.stderr implements the full
writable stream surface (e.g., has write and on/emit methods) and if not fall
back to process.stderr; update the createInterface invocation (the rl creation
that uses input and output) to use that fallback so readline always gets a
proper NodeJS.WritableStream, and ensure io.stdin/io.stderr checks happen before
rl.question/rl.close.
In `@packages/openclaw/src/setup.ts`:
- Around line 443-481: startBeeperGatewayAccount can race with
stopBeeperGatewayAccount causing both to call bridge.stop and delete
startedBridges; to fix, make stop idempotent by tying deletion/stop to the exact
instance you started: when you set startedBridges with key =
gatewayAccountKey(ctx.accountId) store the bridge object (StartedBeeperBridge)
and in the finally block only call startedBridges.delete(key) and await
bridge.stop() if startedBridges.get(key) === bridge (i.e., compare by reference)
— likewise update stopBeeperGatewayAccount to read the bridge from
startedBridges, delete it (or mark it) and call stop only for that retrieved
instance; alternatively use a per-bridge mutex/boolean flag on the stored entry
to ensure stop is executed once.
🪄 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: 15a4fe7d-cf6d-4994-9088-6f5db04da9ae
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (60)
PLAN.mdpackages/bridge/src/bridge.test.tspackages/bridge/src/bridge.tspackages/bridge/src/provisioning.test.tspackages/bridge/src/provisioning.tspackages/bridge/src/types.tspackages/openclaw/LICENSEpackages/openclaw/README.mdpackages/openclaw/openclaw.plugin.jsonpackages/openclaw/package.jsonpackages/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-setup.test.tspackages/openclaw/src/beeper-setup.tspackages/openclaw/src/beeper-stream.test.tspackages/openclaw/src/beeper-stream.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/index.tspackages/openclaw/src/integration.test.tspackages/openclaw/src/openclaw-event-map.test.tspackages/openclaw/src/openclaw-event-map.tspackages/openclaw/src/openclaw-extension.test.tspackages/openclaw/src/openclaw-extension.tspackages/openclaw/src/openclaw-runtime.test.tspackages/openclaw/src/openclaw-runtime.tspackages/openclaw/src/plugin-entry.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/serial.tspackages/openclaw/src/setup-entry.tspackages/openclaw/src/setup.test.tspackages/openclaw/src/setup.tspackages/openclaw/src/stream-map.tspackages/openclaw/src/types.tspackages/openclaw/tsconfig.jsonpackages/openclaw/tsdown.config.tspackages/openclaw/vitest.config.tspackages/pickle/native/internal/core/appservice.gopackages/pickle/native/internal/core/appservice_test.gopackages/pickle/src/beeper/auth.test.tspackages/pickle/src/beeper/auth.tspackages/pickle/src/generated-runtime-types.tspnpm-workspace.yaml
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: CI / 0_Typecheck, test, build, and package.txt
packages/openclaw/package.json
[error] 1-1: Dependency specifier mismatch versus pnpm-lock.yaml: openclaw@>=2026.5.24 appears to have been added in package.json but not reflected in the lockfile.
🪛 GitHub Actions: CI / Typecheck, test, build, and package
packages/openclaw/package.json
[error] 1-1: Dependency specifiers added/changed in package.json require updating pnpm-lock.yaml (openclaw@>=2026.5.24).
🪛 LanguageTool
packages/openclaw/README.md
[grammar] ~18-~18: Ensure spelling is correct
Context: ...e login for existing accounts. - Beeper appservice registration for the OpenClaw bridge. -...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~25-~25: Did you mean Apple’s computer “Mac” (= trademark, capitalized)?
Context: ...ation. - Backfill helpers for terminal, mac app, and external one-to-one OpenClaw s...
(APPLE_PRODUCTS)
[grammar] ~46-~46: Ensure spelling is correct
Context: ...-code 123456 Register the OpenClaw appservice with Beeper: sh pickle-openclaw bee...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~54-~54: Ensure spelling is correct
Context: ...BRIDGE_MANAGER_TOKEN" Do login and appservice registration in one step: sh pickle...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~70-~70: Did you mean Apple’s computer “Mac” (= trademark, capitalized)?
Context: ...to-one OpenClaw sessions from terminal, mac app, and channel surfaces: ```sh pickl...
(APPLE_PRODUCTS)
PLAN.md
[grammar] ~5-~5: Ensure spelling is correct
Context: ...nto Beeper through a self-hosted Beeper appservice. The plugin owns Beeper login, appservi...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~5-~5: Ensure spelling is correct
Context: ...pservice. The plugin owns Beeper login, appservice registration, settings/setup, contact d...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~44-~44: Ensure spelling is correct
Context: ...nit tests: - Beeper OTP/setup config, appservice registration, ClawHub/package metadata,...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~58-~58: Ensure spelling is correct
Context: ...build` - Focused Pickle bridge stream/appservice tests - Package validation for OpenCl...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 OpenGrep (1.21.0)
packages/openclaw/src/backfill.ts
[ERROR] 208-208: 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/connector.ts
[ERROR] 922-922: 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 (58)
pnpm-workspace.yaml (1)
6-6: LGTM!packages/openclaw/tsdown.config.ts (1)
3-8: LGTM!packages/openclaw/src/setup-entry.ts (1)
3-8: LGTM!packages/openclaw/src/rooms.test.ts (1)
15-98: LGTM!packages/openclaw/src/openclaw-extension.ts (1)
3-21: LGTM!packages/openclaw/vitest.config.ts (1)
3-23: LGTM!packages/openclaw/LICENSE (1)
1-1: ⚡ Quick winCheck MPL-2.0 notice/availability for
packages/openclaw
packages/openclaw/LICENSEis SPDX-only (MPL-2.0, 1 line). This aligns with the repo’s monorepo licensing approach for other MPL packages (e.g., multiplepackages/*/LICENSEfiles are also SPDX-only or contain a brief MPL notice + link rather than embedding the full MPL text).Verify for the published OpenClaw artifacts:
- The required MPL 2.0 attribution/notice is included (and users are given a way to obtain the MPL text).
- The root
./LICENSEand the relevant package-level LICENSE files are included in what’s actually distributed.- Whether you specifically need to ship the full MPL 2.0 text in
packages/openclaw/LICENSE, or whether the repo’s current SPDX-only/pointer pattern satisfies your distribution model.packages/openclaw/src/config.test.ts (1)
1-69: LGTM!packages/pickle/src/beeper/auth.test.ts (1)
70-102: LGTM!PLAN.md (1)
1-67: LGTM!packages/openclaw/src/index.ts (1)
1-22: LGTM!packages/openclaw/tsconfig.json (1)
1-9: LGTM!packages/openclaw/src/registration.test.ts (1)
1-69: LGTM!packages/bridge/src/bridge.ts (2)
292-292: LGTM!
851-860: LGTM!packages/openclaw/src/plugin-entry.ts (1)
1-4: LGTM!packages/openclaw/src/beeper-setup.test.ts (1)
1-150: LGTM!packages/openclaw/src/openclaw-runtime.test.ts (1)
1-326: LGTM!packages/openclaw/src/beeper-setup.ts (1)
1-177: LGTM!packages/openclaw/src/openclaw-extension.test.ts (1)
1-110: LGTM!packages/openclaw/src/serial.ts (1)
1-9: LGTM!packages/pickle/src/generated-runtime-types.ts (1)
77-77: LGTM!packages/pickle/native/internal/core/appservice_test.go (1)
31-34: LGTM!Also applies to: 56-58
packages/openclaw/openclaw.plugin.json (1)
1-176: LGTM!packages/openclaw/src/approval.test.ts (1)
1-92: LGTM!packages/openclaw/src/types.ts (1)
1-93: LGTM!packages/openclaw/src/appservice.test.ts (1)
1-81: LGTM!packages/bridge/src/types.ts (1)
163-165: LGTM!Also applies to: 655-656, 894-902
packages/bridge/src/provisioning.test.ts (1)
35-35: LGTM!Also applies to: 49-98, 112-117
packages/pickle/src/beeper/auth.ts (1)
12-12: LGTM!Also applies to: 40-43, 87-89, 93-93, 102-104, 107-107
packages/openclaw/src/protocol-coverage.test.ts (1)
1-72: LGTM!packages/bridge/src/bridge.test.ts (1)
248-253: LGTM!Also applies to: 264-269
packages/pickle/native/internal/core/appservice.go (1)
89-95: LGTM!Also applies to: 356-361, 709-716
packages/openclaw/src/registration.ts (1)
9-63: LGTM!packages/bridge/src/provisioning.ts (1)
11-11: LGTM!Also applies to: 23-23, 46-55, 101-101, 159-164, 234-239
packages/openclaw/src/cli.test.ts (1)
9-412: LGTM!packages/openclaw/src/protocol-coverage.ts (1)
1-226: LGTM!packages/openclaw/package.json (1)
185-191: ⚡ Quick winRemove the
pnpm-lock.yamlupdate request for theopenclawpeerDependency spec change
pnpm-lock.yamldoes not recordpeerDependencies/peerDependenciesMetafor thepackages/openclawworkspace importer (it contains noopenclawspecifier anywhere beyond the importer key), so regenerating the lockfile shouldn’t be required for this change.> Likely an incorrect or invalid review comment.packages/openclaw/src/integration.test.ts (1)
13-266: LGTM!packages/openclaw/src/beeper-stream.test.ts (1)
7-263: LGTM!packages/openclaw/src/config.ts (1)
8-151: LGTM!packages/openclaw/src/appservice.ts (1)
23-97: LGTM!packages/openclaw/src/approval.ts (1)
24-127: LGTM!packages/openclaw/src/openclaw-event-map.ts (1)
212-216: ⚡ Quick winFix overreach: circular payloads are unlikely on the
run.failedpath
errorText()is called forrun.failedwithdata.error ?? data.message ?? data, wheredataoriginates from gateway frames parsed viaJSON.parse(SSEparseEventFrameand WebSocket#handleFrame). That means circular object graphs and non-JSONErrorinstances shouldn’t reacherrorText()from the runtime, so this is not a reliability issue forrun.failedhandling. Keep the fallback only as optional hardening for non-wire callers.> Likely an incorrect or invalid review comment.packages/openclaw/src/bridge-agent.test.ts (1)
1-276: LGTM!packages/openclaw/src/cli.ts (2)
28-205: LGTM!
207-416: LGTM!packages/openclaw/src/openclaw-event-map.test.ts (1)
1-297: LGTM!packages/openclaw/src/beeper-stream.ts (2)
52-203: LGTM!
205-389: LGTM!packages/openclaw/src/setup.test.ts (1)
1-548: LGTM!packages/openclaw/src/bridge-agent.ts (1)
1-112: LGTM!packages/openclaw/src/backfill.ts (1)
1-102: LGTM!Also applies to: 157-271
packages/openclaw/src/setup.ts (1)
1-442: LGTM!Also applies to: 482-707
packages/openclaw/src/connector.ts (1)
1-935: LGTM!The static analysis hint about "command injection" on line 922 is a false positive - the code uses
RegExp.prototype.exec()for pattern matching, notchild_process.exec/execSync.packages/openclaw/src/connector.test.ts (1)
1-1087: LGTM!packages/openclaw/src/openclaw-runtime.ts (1)
1-721: LGTM!packages/openclaw/src/stream-map.ts (1)
1-284: LGTM!
Comment on lines
+8
to
+46
| it("persists agent contacts, user contacts, session bindings, and dedupe keys", async () => { | ||
| const dir = await mkdtemp(resolve(tmpdir(), "pickle-openclaw-")); | ||
| const path = resolve(dir, "registry.json"); | ||
| const registry = new OpenClawBridgeRegistry(path); | ||
| await registry.load(); | ||
| registry.upsertAgent({ | ||
| agentId: "codex", | ||
| displayName: "Codex", | ||
| ghostUserId: "@openclaw_agent_codex:example.com", | ||
| }); | ||
| registry.upsertUser({ | ||
| displayName: "Alice", | ||
| ghostUserId: "@openclaw_user_alice:example.com", | ||
| source: "whatsapp", | ||
| userId: "alice", | ||
| }); | ||
| registry.upsertBinding({ | ||
| agentId: "codex", | ||
| createdAt: 1, | ||
| ghostUserId: "@openclaw_agent_codex:example.com", | ||
| id: "binding", | ||
| kind: "session", | ||
| owner: "bridge", | ||
| roomId: "!room:example.com", | ||
| sessionKey: "agent:codex:main", | ||
| updatedAt: 1, | ||
| }); | ||
| registry.markDedupe("$event"); | ||
| await registry.save(); | ||
|
|
||
| const loaded = new OpenClawBridgeRegistry(path); | ||
| await loaded.load(); | ||
| expect(loaded.getAgent("codex")?.displayName).toBe("Codex"); | ||
| expect(loaded.getUser("alice")?.ghostUserId).toBe("@openclaw_user_alice:example.com"); | ||
| expect(loaded.getBindingByRoom("!room:example.com")?.sessionKey).toBe("agent:codex:main"); | ||
| expect(loaded.getBindingBySessionKey("agent:codex:main")?.id).toBe("binding"); | ||
| expect(loaded.getBindingsByAgent("codex")).toHaveLength(1); | ||
| expect(loaded.hasDedupe("$event")).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Add cleanup for temporary test directory.
The test creates a temporary directory but never removes it, which will accumulate temp files over multiple test runs.
🧹 Proposed fix to add cleanup
+import { rm } from "node:fs/promises";
+
describe("OpenClawBridgeRegistry", () => {
it("persists agent contacts, user contacts, session bindings, and dedupe keys", async () => {
const dir = await mkdtemp(resolve(tmpdir(), "pickle-openclaw-"));
- const path = resolve(dir, "registry.json");
- const registry = new OpenClawBridgeRegistry(path);
- await registry.load();
+ try {
+ const path = resolve(dir, "registry.json");
+ const registry = new OpenClawBridgeRegistry(path);
+ await registry.load();
- // ... rest of test
+ // ... rest of test
- expect(loaded.hasDedupe("$event")).toBe(true);
+ expect(loaded.hasDedupe("$event")).toBe(true);
+ } finally {
+ await rm(dir, { recursive: true, force: true });
+ }
});
});🤖 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 - 46, The test creates
a temp dir via mkdtemp and never removes it; update the test that uses
mkdtemp/dir/path/OpenClawBridgeRegistry to delete the temporary directory when
done (use a finally block or afterEach/teardown) so the temp folder is removed
even on failures; ensure you call the appropriate async removal (e.g.,
fs.rm/fs.rmdir with recursive/force) on dir after registry.save() and after the
loaded assertions to prevent accumulating temp files.
Comment on lines
+37
to
+39
| const tmp = `${this.path}.${process.pid}.tmp`; | ||
| await writeFile(tmp, `${JSON.stringify(this.#data, null, 2)}\n`, { mode: 0o600 }); | ||
| await rename(tmp, this.path); |
There was a problem hiding this comment.
Use a unique temp filename per save operation.
Using only process.pid for the temp path can collide under concurrent save() calls in the same process.
🛠️ Proposed fix
+import { randomUUID } from "node:crypto";
import { mkdir, readFile, rename, writeFile } from "node:fs/promises";
@@
- const tmp = `${this.path}.${process.pid}.tmp`;
+ const tmp = `${this.path}.${process.pid}.${randomUUID()}.tmp`;📝 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
| const tmp = `${this.path}.${process.pid}.tmp`; | |
| await writeFile(tmp, `${JSON.stringify(this.#data, null, 2)}\n`, { mode: 0o600 }); | |
| await rename(tmp, this.path); | |
| import { randomUUID } from "node:crypto"; | |
| import { mkdir, readFile, rename, writeFile } from "node:fs/promises"; | |
| const tmp = `${this.path}.${process.pid}.${randomUUID()}.tmp`; | |
| await writeFile(tmp, `${JSON.stringify(this.#data, null, 2)}\n`, { mode: 0o600 }); | |
| await rename(tmp, this.path); |
🤖 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.ts` around lines 37 - 39, The temp filename
for atomic saves can collide when multiple save() calls run in the same process;
change the tmp name generation in the save routine to include a per-operation
unique token (e.g., add crypto.randomUUID() or a timestamp+random suffix) so
each save uses its own temp file (use the same this.path and this.#data
write/rename flow but build tmp as
`${this.path}.${process.pid}.${uniqueToken}.tmp` and import/use
crypto.randomUUID() or a safe fallback).
added 14 commits
May 25, 2026 02:47
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
+1756
to
+1760
| #matchingRemoteTarget(existing: SentEvent[], partId: string | undefined, index: number): SentEvent | undefined { | ||
| if (partId) { | ||
| return existing[index] ?? existing[0]; | ||
| } | ||
| return existing[index] ?? existing[0]; |
There was a problem hiding this comment.
Match remote edit targets by partId instead of index-only fallback.
#matchingRemoteTarget ignores partId, so multipart edits/removals can update the wrong Matrix event when ordering differs from stored parts.
🛠️ Proposed fix
- const target = this.#matchingRemoteTarget(existing.sent, part.id, index);
+ const target = this.#matchingRemoteTarget(existing, part.id, index);
- `#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];
}Also applies to: 1529-1530
🤖 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 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.
Comment on lines
+60
to
+67
| export function defaultBeeperApprovalActions(decisions: readonly ApprovalDecision[] = ["allow_once", "allow_session", "allow_room", "deny"]): Record<string, unknown>[] { | ||
| return decisions.map((decision) => ({ | ||
| decision: decision.replace(/_/gu, "-"), | ||
| id: decision.replace(/_/gu, "-"), | ||
| reactionKey: approvalReactionKey(decision), | ||
| title: approvalActionTitle(decision), | ||
| variant: decision === "deny" ? "destructive" : "secondary", | ||
| })); |
There was a problem hiding this comment.
Don't advertise approval scopes the resolver can't preserve.
defaultBeeperApprovalActions() exposes allow_session and allow_room, but toOpenClawApprovalResolvePayload() later collapses everything to approve / approve_always / deny. That means a click on "Allow This Room" is serialized as an always-approve, which widens the grant beyond what the UI promised.
Suggested fix
-export function defaultBeeperApprovalActions(decisions: readonly ApprovalDecision[] = ["allow_once", "allow_session", "allow_room", "deny"]): Record<string, unknown>[] {
+export function defaultBeeperApprovalActions(decisions: readonly ApprovalDecision[] = ["allow_once", "allow_always", "deny"]): Record<string, unknown>[] {🤖 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/approval.ts` around lines 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.
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.
Comment on lines
+70
to
+95
| async sendText(options: { | ||
| content?: Record<string, unknown>; | ||
| replyToId?: string | null; | ||
| roomId: string; | ||
| text: string; | ||
| threadRoot?: string | number | null; | ||
| }): Promise<SentEvent> { | ||
| const content = { | ||
| body: options.text, | ||
| msgtype: "m.text", | ||
| ...options.content, | ||
| }; | ||
| return await this.#queueRemoteText(options.roomId, withReplyRelation(content, options.replyToId)); | ||
| } | ||
|
|
||
| async sendMedia(options: BeeperOutboundMedia & { roomId: string }): Promise<SentEvent> { | ||
| const bytes = options.bytes ?? (options.path ? await readFile(options.path) : undefined); | ||
| if (!bytes) { | ||
| throw new Error("Beeper media send requires bytes or a local file path."); | ||
| } | ||
| return await this.#queueRemoteMedia(options.roomId, { | ||
| bytes, | ||
| kind: options.kind ?? "file", | ||
| ...(options.caption !== undefined ? { caption: options.caption } : {}), | ||
| ...(options.filename !== undefined ? { filename: options.filename } : {}), | ||
| }); |
There was a problem hiding this comment.
threadRoot is accepted but never serialized.
Both sendText() and sendMedia() take threadRoot, but the queued Matrix content only handles replies. Any caller trying to send into a thread will post a top-level message instead.
🤖 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-runtime.ts` around lines 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.
| it("uses an injected Beeper turn sender for live Matrix room turns", async () => { | ||
| const registry = await tempRegistry(); | ||
| registry.upsertBinding(testBinding()); | ||
| const runtime = runtimeWith({}); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify call sites and function signature in bridge-agent test file
rg -n 'runtimeWith\(\{\}\)' packages/openclaw/src/bridge-agent.test.ts
rg -n 'function runtimeWith\(options' -A8 packages/openclaw/src/bridge-agent.test.tsRepository: beeper/pickle
Length of output: 562
🏁 Script executed:
#!/bin/bash
rg -n 'runtimeWith\(\{\}\)' packages/openclaw/src/bridge-agent.test.ts
rg -n 'runtimeWith\(' packages/openclaw/src/bridge-agent.test.tsRepository: beeper/pickle
Length of output: 373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show context around the problematic call and other runtimeWith usages
nl -ba packages/openclaw/src/bridge-agent.test.ts | sed -n '35,90p'
nl -ba packages/openclaw/src/bridge-agent.test.ts | sed -n '90,170p'
nl -ba packages/openclaw/src/bridge-agent.test.ts | sed -n '200,250p'Repository: beeper/pickle
Length of output: 100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n packages/openclaw/src/bridge-agent.test.ts | sed -n '35,90p'
cat -n packages/openclaw/src/bridge-agent.test.ts | sed -n '90,170p'
cat -n packages/openclaw/src/bridge-agent.test.ts | sed -n '200,250p'Repository: beeper/pickle
Length of output: 7006
🏁 Script executed:
#!/bin/bash
rg -n 'runtimeWith\(\s*\{\s*\}\s*\)' packages/openclaw/src/bridge-agent.test.tsRepository: beeper/pickle
Length of output: 97
Pass responses into runtimeWith in this test (missing required option at line 53)
runtimeWith requires responses: Record<string, unknown> (runtimeWith(options: { events?: ...; responses: ... })), but runtimeWith({}) omits it at packages/openclaw/src/bridge-agent.test.ts:53.
💡 Proposed fix
- const runtime = runtimeWith({});
+ const runtime = runtimeWith({ responses: {} });📝 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
| const runtime = runtimeWith({}); | |
| const runtime = runtimeWith({ responses: {} }); |
🤖 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/bridge-agent.test.ts` at 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.
Comment on lines
+186
to
+204
| media: async (ctx: { | ||
| cfg: OpenClawSetupConfig; | ||
| to: string; | ||
| text?: string; | ||
| mediaUrl?: string; | ||
| mediaReadFile?: (filePath: string) => Promise<Buffer>; | ||
| replyToId?: string | null; | ||
| threadId?: string | number | null; | ||
| }) => beeperMessageSendResult(await beeperOutboundAdapter.sendMedia(ctx)), | ||
| payload: async (ctx: { | ||
| cfg: OpenClawSetupConfig; | ||
| to: string; | ||
| text?: string; | ||
| mediaUrl?: string; | ||
| mediaReadFile?: (filePath: string) => Promise<Buffer>; | ||
| payload?: unknown; | ||
| replyToId?: string | null; | ||
| threadId?: string | number | null; | ||
| }) => beeperMessageSendResult(await beeperOutboundAdapter.sendPayload(ctx)), |
There was a problem hiding this comment.
Media sends currently drop replyToId.
replyTo is advertised in the message capabilities and send.media accepts replyToId, but the media path never forwards it. sendPayload() inherits the same bug when it routes through sendMedia(), so replied media messages are sent as top-level posts.
Suggested fix
send: {
@@
media: async (ctx: {
cfg: OpenClawSetupConfig;
to: string;
text?: string;
mediaUrl?: string;
mediaReadFile?: (filePath: string) => Promise<Buffer>;
replyToId?: string | null;
threadId?: string | number | null;
}) => beeperMessageSendResult(await beeperOutboundAdapter.sendMedia(ctx)),
@@
sendMedia: async (ctx: {
to: string;
text?: string;
mediaUrl?: string;
mediaReadFile?: (filePath: string) => Promise<Buffer>;
+ replyToId?: string | null;
threadId?: string | number | null;
}) => {
@@
const mediaOptions = {
roomId: resolveBeeperRoomTarget(ctx.to),
...(bytes !== undefined ? { bytes } : {}),
...(ctx.text !== undefined ? { caption: ctx.text } : {}),
...(filename ? { filename } : {}),
...(bytes === undefined ? { path: mediaUrl } : {}),
+ ...(ctx.replyToId ? { replyToId: ctx.replyToId } : {}),
...(ctx.threadId != null ? { threadRoot: String(ctx.threadId) } : {}),
};
@@
return await beeperOutboundAdapter.sendMedia({
mediaUrl,
text,
to: ctx.to,
...(ctx.mediaReadFile !== undefined ? { mediaReadFile: ctx.mediaReadFile } : {}),
+ ...(ctx.replyToId ? { replyToId: ctx.replyToId } : {}),
...(ctx.threadId != null ? { threadId: ctx.threadId } : {}),
});Also applies to: 225-279
🤖 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/setup.ts` around lines 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.
Comment on lines
+487
to
+500
| const beeperMessageToolActions = ["send", "react", "read"] as const satisfies readonly ChannelMessageActionName[]; | ||
|
|
||
| function beeperToolTextResult(text: string) { | ||
| return { content: [{ type: "text" as const, text }], details: {} }; | ||
| } | ||
|
|
||
| export const beeperMessageActions = { | ||
| resolveExecutionMode: () => "gateway" as const, | ||
| describeMessageTool: () => ({ | ||
| actions: beeperMessageToolActions, | ||
| capabilities: [], | ||
| }), | ||
| supportsAction: ({ action }: { action: string }) => | ||
| action === "send" || action === "react" || action === "read", |
There was a problem hiding this comment.
Expose mark_unread in the advertised action set.
handleAction() implements mark_unread, but describeMessageTool() and supportsAction() omit it. Any caller that checks capabilities first will reject the action before it reaches the handler.
Suggested fix
-const beeperMessageToolActions = ["send", "react", "read"] as const satisfies readonly ChannelMessageActionName[];
+const beeperMessageToolActions = ["send", "react", "read", "mark_unread"] as const satisfies readonly ChannelMessageActionName[];
@@
supportsAction: ({ action }: { action: string }) =>
- action === "send" || action === "react" || action === "read",
+ action === "send" || action === "react" || action === "read" || action === "mark_unread",Also applies to: 530-534
🤖 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/setup.ts` around lines 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.
Comment on lines
+1094
to
+1105
| try { | ||
| await waitForAbort(ctx.abortSignal); | ||
| } finally { | ||
| startedBridges.delete(key); | ||
| if (hostRuntime && openClawPluginRuntime && hostRuntime !== openClawPluginRuntime) { | ||
| setBeeperChannelRuntimeForHost(openClawPluginRuntime, undefined); | ||
| } | ||
| await bridge.stop?.(); | ||
| ctx.setStatus?.({ | ||
| accountId: ctx.accountId, | ||
| running: false, | ||
| }); |
There was a problem hiding this comment.
stopAccount() does not let startAccount() finish its cleanup.
After startup, startBeeperGatewayAccount() waits only on ctx.abortSignal. stopBeeperGatewayAccount() stops the bridge and clears the map, but it never resolves that wait, so the finally block that clears the host/runtime mapping and reports shutdown may never run when the lifecycle manager calls stopAccount() directly.
Also applies to: 1186-1195
🤖 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/setup.ts` around lines 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.
Comment on lines
+63
to
+67
| run := aistream.NewRun(req.RunID, req.ThreadID, req.Model, req.AgentID, req.AgentName, time.Now()) | ||
| writer := aistream.NewWriter(run, time.Now) | ||
| writer.Start() | ||
| c.beeperAIRuns[run.RunID] = &beeperAIRunState{run: run, writer: writer} | ||
| return c.marshalBeeperAIRunSnapshot(run, outboundEventsFromAGUI(run.Events)) |
There was a problem hiding this comment.
Reject duplicate Beeper AI run IDs on begin.
Line 66 overwrites existing state for the same RunID, which can drop in-flight run data on retries or duplicate calls.
Proposed fix
func (c *Core) handleBeginBeeperAIRun(payload []byte) ([]byte, error) {
@@
- run := aistream.NewRun(req.RunID, req.ThreadID, req.Model, req.AgentID, req.AgentName, time.Now())
+ runID := strings.TrimSpace(req.RunID)
+ if runID == "" {
+ return nil, errors.New("missing Beeper AI run ID")
+ }
+ if c.beeperAIRuns[runID] != nil {
+ return nil, errors.New("Beeper AI run is already registered")
+ }
+ run := aistream.NewRun(runID, req.ThreadID, req.Model, req.AgentID, req.AgentName, time.Now())
writer := aistream.NewWriter(run, time.Now)
writer.Start()
c.beeperAIRuns[run.RunID] = &beeperAIRunState{run: run, writer: writer}📝 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
| run := aistream.NewRun(req.RunID, req.ThreadID, req.Model, req.AgentID, req.AgentName, time.Now()) | |
| writer := aistream.NewWriter(run, time.Now) | |
| writer.Start() | |
| c.beeperAIRuns[run.RunID] = &beeperAIRunState{run: run, writer: writer} | |
| return c.marshalBeeperAIRunSnapshot(run, outboundEventsFromAGUI(run.Events)) | |
| runID := strings.TrimSpace(req.RunID) | |
| if runID == "" { | |
| return nil, errors.New("missing Beeper AI run ID") | |
| } | |
| if c.beeperAIRuns[runID] != nil { | |
| return nil, errors.New("Beeper AI run is already registered") | |
| } | |
| run := aistream.NewRun(runID, req.ThreadID, req.Model, req.AgentID, req.AgentName, time.Now()) | |
| writer := aistream.NewWriter(run, time.Now) | |
| writer.Start() | |
| c.beeperAIRuns[run.RunID] = &beeperAIRunState{run: run, writer: writer} | |
| return c.marshalBeeperAIRunSnapshot(run, outboundEventsFromAGUI(run.Events)) |
🤖 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/pickle/native/internal/core/beeper_ai_run.go` around lines 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.
| backupVersion id.KeyBackupVersion | ||
| beeperStream *beeperstream.Helper | ||
| beeperStreamMessages map[id.EventID]*beeperStreamMessage | ||
| beeperAIRuns map[string]*beeperAIRunState |
There was a problem hiding this comment.
Clear Beeper AI run state during core shutdown.
beeperAIRuns is initialized but never reset in handleClose, so stale run state can survive a close/re-init cycle and leak across sessions.
Proposed fix
func (c *Core) handleClose() ([]byte, error) {
@@
c.beeperStream = nil
+ c.beeperAIRuns = make(map[string]*beeperAIRunState)
c.appserviceProcessor = nilAlso applies to: 58-58
🤖 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/pickle/native/internal/core/core.go` at 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.
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
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "name": "pickle-monorepo", | |||
| "name": "@beeper/openclaw", | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify duplicate package names across all package.json files.
fd -a '^package\.json$' | while read -r f; do
name=$(jq -r '.name // empty' "$f")
if [ -n "$name" ]; then
printf "%s\t%s\n" "$name" "$f"
fi
done | sort | awk -F'\t' '
{
count[$1]++
files[$1]=files[$1] "\n - " $2
}
END {
for (n in count) {
if (count[n] > 1) {
print "DUPLICATE:", n, "(" count[n] " files)" files[n]
}
}
}'Repository: beeper/pickle
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant fields from root and workspace package
echo "=== ./package.json ==="
jq -r '{name, private, scripts_test_go: (.scripts["test:go"] // null)}' package.json
echo
echo "=== ./packages/openclaw/package.json ==="
jq -r '{name, private, scripts_test_go: (.scripts["test:go"] // null)}' packages/openclaw/package.json
echo
# Validate what `@beeper/pickle`'s scripts.test:go actually is (best-effort)
# Find package.json(s) with name "`@beeper/pickle`"
fd -a 'package\.json$' . -S | while read -r f; do
n=$(jq -r '.name // empty' "$f" 2>/dev/null || true)
if [ "$n" = "`@beeper/pickle`" ]; then
echo "=== $f (name=`@beeper/pickle`) ==="
jq -r '{path: "'"$f"'", scripts_test_go: (.scripts["test:go"] // null)}' "$f"
fi
done | head -n 50Repository: beeper/pickle
Length of output: 443
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all package.json files and print scripts.test:go for the package(s) named `@beeper/pickle`
fd -t f 'package\.json$' . | while read -r f; do
n="$(jq -r '.name // empty' "$f" 2>/dev/null || true)"
if [ "$n" = "`@beeper/pickle`" ]; then
echo "=== $f (name=`@beeper/pickle`) ==="
jq -r '{path: "'"$f"'", scripts_test_go: (.scripts["test:go"] // null)}' "$f"
fi
doneRepository: beeper/pickle
Length of output: 228
Avoid duplicate workspace package identity.
- Line 2: root
"name": "@beeper/openclaw"duplicatespackages/openclaw/package.json’s"name": "@beeper/openclaw", which can cause workspace/package selection ambiguity in pnpm tooling—rename the root to a distinct identity (keep itprivate) or drop the rootname. - Line 18: the
test:godelegation topnpm --filter@beeper/pickletest:gois correct;@beeper/picklerunscd native && go test -tags goolm ./....
🤖 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 `@package.json` at 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.
|
Review posted on the banner above. Highlights: Functional
Latent
Nit
|
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.