Add pickle bridge-building package with bridgev2-style runtime#4
Add pickle bridge-building package with bridgev2-style runtime#4
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds the ChangesBridge Package Core
Appservice Support in Pickle
DummyBridge Example
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 7
🧹 Nitpick comments (2)
packages/bridge/src/events.ts (1)
13-13: ⚡ Quick winConsider overloads to preserve the return type when
transactionIdis known.The current union return type
RemoteMessage | RemoteMessageWithTransactionIDforces callers to narrow the result even when they statically knowtransactionIdis present. Function overloads eliminate the cast:♻️ Proposed refactor
+export function createRemoteMessage<T>(options: CreateRemoteMessageOptions<T> & { transactionId: string }): RemoteMessageWithTransactionID; +export function createRemoteMessage<T>(options: CreateRemoteMessageOptions<T>): RemoteMessage; export function createRemoteMessage<T>(options: CreateRemoteMessageOptions<T>): RemoteMessage | RemoteMessageWithTransactionID {🤖 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/events.ts` at line 13, The function createRemoteMessage currently returns a union RemoteMessage | RemoteMessageWithTransactionID which forces callers to narrow; add two TypeScript overloads — one overload taking CreateRemoteMessageOptions<T> with a required transactionId (or a discriminant indicating presence) that returns RemoteMessageWithTransactionID, and another overload for options without transactionId that returns RemoteMessage — and keep the existing implementation signature (the broad one returning the union) unchanged; update the overloads to reference CreateRemoteMessageOptions, RemoteMessage, and RemoteMessageWithTransactionID so callers who pass a transactionId get the narrower return type without casts.packages/bridge/tsdown.config.ts (1)
4-4: ⚡ Quick winRemove
src/events.tsfrom the build entry points in tsdown.config.ts.
src/events.tsis built as an entry point, butpackage.jsonhas no./eventssubpath export—only".","./node", and"./types". SincecreateRemoteMessageis already re-exported via bothindex.tsandnode.ts, theevents.tsentry produces an unreachabledist/events.jsfile. Removing it from the entry array will clean up the build without affecting the public API.🤖 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/tsdown.config.ts` at line 4, Remove "src/events.ts" from the entry array in tsdown.config.ts so it is no longer built as an independent entry point; keep the remaining entries ["src/index.ts", "src/node.ts", "src/types.ts"], and verify that createRemoteMessage (and any public exports from events.ts) are still re-exported via index.ts and node.ts so the package.json exports (".", "./node", "./types") continue to cover the public API and no unreachable dist/events.js is produced.
🤖 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/docs/BRIDGE_TODO.md`:
- Around line 140-143: Update the checklist in BRIDGE_TODO.md to reflect the
tests added in packages/bridge/src/bridge.test.ts: mark "Runtime start/stop
tests", "Remote event queue tests" (queued remote event draining), and "Matrix
sync dispatch tests" as done/checked; leave or separately track "WASM option
forwarding tests" if still unimplemented. Reference the test file name
bridge.test.ts and the specific behaviors (runtime start/stop, queued remote
event draining, Matrix event dispatch) so future contributors see these items
are covered.
In `@packages/bridge/LICENSE`:
- Line 1: The LICENSE file currently contains only the title; replace its
contents with the full Mozilla Public License Version 2.0 text (the complete
license body, including the disclaimer, terms, and how to obtain the license),
ensuring you include the SPDX identifier "MPL-2.0" and any required
copyright/attribution placeholders; update the repository's LICENSE file so it
contains the canonical MPL 2.0 text in its entirety rather than just the title.
In `@packages/bridge/src/bridge.ts`:
- Around line 273-277: The drain logic removes items from the queue before the
handler completes, so failed conversions/sends are lost; modify the drain loop
in `#drainRemoteEvents` (invoked by `#scheduleDrain`) to peek at the next item from
this.#remoteEvents instead of shifting it immediately, call
this.#handleRemoteEvent(item) and only shift/remove the item after the handler
resolves successfully, and if the handler rejects requeue or leave the item in
place (or push it back) inside a catch so it remains for retry; ensure
`#scheduleDrain`'s retry semantics remain unchanged and reference
this.#remoteEvents, `#drainRemoteEvents`, and `#handleRemoteEvent` when implementing
the change.
- Around line 91-98: The current loadUserLogin implementation caches the client
in `#networkClients` before await client.connect(), causing half-initialized
clients to be reused if connect() rejects; change it to either memoize the
in-flight Promise or only insert the client into `#networkClients` after connect()
resolves. Concretely, in loadUserLogin (and where you call
this.connector.loadUserLogin and client.connect) either store a Promise for the
whole load flow in `#networkClients` (so concurrent callers await the same
promise) or defer calling this.#networkClients.set(login.id, client) until after
await client.connect(...) completes and has succeeded (ensure login.client is
set after successful connect as well). Ensure failed connect() removes any
in-flight entry so retries can occur.
- Around line 101-105: queueRemoteEvent currently always calls `#scheduleDrain`
even when the bridge is stopped, which causes `#drainRemoteEvents` to immediately
return (since `#context` is null) and the finally block to reschedule another
drain, potentially spinning; update the logic so queuing only triggers
scheduling when the bridge is running: check the bridge state (e.g., that
`#context` is non-null or start() has been called) before calling `#scheduleDrain`
in queueRemoteEvent and in the other enqueue sites (the similar blocks around
the other queue calls referenced), or move the guard into `#scheduleDrain` itself
so it no-ops when `#context` is null; ensure you still push to `#remoteEvents` but
avoid scheduling when stopped to prevent the microtask loop.
- Around line 183-187: The current loops broadcast Matrix events to every loaded
NetworkAPI (this.#networkClients) which causes events to be handled by
non-owning logins; change the dispatch to first determine the portal's owning
receiver (portalKey.receiver or portal.receiver) and if present, look up the
matching network client (keyed by that login id) and invoke its handler (e.g.,
handleMatrixMessage via client.handleMatrixMessage(this.#requestContext(),
msg)); if no owner exists, fall back to broadcasting to all clients that
implement the handler. Apply the same change to the other similar broadcast
sites that call handleMatrixMessage / handleMatrixTyping /
handleMatrixReadReceipt so each event routes to the owning login when a receiver
is set.
In `@packages/bridge/src/events.ts`:
- Around line 27-32: getTimestamp() and getStreamOrder() compute Date.now()/new
Date() lazily and can diverge across calls; capture a single fallback timestamp
at construction and have both getTimestamp() and getStreamOrder() return values
derived from that captured fallback (e.g., store const fallbackTimestamp =
options.timestamp ?? new Date() or fallbackMillis = options.timestamp?.getTime()
?? Date.now()) so getTimestamp() returns the same Date instance and
getStreamOrder() uses the same millis, updating the implementations of
getTimestamp and getStreamOrder to reference that stored value instead of
recomputing.
---
Nitpick comments:
In `@packages/bridge/src/events.ts`:
- Line 13: The function createRemoteMessage currently returns a union
RemoteMessage | RemoteMessageWithTransactionID which forces callers to narrow;
add two TypeScript overloads — one overload taking CreateRemoteMessageOptions<T>
with a required transactionId (or a discriminant indicating presence) that
returns RemoteMessageWithTransactionID, and another overload for options without
transactionId that returns RemoteMessage — and keep the existing implementation
signature (the broad one returning the union) unchanged; update the overloads to
reference CreateRemoteMessageOptions, RemoteMessage, and
RemoteMessageWithTransactionID so callers who pass a transactionId get the
narrower return type without casts.
In `@packages/bridge/tsdown.config.ts`:
- Line 4: Remove "src/events.ts" from the entry array in tsdown.config.ts so it
is no longer built as an independent entry point; keep the remaining entries
["src/index.ts", "src/node.ts", "src/types.ts"], and verify that
createRemoteMessage (and any public exports from events.ts) are still
re-exported via index.ts and node.ts so the package.json exports (".", "./node",
"./types") continue to cover the public API and no unreachable dist/events.js is
produced.
🪄 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: 72490ef0-b37d-4b28-a195-631c0d0096dc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
packages/bridge/LICENSEpackages/bridge/README.mdpackages/bridge/docs/BRIDGE_TODO.mdpackages/bridge/package.jsonpackages/bridge/src/bridge.test.tspackages/bridge/src/bridge.tspackages/bridge/src/events.tspackages/bridge/src/index.tspackages/bridge/src/node.tspackages/bridge/src/types.tspackages/bridge/tsconfig.jsonpackages/bridge/tsdown.config.tspackages/bridge/vitest.config.tspnpm-workspace.yamltsconfig.base.json
📜 Review details
🧰 Additional context used
🪛 LanguageTool
packages/bridge/docs/BRIDGE_TODO.md
[style] ~46-~46: This phrase is redundant (‘I’ stands for ‘Interfaces’). Use simply “APIInterfaces”.
Context: ...ta registration/migrations. ## Network API Interfaces - [x] NetworkAPI. - [x] `PushableNet...
(ACRONYM_TAUTOLOGY)
🔇 Additional comments (6)
pnpm-workspace.yaml (1)
3-3: LGTM — workspace entry correctly inserted.packages/bridge/tsconfig.json (1)
1-8: LGTM — standard tsconfig for the package.packages/bridge/vitest.config.ts (1)
1-18: LGTM — alias ordering and coverage config are correct.packages/bridge/src/index.ts (1)
1-3: LGTM — clean barrel export.packages/bridge/src/node.ts (1)
1-10: LGTM — clean Node.js integration layer.tsconfig.base.json (1)
20-22: LGTM — path aliases are consistent with the package's subpath export structure.
Introduce a minimal TypeScript Pickle example bridge (examples/dummybridge) including connector, index, env loader, file-backed state (FileState/MatrixState), smoke test, build and config files. Add Beeper bridge-manager client and helpers (packages/bridge/src/beeper.ts) with tests and export them from the bridge package. Update RuntimeBridge to initialize appservice on startup, create/backfill portal rooms via appservice, and send remote messages through appservice ghost users when appropriate. Also update docs, tests, and package exports to expose the new Beeper APIs.
Add a new createBeeperBridge helper to the bridge runtime that boots a Matrix client, infers homeserver domain, and initializes/fetches Beeper appservice registration via createBeeperAppServiceInit. Export the new API from package entry points and provide corresponding node wrapper. Introduce CreateBeeperBridgeOptions and CreateNodeBeeperBridgeOptions types and factor option-building helpers. Refactor the dummybridge example to use createBeeperBridge (simplified .env.example and README) and remove the local appservice fallback and related helper code.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/pickle/native/internal/core/core.go (1)
235-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset appservice state during close.
handleCloseclears core state but leavesc.appserviceintact. That allows appservice calls after close to use stale credentials/config from the prior session. Please clear it in the close path as well.Suggested fix
func (c *Core) handleClose() ([]byte, error) { @@ c.client = nil + c.appservice = nil c.crypto = nil c.cryptoStore = nil🤖 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` around lines 235 - 263, handleClose currently resets many Core fields but leaves c.appservice populated, which can leak stale appservice credentials; update the handleClose method to clear the appservice reference by closing it if it exposes a Close-like cleanup (call its Close/Shutdown method if present) and then set c.appservice = nil (add this alongside the other nil clears such as c.client = nil and c.crypto = nil) so no appservice state remains after close.
🧹 Nitpick comments (2)
examples/dummybridge/package.json (1)
16-22: ⚡ Quick winAlign Node typings with the declared runtime floor.
@types/nodeat major 25 withengines.node: ">=20"can let newer Node APIs typecheck even when runtime is Node 20. Either lower typings to 20.x or raise the engine floor if 25-only APIs are intended. This misalignment is present across most packages in the monorepo and should be addressed consistently.Proposed adjustment
"devDependencies": { - "@types/node": "^25.3.2", + "@types/node": "^20", "tsdown": "^0.21.10", "typescript": "^5.7.2" },🤖 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 `@examples/dummybridge/package.json` around lines 16 - 22, The package.json currently lists "@types/node" at major version 25 while "engines": { "node": ">=20" } — update these to align: either change the dev dependency "@types/node" to a 20.x spec (e.g., ^20.x) so typings match the declared runtime floor, or raise the "engines.node" floor to ">=25" if you intend to use Node 25 APIs; apply the same consistent decision across other monorepo packages (look for the "@types/node" entry and the "engines": "node" field in package.json files).packages/bridge/src/beeper.test.ts (1)
72-97: ⚡ Quick winAdd a regression test for explicit
usernameoverride.A focused case for
createBeeperBridgeManagerClient({ username: ... })would lock down behavior when the caller overrides whoami-derived usernames.Suggested test shape
+ it("uses explicit username override consistently", async () => { + const fetch = vi.fn(async (url: URL) => { + if (String(url) === "https://api.example/whoami") { + return jsonResponse({ + user: { bridges: {} }, + userInfo: { username: "alice" }, + }); + } + expect(String(url)).toContain("/_hungryserv/bob/"); + return jsonResponse({ + as_token: "as", + hs_token: "hs", + id: "sh-dummy", + namespaces: { user_ids: [{ exclusive: true, regex: "@dummy_.*:beeper.local" }] }, + sender_localpart: "dummybot", + url: "https://bridge.example", + }); + }); + + const client = createBeeperBridgeManagerClient({ + baseDomain: "example", + token: "token", + username: "bob", + fetch: fetch as never, + }); + + await expect(client.createAppServiceInit({ bridge: "sh-dummy" })).resolves.toBeDefined(); + });🤖 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/beeper.test.ts` around lines 72 - 97, Add a regression test that calls createBeeperBridgeManagerClient with an explicit username option to ensure the client uses the provided username instead of the whoami-derived one: mock fetch so the first whoami call returns a different username, create the client with { username: "alice" } and then assert getBridge("sh-dummy") and registerAppService({ bridge: "sh-dummy", getOnly: true }) behave as before (e.g., return the expected bridge version and appservice tokens/id), referencing createBeeperBridgeManagerClient, client.getBridge, and client.registerAppService to locate where to add the test.
🤖 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 `@examples/dummybridge/src/env.ts`:
- Around line 12-17: The parser currently assigns process.env[key] ??= value
even when key is empty (e.g., a line like "=value"); modify the logic around the
computed key/value (variables key and value) to skip any malformed entries with
an empty key by checking if key is a non-empty string before performing the
assignment to process.env; locate the block that computes key = trimmed.slice(0,
index).trim() and add a guard (skip/continue/return) so process.env[key] ??=
value only runs when key.length > 0.
In `@packages/bridge/README.md`:
- Around line 17-21: The README fallback snippet references registration but
never declares it; declare or import a sample registration object before it's
used so the example is runnable — e.g., add a preceding line that defines a
const registration with the expected fields (or show an import of a registration
object) so the object literal that uses homeserver, homeserverDomain and
registration is valid; ensure the declaration name matches "registration" used
in the snippet.
In `@packages/bridge/src/beeper.ts`:
- Around line 110-115: The homeserver URL is always built from
whoami.userInfo.username which ignores an explicitly set client username; in
createAppService replace the hardcoded whoami.userInfo.username usage with the
effective username that prefers this.#username when present (i.e., use
this.#username ?? whoami.userInfo.username) so hungryHomeserver(...) is called
with the same username used for requests; keep the rest of the return shape and
still call registerAppService and whoami as before.
In `@packages/pickle/native/internal/core/appservice.go`:
- Around line 339-345: The fallback error handling in ensureJoined incorrectly
returns the original join error (err) instead of the actual failure; update the
branches that call as.client(as.botUserID) and bot.InviteUser(...) so they
return botErr and inviteErr respectively when those calls fail (i.e., when
botErr != nil return botErr, and when inviteErr != nil return inviteErr),
referencing the ensureJoined function and the as.client/as.botUserID call and
bot.InviteUser(ctx, roomID, &mautrix.ReqInviteUser{UserID: cli.UserID}) to
locate the code to change.
---
Outside diff comments:
In `@packages/pickle/native/internal/core/core.go`:
- Around line 235-263: handleClose currently resets many Core fields but leaves
c.appservice populated, which can leak stale appservice credentials; update the
handleClose method to clear the appservice reference by closing it if it exposes
a Close-like cleanup (call its Close/Shutdown method if present) and then set
c.appservice = nil (add this alongside the other nil clears such as c.client =
nil and c.crypto = nil) so no appservice state remains after close.
---
Nitpick comments:
In `@examples/dummybridge/package.json`:
- Around line 16-22: The package.json currently lists "@types/node" at major
version 25 while "engines": { "node": ">=20" } — update these to align: either
change the dev dependency "@types/node" to a 20.x spec (e.g., ^20.x) so typings
match the declared runtime floor, or raise the "engines.node" floor to ">=25" if
you intend to use Node 25 APIs; apply the same consistent decision across other
monorepo packages (look for the "@types/node" entry and the "engines": "node"
field in package.json files).
In `@packages/bridge/src/beeper.test.ts`:
- Around line 72-97: Add a regression test that calls
createBeeperBridgeManagerClient with an explicit username option to ensure the
client uses the provided username instead of the whoami-derived one: mock fetch
so the first whoami call returns a different username, create the client with {
username: "alice" } and then assert getBridge("sh-dummy") and
registerAppService({ bridge: "sh-dummy", getOnly: true }) behave as before
(e.g., return the expected bridge version and appservice tokens/id), referencing
createBeeperBridgeManagerClient, client.getBridge, and client.registerAppService
to locate where to add the test.
🪄 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: 52903561-7dde-4e0a-8428-52ab213dfcca
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
examples/dummybridge/.env.exampleexamples/dummybridge/README.mdexamples/dummybridge/package.jsonexamples/dummybridge/src/connector.tsexamples/dummybridge/src/env.tsexamples/dummybridge/src/index.tsexamples/dummybridge/src/store.tsexamples/dummybridge/test/smoke.tsexamples/dummybridge/tsconfig.jsonexamples/dummybridge/tsdown.config.tspackages/bridge/README.mdpackages/bridge/docs/BRIDGE_TODO.mdpackages/bridge/package.jsonpackages/bridge/src/beeper.test.tspackages/bridge/src/beeper.tspackages/bridge/src/bridge.test.tspackages/bridge/src/bridge.tspackages/bridge/src/index.tspackages/bridge/src/node.tspackages/bridge/src/types.tspackages/bridge/tsdown.config.tspackages/pickle/native/internal/core/appservice.gopackages/pickle/native/internal/core/core.gopackages/pickle/native/internal/core/operations.gopackages/pickle/src/client-types.tspackages/pickle/src/client.tspackages/pickle/src/generated-runtime-operations.tspackages/pickle/src/generated-runtime-types.tspackages/pickle/src/index.tspackages/pickle/src/node.tspackages/pickle/src/runtime-types.ts
✅ Files skipped from review due to trivial changes (8)
- packages/bridge/src/node.ts
- examples/dummybridge/src/index.ts
- packages/bridge/src/index.ts
- packages/pickle/src/generated-runtime-types.ts
- examples/dummybridge/test/smoke.ts
- examples/dummybridge/src/connector.ts
- examples/dummybridge/tsdown.config.ts
- packages/bridge/src/bridge.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/bridge/src/bridge.ts
- packages/bridge/src/types.ts
- packages/bridge/package.json
📜 Review details
🧰 Additional context used
🪛 dotenv-linter (4.0.0)
examples/dummybridge/.env.example
[warning] 2-2: [UnorderedKey] The MATRIX_ACCESS_TOKEN key should go before the MATRIX_HOMESERVER key
(UnorderedKey)
🪛 LanguageTool
packages/bridge/docs/BRIDGE_TODO.md
[grammar] ~17-~17: Ensure spelling is correct
Context: ... Appservice portal room creation. - [x] Appservice batch backfill. - [x] Basic live Matrix...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~50-~50: This phrase is redundant (‘I’ stands for ‘Interfaces’). Use simply “APIInterfaces”.
Context: ...ta registration/migrations. ## Network API Interfaces - [x] NetworkAPI. - [x] `PushableNet...
(ACRONYM_TAUTOLOGY)
examples/dummybridge/README.md
[grammar] ~34-~34: Ensure spelling is correct
Context: ...AppServiceInit()` to fetch/register the appservice through Beeper's bridge-manager-compati...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (18)
examples/dummybridge/src/env.ts (2)
19-21: Good error boundary in file loading.Ignoring only
ENOENTwhile rethrowing everything else is the right behavior for a best-effort.envloader.
24-28:requiredEnvis clean and fail-fast.The helper is simple and gives a clear missing-variable error path.
examples/dummybridge/.env.example (1)
1-19: Config template is clear and practical.Required vs optional env vars are separated well, and placeholder values reduce accidental secret leakage in examples.
examples/dummybridge/README.md (1)
1-47: README flow is well-structured and runnable.The setup, smoke test, and live-run paths are documented clearly and map cleanly to the package scripts/env flags.
examples/dummybridge/tsconfig.json (1)
1-9: TypeScript project config looks correct for this example package.Includes/excludes and output path are aligned with the documented build and smoke workflow.
packages/pickle/src/runtime-types.ts (1)
12-23: Appservice runtime type exports look consistent.These additions fit the existing
generated-runtime-typesre-export pattern and keep the public surface coherent.packages/pickle/native/internal/core/operations.go (1)
20-31: New appservice operation declarations are cleanly integrated.Operation IDs and
ts:operationmetadata match the existing convention and should generate the expected runtime bindings.packages/pickle/src/client-types.ts (1)
73-82:MatrixAppservicetyping surface is well-defined.The new namespace contract is explicit and aligns with the rest of the client API shape.
Also applies to: 86-88, 111-118
packages/pickle/src/node.ts (1)
38-40: Node client appservice getter matches existing namespace pattern.No concerns with this addition.
packages/pickle/src/client.ts (1)
41-42: Client-side appservice wiring is consistent with existing namespaces.The mapping is straightforward and follows established core delegation patterns.
Also applies to: 74-84
packages/pickle/src/index.ts (1)
7-7: Public exports for appservice types are properly surfaced.No issues found in this export expansion.
Also applies to: 21-34
packages/pickle/src/generated-runtime-operations.ts (3)
6-13: Appservice type imports are clean and complete.Line 6-Line 13 add only the new appservice-related type imports and keep them type-only, which matches the generated-runtime typing pattern.
96-101: Interface extension is consistent with existing operation contracts.Line 96-Line 101 adds a coherent appservice surface to
MatrixCoreOperationswith clear option/result typing and no contract ambiguity.
189-211: Caller delegation wiring looks correct.Line 189-Line 211 correctly delegates each new appservice method to the expected runtime operation string using the same call pattern as the rest of the class.
packages/bridge/docs/BRIDGE_TODO.md (1)
143-147: Test checklist is still stale versus current suite coverage.The unchecked items for runtime start/stop, remote queue draining, and Matrix dispatch were already flagged and still appear unresolved.
packages/bridge/tsdown.config.ts (1)
3-19: Build config looks consistent with the new package surface.Entry points, ESM output, and externalization choices are aligned with the added bridge/beeper exports.
examples/dummybridge/src/store.ts (2)
11-95:FileStateLGTM.The lock acquire/release logic is sound: the check and in-memory mutation are both synchronous before the first
await, so the Node.js single-threaded event loop prevents any JS-level race on lock state.#cleanExpiredLocksis a nice defensive touch on bothconnect()andacquireLock.
97-143:MatrixStateLGTM.Namespace isolation, base64 round-trip, and the index-backed
list()with incremental updates are all correct. Key namespacing is applied consistently inget/set/deletewhile the index stores raw keys, solist(prefix)filters correctly without any prefix confusion.
Introduce a persistent BridgeDataStore and MatrixBridgeDataStore (createBridgeDataStore) and wire it into the runtime so portals, messages and user logins can be persisted. Add high-level APIs to create bridges from a Beeper token or username/password (createBeeperBridgeFromToken / createBeeperBridgeFromPassword) and corresponding node helpers that default to a file-backed Matrix store. Update RuntimeBridge to accept an optional dataStore and persist relevant state; export store types/creators. Update example dummybridge to use BEEPER_* env vars and the new state-file package (remove the example FileState), and update package exports, deps, tsdown and vitest config accordingly.
Replace token/password entrypoints with a unified account-based flow: add loginWithPassword helper and accept a MatrixAccount on createBeeperBridge. Remove createBeeperBridgeFromToken/createBeeperBridgeFromPassword plumbing and consolidate token/homeserver handling from the account. Update types, node helpers, README and example to use loginWithPassword/createBeeperBridge, adjust .env.example to prefer username/password, and add tests for password login behavior. Also introduce loginWithBeeperPassword helper and minor refactors around appservice initialization and runtime options.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@examples/dummybridge/src/index.ts`:
- Around line 16-30: The example ignores documented manager settings
BEEPER_BASE_DOMAIN and DUMMYBRIDGE_URL; thread these into bridge creation by
adding them to the bridge options passed into createBeeperBridgeFromPassword and
createBeeperBridgeFromToken. Update bridgeOptions (the object used when calling
createBeeperBridgeFromPassword and createBeeperBridgeFromToken) to include
values from optionalEnv/requiredEnv for BEEPER_BASE_DOMAIN (e.g. baseDomain) and
DUMMYBRIDGE_URL (e.g. connectorUrl or homeserverUrl) so the factories receive
and propagate those manager settings.
- Around line 78-82: The signal handlers installed with process.once currently
call await bridge.stop() without handling rejection, leaving an unobserved
promise that can prevent process.exit(0); update the handler defined for each
signal (the process.once(async () => { ... }) block) to wrap the bridge.stop()
call in a try/catch (or use .catch()) so any rejection is caught, log the error
(e.g., console.error or the project's logger), and ensure process.exit is always
called (use exit code 0 on success and non‑zero on failure) in the
finally/after-catch branch so shutdown completes even if bridge.stop() fails.
In `@packages/bridge/src/bridge.ts`:
- Around line 77-81: The cache key for createBeeperBridgeFromPassword currently
only uses baseDomain and username (accountKey), causing different
matrix.homeserver values to collide; update the key generation (accountKey) to
also include the homeserver from options.matrix?.homeserver (or a sensible
default) so getAccount/setAccount use a unique key per homeserver, ensuring
loginWithMatrixPassword and stored account entries are not incorrectly reused
across environments.
- Around line 126-140: The startup path in start() never reloads persisted
portals from the bridge's data store into the in-memory maps, so entries written
to `#dataStore` aren't populated into `#portalsByKey` and `#portalsByRoom` after a
restart; add logic (e.g. a private method like `#hydratePortalsFromStore`) that
reads all saved portal records from this.#dataStore, recreates the Portal
objects/state and inserts them into this.#portalsByKey and this.#portalsByRoom,
and call that method in start() after this.#context is created (and before await
this.#subscribeMatrixEvents()) so lookups work for incoming matrix events.
Ensure duplicate keys are handled and any errors are logged but do not prevent
initialization.
- Around line 232-233: flushRemoteEvents currently calls `#drainRemoteEvents`
directly which can start a concurrent drain; instead have flushRemoteEvents join
the in-flight scheduled drain by checking the private `#drainPromise` and awaiting
it if present, otherwise kick off the drain in the same serialized path so
`#drainPromise` is set and awaited (e.g., await this.#drainPromise if defined, or
set this.#drainPromise = this.#drainRemoteEvents() and await it, ensuring
`#drainPromise` is cleared after completion). This preserves
ordering/serialization and avoids starting a second concurrent drain.
- Around line 222-226: When registering a portal in registerPortal, first lookup
the existing portal via
this.#portalsByKey.get(portalKeyString(portal.portalKey)); if an existing portal
exists and has an mxid different from the incoming portal.mxid, remove the old
room mapping from this.#portalsByRoom (delete the entry keyed by the old
portal.mxid) before updating this.#portalsByKey and adding the new
this.#portalsByRoom entry; this ensures stale room IDs are not left pointing to
the moved portal.
In `@packages/bridge/src/store.ts`:
- Around line 66-69: setPortal currently only writes the new portal-mxid entry,
leaving an old mxid -> portalKey mapping when a portal moves rooms; update
setPortal to first load the existing portal (use portalStoreKey and the store
get method used elsewhere), and if an existingPortal.mxid exists and !==
portal.mxid, remove the old index by deleting key("portal-mxid",
existingPortal.mxid) before writing the new mapping with this.#set; also ensure
you delete the old mapping when the incoming portal has no mxid. Use the same
internal primitives (e.g., this.#set and the store delete method) and
key("portal-mxid", ...) to locate entries.
🪄 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: a9cb16d9-16b5-4b98-bc2e-43a86de673b4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
examples/dummybridge/.env.exampleexamples/dummybridge/README.mdexamples/dummybridge/package.jsonexamples/dummybridge/src/index.tspackages/bridge/README.mdpackages/bridge/package.jsonpackages/bridge/src/bridge.tspackages/bridge/src/index.tspackages/bridge/src/node.tspackages/bridge/src/store.tspackages/bridge/src/types.tspackages/bridge/tsdown.config.tspackages/bridge/vitest.config.ts
✅ Files skipped from review due to trivial changes (6)
- packages/bridge/tsdown.config.ts
- examples/dummybridge/package.json
- packages/bridge/vitest.config.ts
- examples/dummybridge/README.md
- packages/bridge/README.md
- packages/bridge/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bridge/src/index.ts
- packages/bridge/src/types.ts
Add support for management rooms and matrix-side command handling across the bridge. Introduces CommandHandlingBridgeConnector and implements command dispatching, text replies and management-room creation in RuntimeBridge; updates the dummy example connector to handle commands and several new login flows (password, cookies, local_storage, display_and_wait). Persist and expose bridge state, ghosts and message requests via the data store; add backfillMessages, remote backfill/chat-info/delete handling, media upload/download wrappers and identifier/message-request APIs. Bind login process lifecycle calls to the bridge request context and update tests to cover the new behaviours.
Add support for posting bridge state to the bridge manager and introduce a bridgeType option. registerAppService now posts a bridge_state POST after hungry registration (unless postState is false) and a new postBridgeState method and PostBridgeStateOptions type were added. The internal request helper now accepts POST and an optional token and safely handles empty responses. bridgeType was threaded through types, beeper, bridge, and index/entry exports; bridgeStateEvent logic selects STARTING/RUNNING based on bridgeType and known bridge names. The Node entrypoint was consolidated into the package index (exports adjusted) and createBeeperBridge/createBridge now auto-wire matrix clients and default file stores; examples updated to import from the package root and handle optional address. Tests and README were updated to reflect the new behaviour, and a .gitignore was added to the dummybridge example.
Introduce AppserviceWebsocket (src/appservice-websocket.ts) to handle appservice websocket connections, parse transactions/http_proxy requests, dispatch Matrix events to the bridge, and manage ping/reconnect behavior. Add unit tests (src/appservice-websocket.test.ts) covering normal transactions and http_proxy handling. Wire the websocket into the runtime bridge (start/stop and dispatch integration), and add a guard to skip starting when a push URL is configured. Add runtime/dev dependencies for ws and @types/ws, update tsdown config to include the new module and avoid bundling ws, and update the pnpm lockfile accordingly.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (7)
packages/bridge/src/bridge.ts (7)
495-499:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRoute Matrix events to the owning login instead of broadcasting them.
These handlers still fan each Matrix event out to every loaded
NetworkAPI. Once multiple logins are active, a room owned byportalKey.receiver = login:awill also invoke handlers onlogin:b, which can duplicate or misroute outbound network actions. Resolve the owner from the portal receiver and dispatch only to that client when one is set.Also applies to: 551-555, 571-575, 596-600
🤖 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 495 - 499, The loop broadcasting Matrix events to every NetworkAPI (iterating this.#networkClients and calling client.handleMatrixMessage) must instead resolve the portal owner from the incoming msg (use portalKey.receiver or equivalent field on the event) and dispatch only to the matching network client for that login; if a portal receiver is set, lookup the correct client in this.#networkClients by that login key and call handleMatrixMessage(this.#requestContext(), msg) only on that client, otherwise fall back to iterating all clients; apply the same change to the other similar blocks (the loops at the other noted ranges) and keep using hasMethod to guard handleMatrixMessage as before.
265-273:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't cache a login before
connect()succeeds.This publishes the client to
#networkClientsbeforeawait client.connect(...)finishes. A failed connect leaves a half-initialized client cached, and concurrent callers can still race into duplicate sessions. Store an in-flight promise here, or only cache the client after a successful connect and clear failures for retry.🤖 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 265 - 273, The current loadUserLogin stores the client in `#networkClients` before client.connect finishes; change loadUserLogin to avoid caching a half-initialized client by either (a) store an in-flight promise in `#networkClients` keyed by login.id (the promise resolves to the connected client) so concurrent callers await the same connect, or (b) only set `#networkClients` and call `#dataStore.setUserLogin` after await client.connect(...) succeeds, and ensure you remove the entry from `#networkClients` if connect throws so retries can proceed; modify references in loadUserLogin, connector.loadUserLogin, client.connect, and `#dataStore`?.setUserLogin accordingly.
396-400:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop the previous room mapping when re-registering a portal.
If the same portal key is registered with a new
mxid, the old#portalsByRoomentry is left behind. Dispatch for the stale room ID will still resolve to this portal and misroute later Matrix 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/bridge/src/bridge.ts` around lines 396 - 400, When re-registering a portal in registerPortal, clear any stale room mapping: lookup the existing portal via this.#portalsByKey.get(portalKeyString(portal.portalKey)); if an existing portal has an mxid different from portal.mxid, call this.#portalsByRoom.delete(existing.mxid) before setting the new entries. Then continue to set this.#portalsByKey and (if portal.mxid) this.#portalsByRoom as currently implemented to ensure the old room key is not left behind.
136-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHydrate persisted portals during startup.
registerPortal()persists portals, butstart()never rebuilds#portalsByKey/#portalsByRoomfrom#dataStore. After a restart,#portalForRemoteEvent()cannot resolve previously registered portals, so remote message/backfill/delete handling fails until something re-registers them in memory.🤖 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 136 - 150, start() never rebuilds in-memory portal indexes from persisted storage, so `#portalsByKey` and `#portalsByRoom` remain empty after a restart; after creating this.#context (and before connector.init/start) iterate the persisted portals in this.#dataStore and hydrate those maps (or call an internal helper used by registerPortal without re-persisting) so that existing entries can be resolved by `#portalForRemoteEvent`; reference registerPortal, `#portalsByKey`, `#portalsByRoom`, `#dataStore` and `#portalForRemoteEvent` when adding the hydration logic and ensure you avoid duplicating persistence for already-stored portals.
410-411:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
flushRemoteEvents()join the serialized drain.Calling
#drainRemoteEvents()directly bypasses#drainPromise. If a scheduled drain is already running,flushRemoteEvents()can process the queue concurrently and break ordering/serialization. Reuse#scheduleDrain()and await the shared promise 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/bridge/src/bridge.ts` around lines 410 - 411, Change flushRemoteEvents to use the shared drain scheduling promise instead of invoking the private drain directly: replace the direct call to this.#drainRemoteEvents() with a call to this.#scheduleDrain() and await the resulting this.#drainPromise so the method participates in the serialized drain flow; ensure you reference the existing private members `#scheduleDrain`, `#drainPromise`, and `#drainRemoteEvents` when making this change so concurrent drains are joined rather than run concurrently.
390-393:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAvoid scheduling the remote-event drain while the bridge is stopped.
queueRemoteEvent()always kicks#scheduleDrain(), but#drainRemoteEvents()immediately returns when#contextis null. Thefinallyblock then reschedules because the backlog is still non-empty, so queuing beforestart()or afterstop()can spin forever. Gate scheduling on the bridge being active, and kick the drain once startup finishes if backlog already exists.Also applies to: 621-625, 628-629
🤖 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 390 - 393, queueRemoteEvent currently always calls `#scheduleDrain` even when the bridge is stopped, which can cause an infinite reschedule loop because `#drainRemoteEvents` returns early if `#context` is null; update queueRemoteEvent to check bridge active state (e.g., ensure `#context` is non-null or a boolean isRunning) before calling `#scheduleDrain`, and if the bridge is not active do not schedule the drain but leave the event queued and ensure start() triggers a single `#scheduleDrain` if backlog exists; apply the same guard to the other queuing call sites referenced (the other methods that push into `#remoteEvents` and currently call `#scheduleDrain`) so scheduling only happens when the bridge is active and startup triggers the backlog drain.
630-633:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep queued remote events until their handler succeeds.
#drainRemoteEvents()removes the item before#handleRemoteEvent()resolves, so any conversion/send failure drops that event permanently. Peek first and remove only after success, or requeue on failure.🤖 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 630 - 633, The drain loop in `#drainRemoteEvents` removes items from this.#remoteEvents before awaiting `#handleRemoteEvent`, which loses events on conversion/send failures; change the loop to peek at the head (use this.#remoteEvents[0] or similar), await this.#handleRemoteEvent(item.login, item.event), and only remove the item (shift) after the await succeeds; on error, do not drop the item—either requeue it or break/return so it can be retried later (ensure you reference `#drainRemoteEvents`, this.#remoteEvents and `#handleRemoteEvent` when updating the logic).
🤖 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/package.json`:
- Line 61: The `@types/node` devDependency currently pins major version 25
("@types/node": "^25.3.2") which mismatches the package's Node engine (">=20");
update the dependency to a `@types/node` major that matches the minimum engine
(for example a 20.x series like "^20.0.0") in this package.json and do the same
for the other occurrences referenced (lines 75–77) so TypeScript only exposes
Node APIs compatible with Node >=20.
- Line 51: prepublishOnly currently only runs the guard script and doesn't
ensure dist/ is rebuilt; update the prepublishOnly script in package.json to run
the package's build (e.g., invoke the repo or workspace build command such as a
pnpm build step or the package's "build" script) before calling node
../../scripts/guard-pnpm-publish.mjs so a fresh dist/ is produced prior to the
publish check; reference the prepublishOnly script and the
guard-pnpm-publish.mjs invocation when making the change.
In `@packages/bridge/src/appservice-websocket.test.ts`:
- Line 2: Replace the value import of AddressInfo with a type-only import to
reflect its usage in type casts: change the import to use "type" (i.e., import
type { AddressInfo } from "node:net";) so the compiler/tooling treats it as a
type-only dependency; update the existing import statement that brings in
AddressInfo (used in the test casts) accordingly.
In `@packages/bridge/src/appservice-websocket.ts`:
- Around line 74-94: The `#handleMessage` method can throw if await
this.#dispatch(...) or await this.#handleHTTPProxy(...) rejects, leaving the
homeserver without a response; wrap the async work for the "transaction" loop
and the "http_proxy" branch in try/catch blocks and on error call
this.#send(messageResponse(message, false, { code: "M_UNKNOWN", message:
error.message || String(error), details: error })) so each incoming message
(identified by message.txn_id/message.id) always gets either a success or an
error frame; ensure you catch errors from the per-event await
this.#dispatch(...) loop as well as the await this.#handleHTTPProxy(...) call
and send the error response before rethrowing or swallowing as appropriate.
- Around line 174-180: The check in messageResponse incorrectly treats id===0 as
falsy and ignores valid requests; change the guard in messageResponse to only
reject null/undefined ids (e.g., use message.id == null or message.id ===
undefined || message.id === null) instead of !message.id so that id 0 is handled
and the function still returns response/error for valid requests (keep the rest
of the logic and returned object using message.id).
In `@packages/pickle/src/auth.ts`:
- Around line 45-46: The error messages in loginWithPassword are misleading
because they only mention BEEPER_USERNAME/BEEPER_PASSWORD; update the thrown
Error texts (the checks that use username and password) to reference both
supported environment variable fallbacks (BEEPER_USERNAME and MATRIX_USERNAME
for username, BEEPER_PASSWORD and MATRIX_PASSWORD for password) so callers see
the correct env var options when misconfigured; locate the checks in
loginWithPassword where it throws on missing username/password and change the
messages accordingly.
---
Duplicate comments:
In `@packages/bridge/src/bridge.ts`:
- Around line 495-499: The loop broadcasting Matrix events to every NetworkAPI
(iterating this.#networkClients and calling client.handleMatrixMessage) must
instead resolve the portal owner from the incoming msg (use portalKey.receiver
or equivalent field on the event) and dispatch only to the matching network
client for that login; if a portal receiver is set, lookup the correct client in
this.#networkClients by that login key and call
handleMatrixMessage(this.#requestContext(), msg) only on that client, otherwise
fall back to iterating all clients; apply the same change to the other similar
blocks (the loops at the other noted ranges) and keep using hasMethod to guard
handleMatrixMessage as before.
- Around line 265-273: The current loadUserLogin stores the client in
`#networkClients` before client.connect finishes; change loadUserLogin to avoid
caching a half-initialized client by either (a) store an in-flight promise in
`#networkClients` keyed by login.id (the promise resolves to the connected client)
so concurrent callers await the same connect, or (b) only set `#networkClients`
and call `#dataStore.setUserLogin` after await client.connect(...) succeeds, and
ensure you remove the entry from `#networkClients` if connect throws so retries
can proceed; modify references in loadUserLogin, connector.loadUserLogin,
client.connect, and `#dataStore`?.setUserLogin accordingly.
- Around line 396-400: When re-registering a portal in registerPortal, clear any
stale room mapping: lookup the existing portal via
this.#portalsByKey.get(portalKeyString(portal.portalKey)); if an existing portal
has an mxid different from portal.mxid, call
this.#portalsByRoom.delete(existing.mxid) before setting the new entries. Then
continue to set this.#portalsByKey and (if portal.mxid) this.#portalsByRoom as
currently implemented to ensure the old room key is not left behind.
- Around line 136-150: start() never rebuilds in-memory portal indexes from
persisted storage, so `#portalsByKey` and `#portalsByRoom` remain empty after a
restart; after creating this.#context (and before connector.init/start) iterate
the persisted portals in this.#dataStore and hydrate those maps (or call an
internal helper used by registerPortal without re-persisting) so that existing
entries can be resolved by `#portalForRemoteEvent`; reference registerPortal,
`#portalsByKey`, `#portalsByRoom`, `#dataStore` and `#portalForRemoteEvent` when adding
the hydration logic and ensure you avoid duplicating persistence for
already-stored portals.
- Around line 410-411: Change flushRemoteEvents to use the shared drain
scheduling promise instead of invoking the private drain directly: replace the
direct call to this.#drainRemoteEvents() with a call to this.#scheduleDrain()
and await the resulting this.#drainPromise so the method participates in the
serialized drain flow; ensure you reference the existing private members
`#scheduleDrain`, `#drainPromise`, and `#drainRemoteEvents` when making this change so
concurrent drains are joined rather than run concurrently.
- Around line 390-393: queueRemoteEvent currently always calls `#scheduleDrain`
even when the bridge is stopped, which can cause an infinite reschedule loop
because `#drainRemoteEvents` returns early if `#context` is null; update
queueRemoteEvent to check bridge active state (e.g., ensure `#context` is non-null
or a boolean isRunning) before calling `#scheduleDrain`, and if the bridge is not
active do not schedule the drain but leave the event queued and ensure start()
triggers a single `#scheduleDrain` if backlog exists; apply the same guard to the
other queuing call sites referenced (the other methods that push into
`#remoteEvents` and currently call `#scheduleDrain`) so scheduling only happens when
the bridge is active and startup triggers the backlog drain.
- Around line 630-633: The drain loop in `#drainRemoteEvents` removes items from
this.#remoteEvents before awaiting `#handleRemoteEvent`, which loses events on
conversion/send failures; change the loop to peek at the head (use
this.#remoteEvents[0] or similar), await this.#handleRemoteEvent(item.login,
item.event), and only remove the item (shift) after the await succeeds; on
error, do not drop the item—either requeue it or break/return so it can be
retried later (ensure you reference `#drainRemoteEvents`, this.#remoteEvents and
`#handleRemoteEvent` when updating the logic).
🪄 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: effce4a8-ad0d-497c-b8d9-d55eed35570d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
examples/dummybridge/.env.exampleexamples/dummybridge/.gitignoreexamples/dummybridge/README.mdexamples/dummybridge/src/connector.tsexamples/dummybridge/src/index.tspackages/bridge/README.mdpackages/bridge/package.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/index.tspackages/bridge/src/node.tspackages/bridge/src/store.tspackages/bridge/src/types.tspackages/bridge/tsdown.config.tspackages/pickle/src/auth.test.tspackages/pickle/src/auth.ts
✅ Files skipped from review due to trivial changes (7)
- examples/dummybridge/.gitignore
- packages/bridge/tsdown.config.ts
- examples/dummybridge/README.md
- packages/bridge/README.md
- packages/bridge/src/node.ts
- packages/bridge/src/beeper.ts
- packages/bridge/src/store.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/bridge/src/index.ts
- examples/dummybridge/src/connector.ts
- packages/bridge/src/types.ts
📜 Review details
🧰 Additional context used
🪛 dotenv-linter (4.0.0)
examples/dummybridge/.env.example
[warning] 2-2: [UnorderedKey] The BEEPER_PASSWORD key should go before the BEEPER_USERNAME key
(UnorderedKey)
🪛 OpenGrep (1.20.0)
packages/bridge/src/appservice-websocket.ts
[ERROR] 100-100: 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 (1)
examples/dummybridge/src/index.ts (1)
90-94: Still unresolved: observe shutdown failures in the signal handler.
bridge.stop()can still reject out of this async listener, which leaves the shutdown path unhandled and can skip the intended exit flow.
Introduce HTTP proxy handling and richer logging for the appservice websocket, including onOpen hooks and message/http proxy debug logs. Implement provisioning endpoints and login flow handling in the bridge (start/step/complete), plus bridge status broadcasting over the websocket and management command replies sent via the appservice bot when available. Add implicit management room creation, command parsing/logging, and helper utilities for JSON responses and login step serialization. Update example dummy bridge to support configurable bridge name/sender, create/backfill multiple dummy chats on startup, and generate multiple historical messages. Adjust createBeeperBridge/store types and validation to require/accept a store in Node usage. Add a test verifying management command replies go through the appservice bot.
Add robust AppserviceWebsocket features and bridge lifecycle/command handling. The websocket now supports configurable timing, capped exponential reconnect backoff, ping/ping-timeout detection, replacement-close handling, and improved close/reconnect bookkeeping; tests for reconnect, ping timeouts, and replacement behavior were added and helper utilities extracted. RuntimeBridge now persists and restores bridge status and user logins, auto-loads persisted logins on startup, tracks per-login bridge state, supports bridgev2-style backfill queueing and message checkpoints, and implements built-in management commands (help, login, logout, cancel-login, set-management-room) with persistence for management rooms; numerous tests and helper mocks were added. Related updates include new/extended types, store interactions, and small API helpers for error/timestamp/TTL handling to support the above.
Move ghost MXID generation into the bridge API and update connectors/examples accordingly. - Remove makeGhostMxid and per-connector sender/server options; DummyConnector now calls ctx.bridge.ghostUserId and injects a ghostUserId function into DummyNetworkAPI. - Simplify remote message helpers to accept ghostUserId callbacks and adjust signatures/usages across the example and test code. - Add RuntimeBridge.ghostUserId and helper functions to derive ghost IDs from appservice registration (ghostUserIdFromRegistration, userIdFromNamespaceRegex, escapeMatrixLocalpart, unescapeRegexLiteral). - Improve backfill task/response handling to use stripUndefined and produce a cleaner task/result shape; add messageCheckpointPayload helper. - Make homeserverDomain optional in CreateBeeperBridgeOptions and propagate it properly when present. - Native side: set host on matrixAppservice and configure HTTP client with host when initializing appservice. These changes centralize ghost user ID logic in the bridge, simplify connector configuration, and ensure consistent ghost ID generation from appservice registrations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/pickle/native/internal/core/appservice.go (1)
342-348:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn fallback failures directly in
ensureJoined.At Line 344 and Line 347, the code returns the original join error instead of
botErr/inviteErr, which masks the real failure source.Suggested fix
if err != nil { bot, botErr := as.client(as.botUserID) if botErr != nil { - return err + return botErr } if _, inviteErr := bot.InviteUser(ctx, roomID, &mautrix.ReqInviteUser{UserID: cli.UserID}); inviteErr != nil { - return err + return inviteErr } resp, err = cli.JoinRoomByID(ctx, roomID) if err != nil { return err } }🤖 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/appservice.go` around lines 342 - 348, In ensureJoined, the function currently returns the wrong error variable when the bot client creation or invite fails; update the error returns to propagate the actual failures by returning botErr after the as.client(as.botUserID) failure and returning inviteErr (or the invite error) after the bot.InviteUser call so the real failure source from as.client and bot.InviteUser is exposed.
🧹 Nitpick comments (1)
packages/bridge/src/bridge.ts (1)
1106-1108: 💤 Low valueRemove dead ternary expression.
This ternary always evaluates to
"m.room.message"regardless of the condition, making the check pointless.-const type = typeof content.msgtype === "string" ? "m.room.message" : "m.room.message"; +const type = "m.room.message";🤖 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 1106 - 1108, In sendMessage, the ternary used to set the local variable "type" inside the sendMessage method is dead (it always resolves to "m.room.message"); remove the ternary and assign the constant directly by replacing the conditional expression with a single constant assignment (e.g., set type to "m.room.message") so the code in sendMessage is simpler and clearer.
🤖 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/pickle/native/internal/core/appservice.go`:
- Around line 118-123: The current validation incorrectly treats
req.HomeserverDomain as mandatory and returns an error when it's empty; update
the validation in the function that checks req to only require req.Homeserver
(not req.HomeserverDomain), leaving homeserverDomain optional, and keep the
registration checks for req.Registration.AppToken,
req.Registration.SenderLocalpart and req.Registration.ID intact; also update the
error message to accurately reflect which fields are required so callers and
logs are clear (reference the req.Homeserver, req.HomeserverDomain, and
req.Registration.* symbols).
---
Duplicate comments:
In `@packages/pickle/native/internal/core/appservice.go`:
- Around line 342-348: In ensureJoined, the function currently returns the wrong
error variable when the bot client creation or invite fails; update the error
returns to propagate the actual failures by returning botErr after the
as.client(as.botUserID) failure and returning inviteErr (or the invite error)
after the bot.InviteUser call so the real failure source from as.client and
bot.InviteUser is exposed.
---
Nitpick comments:
In `@packages/bridge/src/bridge.ts`:
- Around line 1106-1108: In sendMessage, the ternary used to set the local
variable "type" inside the sendMessage method is dead (it always resolves to
"m.room.message"); remove the ternary and assign the constant directly by
replacing the conditional expression with a single constant assignment (e.g.,
set type to "m.room.message") so the code in sendMessage is simpler and clearer.
🪄 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: 5eccd9b6-d07f-48c6-87ef-d66f81ca0f61
📒 Files selected for processing (11)
examples/dummybridge/src/connector.tsexamples/dummybridge/src/index.tsexamples/dummybridge/test/smoke.tspackages/bridge/src/appservice-websocket.test.tspackages/bridge/src/appservice-websocket.tspackages/bridge/src/bridge.test.tspackages/bridge/src/bridge.tspackages/bridge/src/index.tspackages/bridge/src/store.tspackages/bridge/src/types.tspackages/pickle/native/internal/core/appservice.go
✅ Files skipped from review due to trivial changes (1)
- packages/bridge/src/store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/dummybridge/src/index.ts
📜 Review details
🧰 Additional context used
🪛 OpenGrep (1.20.0)
packages/bridge/src/bridge.ts
[ERROR] 650-650: 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] 660-660: 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] 1218-1218: 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] 1220-1220: 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/bridge/src/bridge.test.ts
[ERROR] 727-727: 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/bridge/src/appservice-websocket.ts
[ERROR] 249-249: 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 (17)
packages/bridge/src/appservice-websocket.ts (4)
344-351: Treat websocket request id0as valid.The
!message.idcheck at line 345 drops legitimate request IDs like0, so those commands will never get an ack/error back. This check should only rejectundefined/null.Suggested fix
function messageResponse(message: WebsocketMessage, ok: boolean, data: unknown): WebsocketRequest | null { - if (!message.id || message.command === "response" || message.command === "error") return null; + if (message.id == null || message.command === "response" || message.command === "error") return null; return { command: ok ? "response" : "error", data, id: message.id, }; }
200-237: Reply with anerrorframe when message handling throws.If
#dispatch()or#handleHTTPProxy()fails, this method rejects and the caller only logs it. The homeserver never gets a response for that requestid, so the transaction can hang or be retried indefinitely. Wrap the async work in try/catch blocks and send an error frame on failure.Suggested fix
async `#handleMessage`(data: WebSocket.RawData): Promise<void> { const message = JSON.parse(data.toString()) as WebsocketMessage; this.#log("debug", "appservice_websocket_message", { command: message.command ?? "transaction", eventCount: message.events?.length, id: message.id, txnId: message.txn_id, }); if (this.#closeFromCommand(message)) return; if (this.#handlePingResponse(message)) return; if (message.command === "connect") return; if (message.command === "ping") { this.#send(messageResponse(message, true, message.data ?? { timestamp: Date.now() })); return; } if (message.command === "response" || message.command === "error") return; if (!message.command || message.command === "transaction") { - for (const raw of message.events ?? []) { - const event = rawMatrixEvent(raw); - this.#log("debug", "appservice_websocket_transaction_event", { - eventId: raw.event_id, - roomId: raw.room_id, - sender: raw.sender, - type: raw.type, - }); - if (event) await this.#dispatch(event); + try { + for (const raw of message.events ?? []) { + const event = rawMatrixEvent(raw); + this.#log("debug", "appservice_websocket_transaction_event", { + eventId: raw.event_id, + roomId: raw.room_id, + sender: raw.sender, + type: raw.type, + }); + if (event) await this.#dispatch(event); + } + this.#send(messageResponse(message, true, { txn_id: message.txn_id })); + } catch (error: unknown) { + this.#send(messageResponse(message, false, { code: "M_UNKNOWN", message: error instanceof Error ? error.message : String(error) })); } - this.#send(messageResponse(message, true, { txn_id: message.txn_id })); return; } if (message.command === "http_proxy") { - const response = await this.#handleHTTPProxy(message.data); - this.#log("debug", "appservice_websocket_http_proxy_response", { id: message.id, status: response.status }); - this.#send(messageResponse(message, true, response)); + try { + const response = await this.#handleHTTPProxy(message.data); + this.#log("debug", "appservice_websocket_http_proxy_response", { id: message.id, status: response.status }); + this.#send(messageResponse(message, true, response)); + } catch (error: unknown) { + this.#send(messageResponse(message, false, { code: "M_UNKNOWN", message: error instanceof Error ? error.message : String(error) })); + } return; } this.#send(messageResponse(message, false, { code: "M_UNKNOWN", message: `unknown websocket command ${message.command}` })); }
1-70: LGTM!The WebSocket client setup with typed options, timing configuration, and private state is well-structured. The default timing values are reasonable for production use.
272-289: LGTM!The ping logic correctly handles stale pings by terminating the socket, sets up the timeout timer, and cleans up state appropriately. The guard against sending a new ping while one is pending is a good safeguard.
packages/bridge/src/appservice-websocket.test.ts (2)
2-2: Use a type-only import forAddressInfo.
AddressInfois only used in type casts (lines 24 and 79). Usingtypeimports makes the intent explicit.-import { AddressInfo } from "node:net"; +import type { AddressInfo } from "node:net";
18-234: LGTM!The test suite provides excellent coverage of the AppserviceWebsocket behaviors: transaction dispatch with response matching, HTTP proxy handling, exponential backoff with stable-connection reset, ping timeout reconnection, and replacement close handling. The shared cleanup pattern in
afterEachprevents resource leaks between tests.packages/bridge/src/bridge.ts (6)
343-358: Avoid caching a login beforeconnect()succeeds.Line 350 adds the client to
#networkClientsbeforeawait client.connect()at line 354. Ifconnect()rejects, laterloadUserLogin()calls return that half-initialized client and never retry; concurrent callers can also race past the firstget()and establish duplicate sessions.Consider memoizing the in-flight Promise or only inserting into
#networkClientsafter a successful connect.
495-498: Don't reschedule the drain while the bridge is stopped.
queueRemoteEvent()always kicks#scheduleDrain(), but#drainRemoteEvents()exits immediately when#contextis null. Thefinallyblock at line 996 then reschedules another drain while backlog is still non-empty, so queuing events beforestart()or afterstop()can spin an endless microtask loop.Guard the schedule call:
queueRemoteEvent(login: UserLogin, event: RemoteEvent): QueueRemoteEventResult { this.#remoteEvents.push({ event, login }); - this.#scheduleDrain(); + if (this.#context) this.#scheduleDrain(); return { event, queued: true }; }
1000-1007: Keep queued remote events until their handler succeeds.The item is removed from
#remoteEventsat line 1003 before#handleRemoteEvent()resolves. Any conversion/send failure drops that event permanently, and the scheduled drain then has nothing left to retry.Peek first and
shift()only after success, or requeue in acatch:async `#drainRemoteEvents`(): Promise<void> { if (!this.#context) return; while (this.#remoteEvents.length > 0) { - const item = this.#remoteEvents.shift(); - if (!item) continue; - await this.#handleRemoteEvent(item.login, item.event); + const item = this.#remoteEvents[0]; + if (!item) break; + await this.#handleRemoteEvent(item.login, item.event); + this.#remoteEvents.shift(); } }
501-509: Drop the previous room mapping when re-registering a portal.If the same portal key is registered with a new
mxid, the old room entry stays in#portalsByRoom. That leaves dispatch for the stale room ID pointing at the moved portal.registerPortal(portal: Portal): void { + const key = portalKeyString(portal.portalKey); + const previous = this.#portalsByKey.get(key); + if (previous?.mxid && previous.mxid !== portal.mxid) { + this.#portalsByRoom.delete(previous.mxid); + } - this.#portalsByKey.set(portalKeyString(portal.portalKey), portal); + this.#portalsByKey.set(key, portal); if (portal.mxid) { this.#portalsByRoom.set(portal.mxid, portal); }
519-521: MakeflushRemoteEvents()join the scheduled drain instead of starting a second one.Calling
#drainRemoteEvents()directly bypasses#drainPromise. If a scheduled drain is already running,flushRemoteEvents()can process the queue concurrently and break the intended ordering/serialization.async flushRemoteEvents(): Promise<void> { - await this.#drainRemoteEvents(); + if (this.#remoteEvents.length === 0) return; + this.#scheduleDrain(); + await this.#drainPromise; }
70-112: LGTM!The factory functions
createBridge,createBeeperBridge, andcreateBeeperBridgeWithClientprovide clean entry points for different bridge configurations with appropriate fallback handling for store and homeserver options.examples/dummybridge/test/smoke.ts (1)
1-157: LGTM!This smoke test provides good end-to-end coverage of the RuntimeBridge lifecycle including appservice initialization, portal creation, backfill, and message dispatch. The in-memory store and stubbed MatrixClient are well-structured for testing purposes.
packages/bridge/src/bridge.test.ts (1)
1-957: LGTM!This is an excellent, comprehensive test suite covering the RuntimeBridge lifecycle, login handling, Matrix event dispatch, remote event processing, backfill flows, and management command handling. The fake implementations are well-structured and the test cases cover important edge cases like ignoring bridge-user messages and command prefix requirements.
examples/dummybridge/src/connector.ts (1)
1-394: LGTM!This is a well-structured example connector that demonstrates the full range of bridge capabilities: multiple login flows (password, cookies, local storage, display-and-wait), command handling, message echoing, and ghost user management. It serves as good documentation for implementers of real connectors.
packages/bridge/src/index.ts (1)
1-40: LGTM!Clean module structure with appropriate re-exports. The
createBeeperBridgefactory handles store creation with sensible defaults, and thedefaultDataDirhelper provides a reasonable fallback path structure.packages/bridge/src/types.ts (1)
1-1151: LGTM!This is a comprehensive and well-organized type model for the bridge runtime. The separation of network API capabilities into specific interfaces (BackfillingNetworkAPI, MessageHandlingNetworkAPI, etc.) provides excellent type safety and documentation. The login flow types and remote event types cover the full range of bridging scenarios.
Introduce Beeper support across the bridge and native appservice: add BridgeBeeperOptions to CreateBridgeOptions, thread beeper options into RuntimeBridge, and automatically include the Beeper owner in room invites. Extend createManagementRoom/createPortalRoom to populate beeper-specific create options (auto-join, initial members, portal metadata) and add an autoJoinInvite helper. On the native side, extend MatrixAppserviceCreateRoomOptions with beeper portal types and implement applyBeeperPortalCreateDefaults to populate local room IDs, power level overrides, initial bridge state, and invites. Add unit tests for both the bridge JS and native Go logic, and update generated/runtime TypeScript types and exports to include the new portal and portal key types.
Introduce new portal/management room APIs and migrate to bridge v2 payloads across the stack. Bridge package: switch createPortalRoom to accept an info object (name/avatar/topic), update createManagementRoom/createPortalRoom callsites, refine stripUndefined typing. Pickle runtime: add MatrixAppserviceCreatePortalRoomOptions and MatrixAppserviceCreateManagementRoomOptions types, expose createPortalRoom/createManagementRoom on the client, and wire new core operations. Native core (Go): implement handlers and helper builders for appservice_create_portal_room and appservice_create_management_room, replace legacy beeper portal fields with the new bridge/portal shape, and add deterministicPortalRoomID and helper utilities. Update tests and README to reflect the new signatures and behaviors.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/bridge/src/store.ts (1)
120-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
setPortalleaves a staleportal-mxidreverse-index entry when the portal's room ID changes.If a portal is re-saved with a different
mxid, the oldportal-mxid:<old-mxid>key is never deleted, sogetPortalByMXID(oldMxid)keeps resolving the portal long after it has moved rooms.🐛 Proposed fix
async setPortal(portal: Portal): Promise<void> { const portalKey = portalStoreKey(portal); + const existing = await this.getPortal(portalKey); await this.#set(key("portal", portalKey), portal); + if (existing?.mxid && existing.mxid !== portal.mxid) { + await this.#store.delete(key("portal-mxid", existing.mxid)); + } if (portal.mxid) await this.#set(key("portal-mxid", portal.mxid), portalKey); }🤖 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/store.ts` around lines 120 - 124, setPortal currently writes the new portal and its portal-mxid reverse index but never removes a previous portal-mxid entry, leaving stale reverse mappings; to fix, read the existing portal record (e.g., via this.#get(key("portal", portalStoreKey(portal))) or this.getPortal) before writing, compare its mxid to portal.mxid, and if an old mxid exists and differs, remove the old reverse-index key (call the store delete method, e.g., this.#del(key("portal-mxid", oldMxid))) before setting the new portal-mxid entry and then proceed to write the updated portal with this.#set.
🧹 Nitpick comments (1)
packages/bridge/src/bridge.ts (1)
1107-1122: 💤 Low valueUnused variable in
#matrixIntent.Line 1111 computes
typebut it's always"m.room.message"regardless of the ternary condition. This appears to be dead code from a partially implemented feature.Suggested simplification
`#matrixIntent`(): MatrixIntent { return { client: this.#matrixClient, sendMessage: async (roomId, content) => { - const type = typeof content.msgtype === "string" ? "m.room.message" : "m.room.message"; + const type = "m.room.message"; const transactionId = `pickle-bridge-${Date.now()}-${Math.random().toString(16).slice(2)}`;🤖 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 1107 - 1122, The local variable `type` in the private method `#matrixIntent()` (inside the `sendMessage` implementation) is computed via a redundant ternary and always equals "m.room.message"; remove the dead ternary and simplify by using a single constant or inlining the literal when building the request path in `sendMessage` (keep `transactionId` and the call to `this.#matrixClient.raw.request`, and preserve `eventIdFromRaw(result.body)` and the returned object).
🤖 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/README.md`:
- Around line 7-46: The example references connector when calling
createBeeperBridge but never declares it; add a minimal stub declaration for
connector (e.g., a const connector = { /* minimal stub or TODO: implement
connector methods */ }) before calling createBeeperBridge so the snippet is
copy-paste runnable, and note in the comment that the stub should be replaced
with the real connector matching the expected type/signature used by
createBeeperBridge.
In `@packages/bridge/src/bridge.test.ts`:
- Around line 969-992: createFakeDataStore is missing the listUserLogins stub
used by bridge.start(), causing tests to throw or skip auto-loading logins; fix
by adding a listUserLogins: vi.fn(async () => []) property to
createFakeDataStore (or replace calls to createFakeDataStore with
createFakeBridgeDataStore if that already implements the full BridgeDataStore
interface) so the fake implements BridgeDataStore and bridge.start() can call
dataStore.listUserLogins() safely.
- Around line 763-791: The BridgeDataStore interface is missing
setManagementRoom which prevents `#persistManagementRoom` from actually persisting
rooms; add the signature setManagementRoom(room: ManagementRoom): Promise<void>
to the BridgeDataStore interface and implement it on MatrixBridgeDataStore
(implement a concrete async setManagementRoom that persists the ManagementRoom
to the same backing store used by other methods, e.g., delegate to the existing
persistence layer or db client used by MatrixBridgeDataStore), ensure
MatrixBridgeDataStore's type matches the new interface and update any
constructor/initialization to handle the method so hasMethod(store,
"setManagementRoom") will succeed and `#persistManagementRoom` can call it in
production.
---
Duplicate comments:
In `@packages/bridge/src/store.ts`:
- Around line 120-124: setPortal currently writes the new portal and its
portal-mxid reverse index but never removes a previous portal-mxid entry,
leaving stale reverse mappings; to fix, read the existing portal record (e.g.,
via this.#get(key("portal", portalStoreKey(portal))) or this.getPortal) before
writing, compare its mxid to portal.mxid, and if an old mxid exists and differs,
remove the old reverse-index key (call the store delete method, e.g.,
this.#del(key("portal-mxid", oldMxid))) before setting the new portal-mxid entry
and then proceed to write the updated portal with this.#set.
---
Nitpick comments:
In `@packages/bridge/src/bridge.ts`:
- Around line 1107-1122: The local variable `type` in the private method
`#matrixIntent()` (inside the `sendMessage` implementation) is computed via a
redundant ternary and always equals "m.room.message"; remove the dead ternary
and simplify by using a single constant or inlining the literal when building
the request path in `sendMessage` (keep `transactionId` and the call to
`this.#matrixClient.raw.request`, and preserve `eventIdFromRaw(result.body)` and
the returned object).
🪄 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: 937d6d2d-2b61-4d67-998a-56628d950b6d
📒 Files selected for processing (21)
examples/dummybridge/src/connector.tsexamples/dummybridge/src/index.tsexamples/dummybridge/test/smoke.tspackages/bridge/README.mdpackages/bridge/src/appservice-websocket.test.tspackages/bridge/src/appservice-websocket.tspackages/bridge/src/bridge.test.tspackages/bridge/src/bridge.tspackages/bridge/src/index.tspackages/bridge/src/store.tspackages/bridge/src/types.tspackages/pickle/native/internal/core/appservice.gopackages/pickle/native/internal/core/appservice_test.gopackages/pickle/native/internal/core/core.gopackages/pickle/native/internal/core/operations.gopackages/pickle/src/client-types.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.ts
✅ Files skipped from review due to trivial changes (1)
- packages/pickle/native/internal/core/appservice_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/pickle/src/client.ts
- packages/pickle/src/generated-runtime-operations.ts
- packages/pickle/src/index.ts
- packages/pickle/native/internal/core/core.go
- packages/pickle/src/generated-runtime-types.ts
- packages/pickle/src/runtime-types.ts
- examples/dummybridge/src/index.ts
- packages/bridge/src/types.ts
📜 Review details
🧰 Additional context used
🪛 OpenGrep (1.20.0)
packages/bridge/src/bridge.test.ts
[ERROR] 822-822: 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/bridge/src/appservice-websocket.ts
[ERROR] 249-249: 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/bridge/src/bridge.ts
[ERROR] 654-654: 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] 664-664: 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] 1222-1222: 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] 1224-1224: 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 (24)
packages/pickle/native/internal/core/appservice.go (3)
152-157:homeserverDomainis enforced as required, contradicting the optional-init contract.This validation rejects callers when
homeserverDomainis omitted, but the PR objectives mention makinghomeserverDomainoptional. Consider deriving it from the homeserver URL when not provided.
560-567: Return the actual fallback failure inensureJoined.When the fallback invite/join flow fails, the code returns the original join error (
err) instead ofbotErrorinviteErr, obscuring the actual root cause of the failure.
250-297: LGTM!The
makePortalCreateRoomRequestimplementation correctly handles bridgev2 portal room creation with deterministic room IDs, auto-join invites, bridge state events, and functional member configuration.packages/bridge/src/bridge.ts (7)
334-350: Avoid caching a login beforeconnect()succeeds.The client is added to
#networkClients(line 341) beforeconnect()is awaited (line 345). Ifconnect()rejects, laterloadUserLogin()calls will return the half-initialized client and never retry.
486-490: GuardqueueRemoteEventscheduling when the bridge is stopped.
queueRemoteEvent()unconditionally calls#scheduleDrain(), but#drainRemoteEvents()exits immediately when#contextis null. Thefinallyblock in#scheduleDrain()then reschedules while the backlog remains non-empty, potentially causing an endless microtask loop when events are queued beforestart()or afterstop().
492-500: Drop the previous room mapping when re-registering a portal.If the same portal key is registered with a new
mxid, the old room entry remains in#portalsByRoom, leaving stale dispatch routing.
510-512: MakeflushRemoteEvents()join the scheduled drain instead of starting a second one.Calling
#drainRemoteEvents()directly bypasses#drainPromise, which can cause concurrent queue processing and break ordering guarantees.
1004-1011: Keep queued remote events until their handler succeeds.The item is removed from
#remoteEventsviashift()before#handleRemoteEvent()resolves. Any conversion/send failure drops that event permanently with no retry opportunity.
724-729: Route Matrix events to the owning login instead of broadcasting.These loops fan each Matrix event out to every loaded
NetworkAPI. Once two logins are active, events for a portal registered withportalKey.receiver = login:awill also invoke handlers onlogin:b, potentially duplicating or misrouting outbound actions.
161-192: LGTM!The
start()lifecycle correctly sequences status loading, Matrix client boot, appservice initialization, context creation, persisted data hydration, connector initialization, and event subscription before marking the bridge as started.packages/bridge/src/appservice-websocket.ts (4)
200-237: Reply with anerrorframe when message handling throws.If
#dispatch()or#handleHTTPProxy()fails, this method rejects and the caller only logs it. The homeserver never receives a response for that requestid, so the transaction can hang or be retried indefinitely.
344-350: Treat websocket request id0as valid.
!message.idevaluates totruewhenidis0, causing legitimate requests with id0to never receive an ack/error response.
272-289: LGTM!The ping logic correctly guards against stale pings, terminates the socket on timeout, and uses a single in-flight ping ID to prevent overlapping ping cycles.
123-157: LGTM!The close handler correctly detects replacement closes (code 4001 or
conn_replacedstatus), prevents reconnection for replaced connections, invokes lifecycle hooks, and implements capped exponential backoff for normal reconnects.packages/bridge/src/appservice-websocket.test.ts (3)
2-2: Use a type-only import forAddressInfo.
AddressInfois only used in type casts. Usingimport typemakes the intent explicit.
130-167: LGTM!The exponential backoff test effectively validates capped reconnect delays (5→10→20→20) and reset to initial delay (5) after a stable connection period, with appropriate timing configuration for fast test execution.
194-234: LGTM!The replacement close test correctly verifies that
conn_replacedstatus prevents reconnection and triggers bothonCloseandonReplacedcallbacks with the expected event properties.packages/pickle/native/internal/core/operations.go (1)
20-35: LGTM!The new appservice operation identifiers follow the established naming conventions and align with the corresponding handler implementations in
appservice.goand TypeScript bindings.examples/dummybridge/src/connector.ts (2)
274-280: LGTM!The overloaded method signatures correctly handle both the legacy
(input)and the context-aware(ctx, input)calling conventions, enabling backward compatibility while supporting the new BridgeRequestContext pattern.
369-390: LGTM!The
remoteMessagehelper cleanly constructs bridgev2-style remote events with proper portal key binding and ghost sender resolution.packages/pickle/src/client-types.ts (1)
113-122: LGTM!The
MatrixAppserviceinterface provides a well-typed surface for appservice operations, with methods that align with the native Go handlers and generated runtime operations.examples/dummybridge/test/smoke.ts (2)
23-60: LGTM!The mock
MatrixClientprovides a clean test double that captures calls for assertion while returning appropriate response shapes, effectively isolating the bridge runtime behavior for smoke testing.
116-137: LGTM!The Matrix event dispatch and remote event flush sequence correctly validates the end-to-end message echo flow, asserting that inbound Matrix messages are routed through the connector and produce outbound appservice messages with the expected ghost sender and transformed body.
packages/bridge/src/index.ts (1)
20-36: LGTM —createBeeperBridgestore-derivation and Matrix config merging look correct.The
??-chained store selection ensures a file-backed store is only created when no explicit store is provided, and thehomeserver/tokenfallbacks toaccountfields are a sensible progressive disclosure pattern.
When creating a portal room, use top-level options as fallbacks for info fields: name and topic now fall back to options.name/options.topic, and avatarUrl falls back to options.avatarUrl. Update the BridgeCreatePortalRoomOptions type to include avatarUrl, name, and topic so callers can provide these values at the top level.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/pickle/native/internal/core/appservice.go (2)
560-567:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn the actual fallback failure in
ensureJoined.When fallback steps fail, the code currently returns the original join error instead of
botErr/inviteErr, which hides the root cause and makes debugging difficult.Suggested fix
if err != nil { bot, botErr := as.client(as.botUserID) if botErr != nil { - return err + return botErr } if _, inviteErr := bot.InviteUser(ctx, roomID, &mautrix.ReqInviteUser{UserID: cli.UserID}); inviteErr != nil { - return err + return inviteErr }🤖 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/appservice.go` around lines 560 - 567, The error handling in ensureJoined returns the original join error when the fallback bot lookup or invite fails; change the returns so that if as.client(as.botUserID) fails you return botErr, and if bot.InviteUser fails you return inviteErr, preserving the actual fallback failure (refer to as.client, bot.InviteUser, botErr, inviteErr and the ensureJoined flow).
152-157:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
homeserverDomainis enforced as required, which may break optional-init contract.The validation requires both
homeserverandhomeserverDomain, but based on past review feedback,homeserverDomainshould be derivable from the homeserver URL when not provided.Suggested fix
+import ( + "net/url" + // ... existing imports +) - if req.Homeserver == "" || req.HomeserverDomain == "" { - return nil, errors.New("homeserver and homeserverDomain are required") + if req.Homeserver == "" { + return nil, errors.New("homeserver is required") + } + if req.HomeserverDomain == "" { + hsURL, parseErr := url.Parse(req.Homeserver) + if parseErr != nil || hsURL.Hostname() == "" { + return nil, errors.New("homeserverDomain is required when homeserver cannot be parsed") + } + req.HomeserverDomain = hsURL.Hostname() }🤖 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/appservice.go` around lines 152 - 157, The current validation wrongly requires req.HomeserverDomain; instead, if req.HomeserverDomain is empty, derive it from req.Homeserver by parsing the homeserver URL and extracting the host (stripping any port) and assign it to req.HomeserverDomain before continuing validation; if URL parsing fails or req.Homeserver is empty, return the appropriate error. Keep the other checks for req.Registration.AppToken, req.Registration.SenderLocalpart and req.Registration.ID unchanged. Use the existing req variable and the validation block around Homeserver/HomeserverDomain to locate where to add the parsing and assignment.packages/bridge/src/store.ts (1)
120-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the old MXID index when a portal moves rooms.
When updating an existing portal with a different
mxid, the oldportal-mxidentry is not deleted, leaving a stale mapping that can cause incorrect portal resolution.Suggested fix
async setPortal(portal: Portal): Promise<void> { const portalKey = portalStoreKey(portal); + const existing = await this.getPortal(portalKey); await this.#set(key("portal", portalKey), portal); + if (existing?.mxid && existing.mxid !== portal.mxid) { + await this.#store.delete(key("portal-mxid", existing.mxid)); + } if (portal.mxid) await this.#set(key("portal-mxid", portal.mxid), portalKey); }🤖 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/store.ts` around lines 120 - 124, In setPortal, clear any stale portal-mxid index for a portal that changes mxid: before calling this.#set, fetch the existing portal with this.#get(key("portal", portalKey)) (where portalKey = portalStoreKey(portal)), if existingPortal?.mxid exists and is different from portal.mxid call this.#del(key("portal-mxid", existingPortal.mxid)); then continue to this.#set(key("portal", portalKey), portal) and if portal.mxid set the new mapping with this.#set(key("portal-mxid", portal.mxid), portalKey); also ensure the deletion handles the case where portal.mxid was removed (undefined) so the old index is removed.
🤖 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.
Duplicate comments:
In `@packages/bridge/src/store.ts`:
- Around line 120-124: In setPortal, clear any stale portal-mxid index for a
portal that changes mxid: before calling this.#set, fetch the existing portal
with this.#get(key("portal", portalKey)) (where portalKey =
portalStoreKey(portal)), if existingPortal?.mxid exists and is different from
portal.mxid call this.#del(key("portal-mxid", existingPortal.mxid)); then
continue to this.#set(key("portal", portalKey), portal) and if portal.mxid set
the new mapping with this.#set(key("portal-mxid", portal.mxid), portalKey); also
ensure the deletion handles the case where portal.mxid was removed (undefined)
so the old index is removed.
In `@packages/pickle/native/internal/core/appservice.go`:
- Around line 560-567: The error handling in ensureJoined returns the original
join error when the fallback bot lookup or invite fails; change the returns so
that if as.client(as.botUserID) fails you return botErr, and if bot.InviteUser
fails you return inviteErr, preserving the actual fallback failure (refer to
as.client, bot.InviteUser, botErr, inviteErr and the ensureJoined flow).
- Around line 152-157: The current validation wrongly requires
req.HomeserverDomain; instead, if req.HomeserverDomain is empty, derive it from
req.Homeserver by parsing the homeserver URL and extracting the host (stripping
any port) and assign it to req.HomeserverDomain before continuing validation; if
URL parsing fails or req.Homeserver is empty, return the appropriate error. Keep
the other checks for req.Registration.AppToken, req.Registration.SenderLocalpart
and req.Registration.ID unchanged. Use the existing req variable and the
validation block around Homeserver/HomeserverDomain to locate where to add the
parsing and assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 364de67c-b129-47b0-a22a-c81e4528d4bf
📒 Files selected for processing (21)
examples/dummybridge/src/connector.tsexamples/dummybridge/src/index.tsexamples/dummybridge/test/smoke.tspackages/bridge/README.mdpackages/bridge/src/appservice-websocket.test.tspackages/bridge/src/appservice-websocket.tspackages/bridge/src/bridge.test.tspackages/bridge/src/bridge.tspackages/bridge/src/index.tspackages/bridge/src/store.tspackages/bridge/src/types.tspackages/pickle/native/internal/core/appservice.gopackages/pickle/native/internal/core/appservice_test.gopackages/pickle/native/internal/core/core.gopackages/pickle/native/internal/core/operations.gopackages/pickle/src/client-types.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.ts
✅ Files skipped from review due to trivial changes (3)
- packages/pickle/native/internal/core/appservice_test.go
- packages/bridge/README.md
- examples/dummybridge/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/pickle/src/generated-runtime-operations.ts
- packages/pickle/src/runtime-types.ts
- packages/pickle/native/internal/core/core.go
- packages/pickle/src/generated-runtime-types.ts
📜 Review details
🧰 Additional context used
🪛 OpenGrep (1.20.0)
packages/bridge/src/appservice-websocket.ts
[ERROR] 249-249: 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/bridge/src/bridge.test.ts
[ERROR] 822-822: 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/bridge/src/bridge.ts
[ERROR] 656-656: 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] 666-666: 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] 1224-1224: 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] 1226-1226: 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 (68)
packages/bridge/src/appservice-websocket.test.ts (2)
2-2: 💤 Low valueUse a type-only import for
AddressInfo.
AddressInfois only used in type casts (lines 24, 79, 135, 174, 199). Usingimport typemakes the intent explicit.Suggested change
-import { AddressInfo } from "node:net"; +import type { AddressInfo } from "node:net";
18-234: LGTM on test coverage.Comprehensive test suite covering the key websocket behaviors: connection establishment, transaction dispatch, HTTP proxy handling, exponential backoff with reset, ping timeout reconnection, and replacement close handling. The shared cleanup infrastructure ensures proper resource management across tests.
packages/pickle/src/client.ts (1)
74-86: LGTM!The appservice namespace implementation follows the established patterns in this file correctly. Methods properly delegate to core operations, use
stripUndefinedfor optional parameters, and thesendMessageresult transformation aligns with theSentEventtype contract.packages/pickle/native/internal/core/operations.go (1)
20-35: LGTM!The new appservice operation constants follow the established naming conventions and the
ts:operationcomments are properly formatted for code generation.examples/dummybridge/test/smoke.ts (1)
1-157: LGTM!Well-structured smoke test that validates the key bridge integration points: appservice initialization, portal room creation, backfill operations, and remote event dispatch. The stub client and in-memory store implementations are appropriate for testing purposes.
packages/pickle/src/client-types.ts (1)
113-122: LGTM!The
MatrixAppserviceinterface is well-typed with appropriate parameter and return types that align with the implementation inclient.tsand the corresponding core operations.packages/pickle/src/index.ts (1)
21-38: LGTM!The new appservice type exports are comprehensive and properly sourced from
./runtime-types, making the appservice API surface available to package consumers.packages/pickle/native/internal/core/appservice.go (1)
250-297: LGTM on portal room creation logic.The
makePortalCreateRoomRequestfunction properly constructs the room creation request with appropriate bridge state events, power levels, and deterministic room ID derivation. The room type handling and bridge info content generation follow bridgev2 conventions.Also applies to: 316-318
packages/bridge/src/store.ts (1)
35-39: LGTM on deletion and key generation.The
deletePortalmethod correctly cleans up both the portal entry and its MXID index. TheportalStoreKeyfunction uses a null delimiter to ensure uniqueness when receiver is empty.Also applies to: 144-146
packages/bridge/src/bridge.test.ts (12)
763-791:setManagementRoommock is inlined but still absent fromBridgeDataStoreinterface.The test at line 768 mocks
setManagementRoomby extendingcreateFakeDataStore(). However, as noted in past reviews, this method is not part of theBridgeDataStoreinterface instore.ts. The runtime's#persistManagementRoomuseshasMethodto defensively check, so it works in tests when mocked but won't persist in production withMatrixBridgeDataStore.This is a pre-existing issue flagged in past reviews.
969-992:createFakeDataStoreis missinglistUserLoginsstub.This function lacks
listUserLogins, which is required bybridge.start()when a dataStore is present. Any test using this factory withbridge.start()would either fail or skip auto-loading logins silently.Currently, line 770 uses it for the management room persistence test but doesn't call
bridge.start()before the dispatch calls—wait, looking again at lines 772-778,bridge.start()IS called.However,
#loadPersistedUserLoginsguards withhasMethod(this.#dataStore, "listUserLogins"), so the missing method causes silent skip rather than failure.This is a pre-existing issue flagged in past reviews.
1-23: LGTM!The imports are well-organized, covering the necessary types from
@beeper/pickleand the bridge package modules for comprehensive testing.
24-106: LGTM!The lifecycle and login persistence tests thoroughly verify bridge startup, connector initialization, Matrix subscription setup, login idempotency, persisted login auto-load, and bridgev2 status payload structure.
108-196: LGTM!The login process binding and Matrix message dispatch tests correctly verify that login lifecycle methods receive the bridge request context and that messages are properly routed to network clients while ignoring bridge-user messages.
198-323: LGTM!The remote event queuing, appservice portal creation, backfill, and Beeper metadata tests comprehensively cover the portal/management room creation flows with appropriate assertions on the appservice method calls and Beeper-specific options like
autoJoinInvitesandinitialMembers.
325-498: LGTM!The backfill pagination, bridgev2 queue/checkpoint type assertions, and ghost/portal/message-request/status registration tests thoroughly verify the expected shapes and behaviors of the bridge runtime's metadata management capabilities.
500-640: LGTM!The user profile/media helper wrappers and management command dispatch tests correctly verify the underlying Matrix client calls and the command routing/reply behavior including appservice bot sender selection.
642-761: LGTM!The command prefix enforcement, implicit management room detection, and portal vs management room distinction tests correctly verify that commands are only processed when appropriate (with prefix outside management rooms, from owner for implicit management) and that persisted portals are not mistakenly treated as management rooms.
793-855: LGTM!The login lifecycle built-in command tests comprehensively cover the
login,cancel-login,list-logins, andlogoutcommands including pending login management, network disconnect on logout, and reply content verification.
858-945: LGTM!The test fixture factories (
matrixConfig,createFakeBridgeDataStore,createFakeConnector,createFakeNetworkAPI,loginStep) provide well-structured mocks with appropriate Vitest spy functions for verifying bridge behavior.
994-1054: LGTM!The
createFakeMatrixClientfactory provides a comprehensive mock of the Matrix client interface with appropriate stub implementations for all the methods exercised by the bridge runtime tests.packages/bridge/src/appservice-websocket.ts (7)
200-237: Error handling gap: exceptions from dispatch/proxy don't produce error frames.If
#dispatch()or#handleHTTPProxy()throws, the outer catch at line 111-113 logs the error but the homeserver never receives an error response for that transaction/request. This can cause the server to hang waiting or retry indefinitely.This is a pre-existing issue flagged in past reviews.
344-350:!message.idtreats websocket request id0as invalid.The check at line 345 uses
!message.idwhich evaluates totruewhenid === 0, causing legitimate requests with id 0 to not receive responses.This is a pre-existing issue flagged in past reviews.
1-70: LGTM!The interface definitions and constructor setup are well-structured. The timing options with sensible defaults (2s initial reconnect, 120s max, 180s ping interval, 30s ping timeout, 60s stable connection threshold) provide good resilience configuration.
72-121: LGTM!The
start/stoplifecycle and#connectmethod correctly manage WebSocket state. The connection setup with Bearer auth, custom headers (User-Agent, Process-ID, Websocket-Version), and event handlers (open, message, close, error) follows the expected appservice websocket protocol.
123-198: LGTM!The close handling with status detection (
conn_replaced), reconnect scheduling with exponential backoff, and ping/pong lifecycle management are correctly implemented. TheWeakSetfor#handledClosesprevents duplicate close handling for the same socket instance.
239-299: LGTM!The HTTP proxy handling correctly routes transaction PUTs and appservice query endpoints with appropriate fallback responses. The ping mechanism with timeout watchdog and the public
sendmethod are well-implemented.
353-463: LGTM!The helper functions for HTTP response formatting, URL construction, close status parsing, Matrix event normalization, and utility operations are all correctly implemented. The
rawMatrixEventfunction properly maps different event types to their correspondingMatrixClientEventvariants.packages/bridge/src/bridge.ts (24)
336-351: Client cached beforeconnect()succeeds.Line 343 sets
this.#networkClients.set(login.id, client)before awaitingclient.connect()at line 347. Ifconnect()rejects, the half-initialized client remains cached, and subsequentloadUserLogin()calls return it without retry.This is a pre-existing issue flagged in past reviews.
488-492:queueRemoteEventalways schedules drain even when bridge is stopped.Line 490 calls
#scheduleDrain()unconditionally. When#contextis null (bridge stopped),#drainRemoteEventsexits immediately but thefinallyblock reschedules if backlog exists, potentially spinning.This is a pre-existing issue flagged in past reviews.
494-498:registerPortaldoesn't clean up stale room mapping.If a portal is re-registered with a different
mxid, the old room entry remains in#portalsByRoom. This leaves the stale room ID pointing at the moved portal.This is a pre-existing issue flagged in past reviews.
512-513:flushRemoteEvents()bypasses the scheduled drain.Calling
#drainRemoteEvents()directly can process the queue concurrently with an in-flight#drainPromise, breaking serialization.This is a pre-existing issue flagged in past reviews.
726-731: Matrix events broadcast to all logins instead of routing to owner.The loop iterates over all
#networkClientsvalues and invokes handlers on each. When multiple logins are active, events for a room registered withportalKey.receiver = login:awill also invoke handlers onlogin:b, potentially causing duplicates or misrouting.This is a pre-existing issue flagged in past reviews.
999-1003: Remote events removed from queue before handler succeeds.Line 1009 uses
shift()to remove the item before#handleRemoteEventcompletes. If the handler fails, the event is lost with no retry.This is a pre-existing issue flagged in past reviews.
1-67: LGTM!The imports are comprehensive, covering the Matrix client types from
@beeper/pickle, the appservice websocket, Beeper helpers, and the extensive type definitions for bridge runtime.
70-117: LGTM!The
createBridge,createBeeperBridge, andcreateBeeperBridgeWithClientfactory functions correctly wire up the bridge options with appropriate store validation and Beeper-specific appservice initialization.
119-159: LGTM!The
RuntimeBridgeclass structure with private fields for state management (network clients, portals, ghosts, logins, remote events queue, etc.) and the public accessors forclientandcontextare well-organized.
161-214: LGTM!The
startandstoplifecycle methods correctly sequence: status persistence loading, Matrix client boot, appservice initialization, connector init/start, portal/login hydration, Matrix event subscription, and websocket startup. The stop sequence properly disconnects clients, stops subscriptions, and cleans up state.
216-270: LGTM!The
createLogin,createManagementRoom, andcreatePortalRoommethods correctly delegate to the connector and appservice with appropriate Beeper-specific options (auto-join invites, initial members).
272-334: LGTM!The
backfill,backfillMessages, andqueueBackfillmethods correctly handle backfill fetching through the network API, message conversion, and batch sending with appropriate login state updates and progress tracking.
354-440: LGTM!The bridge state/status management, ghost registration, portal accessors, message request handling, and identifier resolution are correctly implemented with appropriate data store persistence.
450-486: LGTM!The user profile helpers, media upload/download, and send methods correctly delegate to the underlying Matrix client APIs.
504-518: LGTM!The management room registration, remote event flush, and backlog accessor are correctly implemented.
520-561: LGTM!The
dispatchMatrixEvententry point and#requestContext/#createContexthelpers correctly route events by kind and provide the bridge context for downstream handlers.
563-640: LGTM!The persistence loading methods (
#loadPersistedStatus,#loadPersistedUserLogins,#loadPersistedPortals), login state management, Matrix event subscription, and appservice websocket startup are correctly implemented with appropriate error handling and logging.
643-696: LGTM!The HTTP provisioning proxy handling for capabilities, login flows, login list, and login start/step endpoints correctly implements the provisioning API with appropriate error responses and login process state management.
698-760: LGTM!The Matrix message dispatch correctly handles own-user filtering, command parsing, portal lookup, and network client routing with checkpoint reporting.
762-895: LGTM!The management command parsing, built-in command handling (help, list-logins, login, logout, cancel-login, set-management-room), and implicit management room detection are well-implemented with appropriate user feedback.
917-993: LGTM!The Matrix reaction, redaction, and typing dispatch methods correctly handle own-user filtering and route events to network clients with the handler capability check.
1006-1107: LGTM!The remote event handling for messages, backfill, chat info changes, and chat deletes correctly converts events and dispatches to the Matrix room with appropriate portal lookup.
1109-1194: LGTM!The Matrix intent helper, remote message sending (with appservice vs direct path selection), command reply sending, and bridge status/checkpoint dispatch are correctly implemented.
1197-1540: LGTM!The helper functions for logging, type checking, appservice bot user ID derivation, ghost user ID generation, bridge state mapping, error formatting, provisioning capabilities, login binding, portal key stringification, and various utility operations are all correctly implemented.
examples/dummybridge/src/connector.ts (4)
1-27: LGTM!The imports, constants, and module structure are well-organized. The exports provide clear identifiers for testing purposes.
29-189: LGTM!The
DummyConnectorimplementation is clean and follows theCommandHandlingBridgeConnectorinterface correctly. The command handling covers various test scenarios (create-room, message, messages, ghost, kick-me, etc.) and the#remoteMessagehelper properly wraps the sharedremoteMessagefactory.
195-312: LGTM!The
DummyLoginProcesscorrectly implements multiple login flow interfaces (LoginProcess,LoginProcessUserInput,LoginProcessCookies,LoginProcessDisplayAndWait). The cancellation handling via#throwIfCancelled()is properly checked at each step, and the overloaded method signatures handle both context-based and direct invocation patterns.
319-394: LGTM!The
DummyNetworkAPIimplementation correctly echoes Matrix messages back as remote events for testing. ThefetchMessagesmethod provides predictable historical data, and the helper functions (remoteMessage,stringBody) are appropriately scoped.packages/bridge/src/index.ts (2)
16-18: LGTM!The
createBridgefactory is a straightforward wrapper that creates aRuntimeBridgewith the configured Matrix client.
20-40: LGTM!The
createBeeperBridgefactory correctly handles store selection with appropriate fallback chain, creates a data store when not provided, and properly merges Matrix configuration with account credentials. ThedefaultDataDirhelper provides sensible defaults for file-backed storage.packages/bridge/src/types.ts (10)
1-70: LGTM!The imports from
@beeper/pickleand./store, along with the ID type aliases and foundational interfaces (PortalKey,BridgeName,BridgeInfoContent,BridgeConfigPart,DBMetaTypes), provide a clean type foundation.
71-130: LGTM!The
BridgeConnectorinterface and its extensions (CommandHandlingBridgeConnector,StoppableNetwork,DirectMediableNetwork, etc.) provide a well-structured hierarchy for different connector capabilities.
126-249: LGTM!The
NetworkAPIinterface and its numerous extensions for specific capabilities (backfilling, message handling, reactions, typing, polls, etc.) follow a clean composition pattern that allows connectors to implement only the features they support.
251-355: LGTM!The login process interfaces (
LoginProcess,LoginProcessWithOverride,LoginProcessDisplayAndWait,LoginProcessUserInput,LoginProcessCookies) and related types (LoginFlow,LoginStep, cookie/input params) are comprehensive and support the various login flow patterns.
357-494: LGTM!The
RemoteEventinterface hierarchy with its variants for context mutation, pre/post handlers, chat changes, backfill, deletion, and message targeting provides good extensibility for different remote event types.
496-604: LGTM!The
PickleBridgeinterface defines a comprehensive public API for bridge consumers. The configuration types (CreateBridgeOptions,CreateBeeperBridgeOptions,BridgeMatrixConfig,NodeBridgeMatrixConfig) and context types provide clear contracts.
606-768: LGTM!The logger interface, remote event result types, room creation options, management room/command types, dispatch result, capability interfaces, and entity types (
UserLogin,Portal,Ghost) are well-defined.
778-930: LGTM!The bridge state types (
BridgeState,BridgeStateEvent,BridgeStatePayload,BridgeStatus), message request, identifier resolution, profile, media, and message types are comprehensive and cover the bridgev2 requirements.
931-1030: LGTM!The Matrix-facing payload types for messages, reactions, edits, redactions, read receipts, typing, polls, disappearing timer, membership, room metadata, mute, tags, and chat deletion are well-structured.
1032-1158: LGTM!The message checkpoint types (
MessageCheckpoint,MessageCheckpoints), chat info types, and backfill/fetch types (FetchMessagesParams,FetchMessagesResponse,BackfillQueueTask,BackfillQueueParams,BackfillQueueResult) are comprehensive and match the bridgev2 specification.
Update @types/node to ^20.0.0 across packages and adjust code and tests to improve bridge runtime stability and behavior. Notable changes: - Bridge: add try/catch and improved handling in AppserviceWebsocket, guard messageResponse for missing ids, deduplicate network client loads, schedule/drain remote events with a drainPromise, add portal-specific network client filtering, handle portal registration mxid cleanup, and simplify sendMessage type handling. - Store: add management room support and ensure portal mxid mapping is cleaned when changed. - Tests: add missing mocked store methods and update tests to reflect new behavior. - Examples: guard against empty keys when loading env, and respect BEEPER_BASE_DOMAIN in dummy bridge example. - Pickle (TS/Go): make homeserverDomain optional in types and Go init, derive homeserverDomain from URL when omitted, and improve error returns; clarify loginWithPassword error messages. - Documentation/license: add full MPL2 LICENSE and a connector stub to the bridge README. These changes fix race conditions around user login loading and remote event draining, add some missing persistence APIs, and improve robustness of websocket/error handling.
Update packages/state-sqlite devDependency @types/node from ^20.0.0 to ^22.5.0 and regenerate pnpm-lock.yaml. The lockfile now includes @types/node@22.19.17 and updates related vitest/vite snapshots and optional dependency references to use the newer typings. No runtime code changes.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 887-899: The logout handler (`#handleLogoutCommand`) removes the
client and persisted login but fails to clear in-memory login state and the
bridge status entry; update the function to also remove any entry for the
loginId from `#loginStates` and delete or update the persisted
`#bridgeStatus.logins` so subsequent `#sendCurrentBridgeStatus`() won't report it as
connected, ensuring you call the existing `#deleteStoredUserLogin`(loginId) (or an
analogous persistence cleanup method) and then remove from the `#loginStates` map
before sending the current bridge status.
- Around line 1240-1250: The helpers appserviceBotUserId and
ghostUserIdFromRegistration currently interpolate options.homeserverDomain
directly which can be undefined; change them to compute a domain variable first:
use options.homeserverDomain if present, otherwise parse options.homeserver
(e.g., new URL(options.homeserver).hostname) to derive the domain, and use that
domain for the MXID construction in both functions; ensure you handle a missing
options.homeserver with a clear error or fallback so you never produce
`:undefined` in the returned user IDs.
- Around line 964-973: The current MatrixRedaction message sets eventId to the
redaction event's ID (event.eventId) but should set it to the redacted/target
event ID found on the incoming event object (event.raw.redacts); update the
construction of the msg in bridge.ts to use event.raw.redacts (guarding for
existence) before iterating clients via `#networkClientsForPortal` and calling
client.handleMatrixRedaction(this.#requestContext(), msg) so handlers receive
the target event ID rather than the redaction event ID.
- Around line 352-361: The code stores and connects using the incoming UserLogin
as-is, which can be a partial { id } and overwrite canonical fields; before
calling connector.loadUserLogin / client.connect and persisting, resolve and
merge the canonical login record: if this.#userLogins has an entry for login.id
or this.#dataStore exposes a getter (e.g., getUserLogin), load that full record
and shallow-merge its userId, remoteName and connector-specific metadata into
the incoming login object, then use the mergedLogin for client.connect,
this.#userLogins.set and this.#dataStore.setUserLogin so you don't discard
existing fields; keep references to methods shown (`#loadUserLogin`,
`#setLoginBridgeState`, connector.loadUserLogin, client.connect,
this.#userLogins.set, this.#dataStore.setUserLogin).
- Around line 1020-1025: The current `#scheduleDrain` sets `#drainPromise` to
this.#drainRemoteEvents().finally(...) which always reschedules immediately on
failure, causing a hot loop because failed items remain at `#remoteEvents`[0];
change the logic so you only reschedule when `#drainRemoteEvents` resolves
successfully. Replace the finally block with a then/catch (or await try/catch)
pattern: on success clear `#drainPromise` and if this.#context &&
this.#remoteEvents.length > 0 call `#scheduleDrain`(); on failure clear
`#drainPromise` but do not immediately reschedule (optionally set a retry/backoff
if desired). Make changes in `#scheduleDrain`, referencing `#drainPromise`,
`#drainRemoteEvents`, and `#remoteEvents`.
In `@packages/pickle/native/internal/core/appservice.go`:
- Around line 325-326: The deterministicPortalRoomID construction in
deterministicPortalRoomID currently concatenates portalKey.ID and
portalKey.Receiver verbatim, causing collisions and invalid RoomIDs; update
deterministicPortalRoomID to first encode each PortalKey field (e.g., use
URL-safe base64 or percent-encoding) so delimiters like '.' and ':' cannot
collide or produce illegal characters, then compose the RoomID from the encoded
id and receiver along with as.homeserverDomain; ensure whatever encoder you pick
is reversible/consistent and does not emit ':' so existing RoomID formatting
remains valid and unique for MatrixAppservicePortalKey.
In `@packages/pickle/src/auth.ts`:
- Around line 119-123: In authOptions, the homeserver field currently allows an
empty string to pass through and later produce an invalid URL; normalize blank
or whitespace-only homeserver inputs by treating them as unset (e.g., trim and
check length === 0) before applying the fallback logic so the computed
homeserver uses process.env or the default; update the homeserver assignment in
authOptions to check for a falsy/empty trimmed homeserver from
PasswordAuthOptions and only then fall back to process.env.MATRIX_HOMESERVER or
the generated default, preserving the rest of MatrixAuthOptions (including
optional fetch).
🪄 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: e9879950-b3e0-4817-bce7-c48447f3382f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
examples/dummybridge/package.jsonexamples/dummybridge/src/env.tsexamples/dummybridge/src/index.tsexamples/dummybridge/test/smoke.tspackage.jsonpackages/ai-sdk/package.jsonpackages/bridge/LICENSEpackages/bridge/README.mdpackages/bridge/docs/BRIDGE_TODO.mdpackages/bridge/package.jsonpackages/bridge/src/appservice-websocket.test.tspackages/bridge/src/appservice-websocket.tspackages/bridge/src/beeper.tspackages/bridge/src/bridge.test.tspackages/bridge/src/bridge.tspackages/bridge/src/events.tspackages/bridge/src/store.tspackages/chat-adapter/package.jsonpackages/cloudflare/package.jsonpackages/pickle/native/internal/core/appservice.gopackages/pickle/package.jsonpackages/pickle/src/auth.tspackages/pickle/src/generated-runtime-types.tspackages/state-file/package.jsonpackages/state-indexeddb/package.jsonpackages/state-memory/package.jsonpackages/state-simple/package.jsonpackages/state-sqlite/package.json
✅ Files skipped from review due to trivial changes (18)
- packages/cloudflare/package.json
- packages/state-memory/package.json
- packages/state-file/package.json
- packages/pickle/package.json
- packages/state-simple/package.json
- packages/ai-sdk/package.json
- packages/chat-adapter/package.json
- packages/state-sqlite/package.json
- packages/state-indexeddb/package.json
- packages/bridge/LICENSE
- examples/dummybridge/package.json
- packages/bridge/src/events.ts
- examples/dummybridge/src/env.ts
- examples/dummybridge/test/smoke.ts
- packages/pickle/src/generated-runtime-types.ts
- examples/dummybridge/src/index.ts
- packages/bridge/src/beeper.ts
- packages/bridge/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bridge/src/store.ts
📜 Review details
🧰 Additional context used
🪛 LanguageTool
packages/bridge/docs/BRIDGE_TODO.md
[grammar] ~17-~17: Ensure spelling is correct
Context: ... Appservice portal room creation. - [x] Appservice batch backfill. - [x] Basic live Matrix...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~50-~50: This phrase is redundant (‘I’ stands for ‘Interfaces’). Use simply “APIInterfaces”.
Context: ...ta registration/migrations. ## Network API Interfaces - [x] NetworkAPI. - [x] `PushableNet...
(ACRONYM_TAUTOLOGY)
🪛 OpenGrep (1.20.0)
packages/bridge/src/bridge.ts
[ERROR] 676-676: 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] 686-686: 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] 1254-1254: 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] 1256-1256: 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/bridge/src/appservice-websocket.ts
[ERROR] 255-255: 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/bridge/src/bridge.test.ts
[ERROR] 822-822: 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 (4)
packages/pickle/src/auth.ts (2)
19-24:PasswordAuthOptionsshape looks clean and consistent.Good use of
Omit<MatrixAuthOptions, "homeserver">plus optional overrides for ergonomic auth input.
42-52:loginWithPasswordfallback and validation flow is solid.The credential resolution order and explicit missing-input errors are clear and practical.
packages/pickle/native/internal/core/appservice.go (2)
159-176: Nice compatibility fix for optionalhomeserverDomain.Deriving the domain from
homeserverkeeps the native init path aligned with the TS contract and still fails fast on malformed URLs.
568-585: GoodensureJoinedfallback cleanup.Returning the actual fallback error and updating the membership cache after a successful join makes this path much easier to debug.
| async #loadUserLogin(login: UserLogin): Promise<NetworkAPI> { | ||
| await this.#setLoginBridgeState(login, "CONNECTING"); | ||
| const client = await this.connector.loadUserLogin(this.#requestContext(), login); | ||
| await client.connect({ ...this.#requestContext(), login }); | ||
| login.client = client; | ||
| this.#userLogins.set(login.id, login); | ||
| this.#networkClients.set(login.id, client); | ||
| if (this.#dataStore && hasMethod(this.#dataStore, "setUserLogin")) { | ||
| await this.#dataStore.setUserLogin(login); | ||
| } |
There was a problem hiding this comment.
Resolve the canonical UserLogin before persisting it.
This path can be reached with only { id } (for example after a login flow completes), and then it writes that partial object back into #userLogins/dataStore and passes it into client.connect(). That can silently discard userId, remoteName, and connector-specific metadata from an existing login record.
🛠️ Suggested fix
async `#loadUserLogin`(login: UserLogin): Promise<NetworkAPI> {
- await this.#setLoginBridgeState(login, "CONNECTING");
- const client = await this.connector.loadUserLogin(this.#requestContext(), login);
- await client.connect({ ...this.#requestContext(), login });
- login.client = client;
- this.#userLogins.set(login.id, login);
- this.#networkClients.set(login.id, client);
+ const storedLogin = this.#dataStore && hasMethod(this.#dataStore, "getUserLogin")
+ ? await this.#dataStore.getUserLogin(login.id)
+ : null;
+ const resolvedLogin = storedLogin ? { ...storedLogin, ...login } : login;
+ await this.#setLoginBridgeState(resolvedLogin, "CONNECTING");
+ const client = await this.connector.loadUserLogin(this.#requestContext(), resolvedLogin);
+ await client.connect({ ...this.#requestContext(), login: resolvedLogin });
+ resolvedLogin.client = client;
+ this.#userLogins.set(resolvedLogin.id, resolvedLogin);
+ this.#networkClients.set(resolvedLogin.id, client);
if (this.#dataStore && hasMethod(this.#dataStore, "setUserLogin")) {
- await this.#dataStore.setUserLogin(login);
+ await this.#dataStore.setUserLogin(resolvedLogin);
}🤖 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 352 - 361, The code stores and
connects using the incoming UserLogin as-is, which can be a partial { id } and
overwrite canonical fields; before calling connector.loadUserLogin /
client.connect and persisting, resolve and merge the canonical login record: if
this.#userLogins has an entry for login.id or this.#dataStore exposes a getter
(e.g., getUserLogin), load that full record and shallow-merge its userId,
remoteName and connector-specific metadata into the incoming login object, then
use the mergedLogin for client.connect, this.#userLogins.set and
this.#dataStore.setUserLogin so you don't discard existing fields; keep
references to methods shown (`#loadUserLogin`, `#setLoginBridgeState`,
connector.loadUserLogin, client.connect, this.#userLogins.set,
this.#dataStore.setUserLogin).
| async #handleLogoutCommand(command: MatrixCommand): Promise<MatrixCommandResponse> { | ||
| const loginId = command.args[0]; | ||
| if (!loginId) return { handled: true, text: "Usage: logout <login-id>" }; | ||
| const client = this.#networkClients.get(loginId); | ||
| const login = this.#userLogins.get(loginId); | ||
| if (!client && !login) return { handled: true, text: `Login not found: ${loginId}` }; | ||
| if (client) await client.disconnect(); | ||
| this.#networkClients.delete(loginId); | ||
| this.#userLogins.delete(loginId); | ||
| await this.#deleteStoredUserLogin(loginId); | ||
| this.#sendCurrentBridgeStatus(); | ||
| return { handled: true, text: `Logged out: ${loginId}` }; | ||
| } |
There was a problem hiding this comment.
Clear the login state when logging out.
This removes the session from #networkClients/#userLogins, but #loginStates and the persisted #bridgeStatus.logins entry are left behind. Subsequent bridge_status updates can still report the just-logged-out login as connected.
🤖 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 887 - 899, The logout handler
(`#handleLogoutCommand`) removes the client and persisted login but fails to clear
in-memory login state and the bridge status entry; update the function to also
remove any entry for the loginId from `#loginStates` and delete or update the
persisted `#bridgeStatus.logins` so subsequent `#sendCurrentBridgeStatus`() won't
report it as connected, ensuring you call the existing
`#deleteStoredUserLogin`(loginId) (or an analogous persistence cleanup method) and
then remove from the `#loginStates` map before sending the current bridge status.
| const msg: MatrixRedaction = { | ||
| eventId: event.eventId, | ||
| portal: this.#portalForRoom(roomId), | ||
| }; | ||
| let handlers = 0; | ||
| for (const client of this.#networkClientsForPortal(msg.portal)) { | ||
| if (!hasMethod(client, "handleMatrixRedaction")) continue; | ||
| handlers += 1; | ||
| await client.handleMatrixRedaction(this.#requestContext(), msg); | ||
| } |
There was a problem hiding this comment.
Use the redacted event ID here, not the redaction event ID.
For Matrix redactions, event.eventId is the ID of the redaction event itself. The message being removed is in event.raw.redacts; forwarding the current value leaves handleMatrixRedaction() without the target it needs.
🛠️ Suggested fix
async `#dispatchMatrixRedaction`(event: GenericMatrixEvent): Promise<MatrixDispatchResult> {
const roomId = event.roomId;
if (!roomId || !event.eventId) {
return roomId
? { dispatched: false, handlers: 0, kind: event.kind, roomId }
: { dispatched: false, handlers: 0, kind: event.kind };
}
+ const redacts =
+ event.raw && typeof event.raw === "object" && typeof (event.raw as { redacts?: unknown }).redacts === "string"
+ ? (event.raw as { redacts: string }).redacts
+ : null;
+ if (!redacts) {
+ return { dispatched: false, eventId: event.eventId, handlers: 0, kind: event.kind, roomId };
+ }
const msg: MatrixRedaction = {
- eventId: event.eventId,
+ eventId: redacts,
portal: this.#portalForRoom(roomId),
};🤖 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 964 - 973, The current
MatrixRedaction message sets eventId to the redaction event's ID (event.eventId)
but should set it to the redacted/target event ID found on the incoming event
object (event.raw.redacts); update the construction of the msg in bridge.ts to
use event.raw.redacts (guarding for existence) before iterating clients via
`#networkClientsForPortal` and calling
client.handleMatrixRedaction(this.#requestContext(), msg) so handlers receive
the target event ID rather than the redaction event ID.
| #scheduleDrain(): void { | ||
| if (!this.#context) return; | ||
| this.#drainPromise ??= this.#drainRemoteEvents().finally(() => { | ||
| this.#drainPromise = null; | ||
| if (this.#context && this.#remoteEvents.length > 0) this.#scheduleDrain(); | ||
| }); |
There was a problem hiding this comment.
Don't immediately reschedule a failed remote-event drain.
Because failed items stay at #remoteEvents[0], this finally will requeue the same drain again right away. One permanently failing remote event will spin a hot retry loop until shutdown.
🛠️ Suggested fix
`#scheduleDrain`(): void {
if (!this.#context) return;
- this.#drainPromise ??= this.#drainRemoteEvents().finally(() => {
- this.#drainPromise = null;
- if (this.#context && this.#remoteEvents.length > 0) this.#scheduleDrain();
- });
+ let completed = false;
+ this.#drainPromise ??= this.#drainRemoteEvents()
+ .then(() => {
+ completed = true;
+ })
+ .finally(() => {
+ this.#drainPromise = null;
+ if (completed && this.#context && this.#remoteEvents.length > 0) {
+ this.#scheduleDrain();
+ }
+ });
}🤖 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 1020 - 1025, The current
`#scheduleDrain` sets `#drainPromise` to this.#drainRemoteEvents().finally(...)
which always reschedules immediately on failure, causing a hot loop because
failed items remain at `#remoteEvents`[0]; change the logic so you only reschedule
when `#drainRemoteEvents` resolves successfully. Replace the finally block with a
then/catch (or await try/catch) pattern: on success clear `#drainPromise` and if
this.#context && this.#remoteEvents.length > 0 call `#scheduleDrain`(); on failure
clear `#drainPromise` but do not immediately reschedule (optionally set a
retry/backoff if desired). Make changes in `#scheduleDrain`, referencing
`#drainPromise`, `#drainRemoteEvents`, and `#remoteEvents`.
| function appserviceBotUserId(options: MatrixAppserviceInitOptions): string { | ||
| return `@${options.registration.senderLocalpart}:${options.homeserverDomain}`; | ||
| } | ||
|
|
||
| function ghostUserIdFromRegistration(options: MatrixAppserviceInitOptions, escapedLocalId: string): string { | ||
| const botUserId = appserviceBotUserId(options); | ||
| for (const namespace of options.registration.namespaces.users ?? []) { | ||
| const userId = userIdFromNamespaceRegex(namespace.regex, escapedLocalId); | ||
| if (userId && userId !== botUserId) return userId; | ||
| } | ||
| return `@${options.registration.senderLocalpart}_${escapedLocalId}:${options.homeserverDomain}`; |
There was a problem hiding this comment.
Fallback to the homeserver URL when homeserverDomain is omitted.
homeserverDomain is optional now, but these helpers still interpolate it directly. Callers that rely on the new optional flow will produce bot/ghost MXIDs like @testbot:undefined even though the domain is derivable from options.homeserver.
🛠️ Suggested fix
+function appserviceDomain(options: MatrixAppserviceInitOptions): string {
+ return options.homeserverDomain ?? new URL(options.homeserver).hostname;
+}
+
function appserviceBotUserId(options: MatrixAppserviceInitOptions): string {
- return `@${options.registration.senderLocalpart}:${options.homeserverDomain}`;
+ return `@${options.registration.senderLocalpart}:${appserviceDomain(options)}`;
}
function ghostUserIdFromRegistration(options: MatrixAppserviceInitOptions, escapedLocalId: string): string {
const botUserId = appserviceBotUserId(options);
@@
- return `@${options.registration.senderLocalpart}_${escapedLocalId}:${options.homeserverDomain}`;
+ return `@${options.registration.senderLocalpart}_${escapedLocalId}:${appserviceDomain(options)}`;
}🤖 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 1240 - 1250, The helpers
appserviceBotUserId and ghostUserIdFromRegistration currently interpolate
options.homeserverDomain directly which can be undefined; change them to compute
a domain variable first: use options.homeserverDomain if present, otherwise
parse options.homeserver (e.g., new URL(options.homeserver).hostname) to derive
the domain, and use that domain for the MXID construction in both functions;
ensure you handle a missing options.homeserver with a clear error or fallback so
you never produce `:undefined` in the returned user IDs.
| func (as *matrixAppservice) deterministicPortalRoomID(portalKey MatrixAppservicePortalKey) id.RoomID { | ||
| return id.RoomID(fmt.Sprintf("!%s.%s:%s", portalKey.ID, portalKey.Receiver, as.homeserverDomain)) |
There was a problem hiding this comment.
Encode PortalKey parts before composing the deterministic room ID.
This concatenation is ambiguous for arbitrary external IDs. For example, {"id":"foo.bar","receiver":"baz"} and {"id":"foo","receiver":"bar.baz"} both collapse to the same value, and : inside either field produces a malformed id.RoomID. Because this value is then reused as both BeeperLocalRoomID and MeowRoomID, two distinct portals can end up sharing the same local-room identity.
Proposed fix
import (
"context"
+ "encoding/base64"
"encoding/json"
"errors"
"fmt"
"net/http"
"net/url"
@@
func (as *matrixAppservice) deterministicPortalRoomID(portalKey MatrixAppservicePortalKey) id.RoomID {
- return id.RoomID(fmt.Sprintf("!%s.%s:%s", portalKey.ID, portalKey.Receiver, as.homeserverDomain))
+ encodedID := base64.RawURLEncoding.EncodeToString([]byte(portalKey.ID))
+ encodedReceiver := base64.RawURLEncoding.EncodeToString([]byte(portalKey.Receiver))
+ return id.RoomID(fmt.Sprintf("!%s.%s:%s", encodedID, encodedReceiver, as.homeserverDomain))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (as *matrixAppservice) deterministicPortalRoomID(portalKey MatrixAppservicePortalKey) id.RoomID { | |
| return id.RoomID(fmt.Sprintf("!%s.%s:%s", portalKey.ID, portalKey.Receiver, as.homeserverDomain)) | |
| import ( | |
| "context" | |
| "encoding/base64" | |
| "encoding/json" | |
| "errors" | |
| "fmt" | |
| "net/http" | |
| "net/url" | |
| ... | |
| ) | |
| func (as *matrixAppservice) deterministicPortalRoomID(portalKey MatrixAppservicePortalKey) id.RoomID { | |
| encodedID := base64.RawURLEncoding.EncodeToString([]byte(portalKey.ID)) | |
| encodedReceiver := base64.RawURLEncoding.EncodeToString([]byte(portalKey.Receiver)) | |
| return id.RoomID(fmt.Sprintf("!%s.%s:%s", encodedID, encodedReceiver, as.homeserverDomain)) | |
| } |
🤖 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/appservice.go` around lines 325 - 326,
The deterministicPortalRoomID construction in deterministicPortalRoomID
currently concatenates portalKey.ID and portalKey.Receiver verbatim, causing
collisions and invalid RoomIDs; update deterministicPortalRoomID to first encode
each PortalKey field (e.g., use URL-safe base64 or percent-encoding) so
delimiters like '.' and ':' cannot collide or produce illegal characters, then
compose the RoomID from the encoded id and receiver along with
as.homeserverDomain; ensure whatever encoder you pick is reversible/consistent
and does not emit ':' so existing RoomID formatting remains valid and unique for
MatrixAppservicePortalKey.
| function authOptions(options: PasswordAuthOptions): MatrixAuthOptions { | ||
| const output: MatrixAuthOptions = { | ||
| homeserver: options.homeserver ?? process.env.MATRIX_HOMESERVER ?? `https://matrix.${options.baseDomain ?? process.env.BEEPER_BASE_DOMAIN ?? "beeper.com"}`, | ||
| }; | ||
| if (options.fetch !== undefined) output.fetch = options.fetch; |
There was a problem hiding this comment.
Handle empty-string homeserver inputs as “unset”.
Right now, a blank homeserver value can pass through and fail later with a non-actionable URL error. Normalize blank strings before fallback selection.
Suggested hardening
function authOptions(options: PasswordAuthOptions): MatrixAuthOptions {
+ const nonEmpty = (value: string | undefined): string | undefined => {
+ const trimmed = value?.trim();
+ return trimmed ? trimmed : undefined;
+ };
+
+ const baseDomain = nonEmpty(options.baseDomain) ?? nonEmpty(process.env.BEEPER_BASE_DOMAIN) ?? "beeper.com";
const output: MatrixAuthOptions = {
- homeserver: options.homeserver ?? process.env.MATRIX_HOMESERVER ?? `https://matrix.${options.baseDomain ?? process.env.BEEPER_BASE_DOMAIN ?? "beeper.com"}`,
+ homeserver: nonEmpty(options.homeserver) ?? nonEmpty(process.env.MATRIX_HOMESERVER) ?? `https://matrix.${baseDomain}`,
};
if (options.fetch !== undefined) output.fetch = options.fetch;🤖 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/src/auth.ts` around lines 119 - 123, In authOptions, the
homeserver field currently allows an empty string to pass through and later
produce an invalid URL; normalize blank or whitespace-only homeserver inputs by
treating them as unset (e.g., trim and check length === 0) before applying the
fallback logic so the computed homeserver uses process.env or the default;
update the homeserver assignment in authOptions to check for a falsy/empty
trimmed homeserver from PasswordAuthOptions and only then fall back to
process.env.MATRIX_HOMESERVER or the generated default, preserving the rest of
MatrixAuthOptions (including optional fetch).
Introduce a higher-level API for queuing remote events and portal handling: add RuntimeBridge.createPortal and backfillPortal, plus queue(login).message/event helpers and queueMessage/queueEvent methods. Add convertedMessageFromOptions and textMessage helpers to convert simple message options into ConvertedMessage parts. Expand types with PortalReference, BridgeCreatePortalOptions, RemoteEventQueue, BridgeRemoteEventOptions and BridgeRemoteMessageOptions to accept portal IDs, keys or Portal objects and simplify sender handling. Update runtime internals to resolve portal/sender references, expose queue helpers in BridgeContext, and wire new methods through the runtime. Update examples (dummybridge) and smoke test to use the new APIs, and update README usage accordingly.
Add explicit backfill support to the bridge API and consolidate remote message creation. Introduce RemoteEventQueue.backfill and new types BridgeRemoteBackfillOptions / BridgeRemoteBackfillMessageOptions. Refactor RuntimeBridge to use private helpers (#queueEvent, #queueMessage, #queueBackfillEvent and #remoteMessageEvent) and import RemoteMessageWithTransactionID; queueMessage/queueEvent were converted to private implementations and removed from the public PickleBridge/BridgeContext surface. These changes centralize message construction and enable batching backfill payloads when queuing remote events.
Refactor #queueBackfillEvent to construct a RemoteBackfill object and pass it to queueRemoteEvent. The backfill payload is now wrapped with stripUndefined to remove undefined properties before being resolved, and message portal values fall back to options.portal. This cleans up the queued backfill payload and makes the code clearer by using an explicit event object.
There was a problem hiding this comment.
♻️ Duplicate comments (5)
packages/bridge/src/bridge.ts (5)
1058-1060:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the redacted target event ID for redaction dispatch
At Line 1058-1060,
MatrixRedaction.eventIdis set toevent.eventId(redaction event). Handlers typically need the redacted target (event.raw.redacts).Suggested fix
async `#dispatchMatrixRedaction`(event: GenericMatrixEvent): Promise<MatrixDispatchResult> { @@ + const redacts = + event.raw && typeof event.raw === "object" && typeof (event.raw as { redacts?: unknown }).redacts === "string" + ? (event.raw as { redacts: string }).redacts + : null; + if (!redacts) { + return { dispatched: false, eventId: event.eventId, handlers: 0, kind: event.kind, roomId }; + } const msg: MatrixRedaction = { - eventId: event.eventId, + eventId: redacts, portal: this.#portalForRoom(roomId), };🤖 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 1058 - 1060, The redaction dispatch is currently using the redaction event's own ID (event.eventId) instead of the ID of the event being redacted; update the MatrixRedaction construction (the msg object) to use the redacted target ID (e.g. event.raw.redacts) for MatrixRedaction.eventId so downstream handlers receive the actual target event ID (ensure you reference the same variable names: MatrixRedaction, msg, and event).
981-992:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear login state/status entries on logout
At Line 981-992, logout removes the client/login, but leaves
#loginStatesuntouched. Bridge status can keep reporting a logged-out login.Suggested fix
async `#handleLogoutCommand`(command: MatrixCommand): Promise<MatrixCommandResponse> { const loginId = command.args[0]; if (!loginId) return { handled: true, text: "Usage: logout <login-id>" }; @@ this.#networkClients.delete(loginId); this.#userLogins.delete(loginId); + this.#loginStates.delete(loginId); + if (this.#bridgeStatus) { + this.#bridgeStatus = { ...this.#bridgeStatus, logins: this.#loginStatesRecord() }; + } await this.#deleteStoredUserLogin(loginId); this.#sendCurrentBridgeStatus(); return { handled: true, text: `Logged out: ${loginId}` }; }🤖 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 981 - 992, The logout handler in `#handleLogoutCommand` removes the client and login but leaves the in-memory `#loginStates` entry, causing stale status in `#sendCurrentBridgeStatus`; update `#handleLogoutCommand` to also remove the login's state (e.g., delete this.#loginStates[loginId] or this.#loginStates.delete(loginId) depending on its type) after disconnecting and before calling this.#sendCurrentBridgeStatus, and ensure any related per-login state maps (such as this.#networkClients, this.#userLogins) are cleared consistently and persisted by awaiting this.#deleteStoredUserLogin(loginId).
1114-1119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid immediate retry loop after drain failure
At Line 1116-1119, rescheduling in
finallyruns even when#drainRemoteEvents()throws. A permanently failing first event can cause a hot retry loop.Suggested fix
`#scheduleDrain`(): void { if (!this.#context) return; - this.#drainPromise ??= this.#drainRemoteEvents().finally(() => { - this.#drainPromise = null; - if (this.#context && this.#remoteEvents.length > 0) this.#scheduleDrain(); - }); + if (this.#drainPromise) return; + this.#drainPromise = this.#drainRemoteEvents() + .then(() => { + this.#drainPromise = null; + if (this.#context && this.#remoteEvents.length > 0) this.#scheduleDrain(); + }) + .catch(() => { + this.#drainPromise = null; + }); }🤖 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 1114 - 1119, The reschedule logic in `#scheduleDrain` currently runs in the finally of this.#drainRemoteEvents() so it retries immediately even on errors; change it to only reschedule immediately when the drain succeeds, and on rejection avoid a hot loop by scheduling a delayed retry (e.g., setTimeout/backoff) or not rescheduling at all; update the implementation around `#scheduleDrain` and the call to this.#drainRemoteEvents() so the success path re-invokes `#scheduleDrain` immediately when this.#remoteEvents.length>0 while the error path uses a controlled backoff delay before re-enqueueing scheduling.
427-436:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve canonical login data before persisting/loading client context
At Line 427-436, this path still persists the incoming
loginobject as-is. If callers pass{ id }, canonical fields (userId,remoteName, connector metadata) can be lost in memory/store and downstream connect context.Suggested fix
async `#loadUserLogin`(login: UserLogin): Promise<NetworkAPI> { - await this.#setLoginBridgeState(login, "CONNECTING"); - const client = await this.connector.loadUserLogin(this.#requestContext(), login); - await client.connect({ ...this.#requestContext(), login }); - login.client = client; - this.#userLogins.set(login.id, login); - this.#networkClients.set(login.id, client); + const stored = this.#userLogins.get(login.id) + ?? (this.#dataStore && hasMethod(this.#dataStore, "getUserLogin") + ? await this.#dataStore.getUserLogin(login.id) + : undefined); + const resolved = stored ? { ...stored, ...login } : login; + await this.#setLoginBridgeState(resolved, "CONNECTING"); + const client = await this.connector.loadUserLogin(this.#requestContext(), resolved); + await client.connect({ ...this.#requestContext(), login: resolved }); + resolved.client = client; + this.#userLogins.set(resolved.id, resolved); + this.#networkClients.set(resolved.id, client); if (this.#dataStore && hasMethod(this.#dataStore, "setUserLogin")) { - await this.#dataStore.setUserLogin(login); + await this.#dataStore.setUserLogin(resolved); } - await this.#setLoginBridgeState(login, "CONNECTED"); + await this.#setLoginBridgeState(resolved, "CONNECTED");🤖 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 427 - 436, The code is persisting the raw incoming login object instead of a canonicalized version; update async `#loadUserLogin` so you first resolve the canonical login (via the connector or client normalization API — e.g., call a canonicalize/resolveLogin helper on this.connector or client) and then use that canonicalLogin everywhere: pass canonicalLogin into client.connect, assign canonicalLogin.client and store canonicalLogin into this.#userLogins and this.#networkClients, and call this.#dataStore.setUserLogin(canonicalLogin) if present; ensure the original incoming object is not persisted but replaced by the resolved canonicalLogin before any map/store updates.
1334-1345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerive appservice domain when
homeserverDomainis omittedAt Line 1334-1345, MXID helpers interpolate
options.homeserverDomaindirectly. With optional-domain flows, this can produce:undefined.Suggested fix
+function appserviceDomain(options: MatrixAppserviceInitOptions): string { + return options.homeserverDomain ?? new URL(options.homeserver).hostname; +} + function appserviceBotUserId(options: MatrixAppserviceInitOptions): string { - return `@${options.registration.senderLocalpart}:${options.homeserverDomain}`; + return `@${options.registration.senderLocalpart}:${appserviceDomain(options)}`; } @@ - return `@${options.registration.senderLocalpart}_${escapedLocalId}:${options.homeserverDomain}`; + return `@${options.registration.senderLocalpart}_${escapedLocalId}:${appserviceDomain(options)}`; }🤖 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 1334 - 1345, The helpers appserviceBotUserId and ghostUserIdFromRegistration currently interpolate options.homeserverDomain and can produce ":undefined"; change them to derive a safe domain first (e.g. const domain = options.homeserverDomain ?? (() => { try { return new URL(options.registration.url).hostname } catch { return undefined } })() ?? throw new Error('missing homeserver domain')) and then use that domain variable when building MXIDs; update appserviceBotUserId (use domain) and ghostUserIdFromRegistration (use domain when returning the fallback MXID) so they never interpolate undefined.
🤖 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.
Duplicate comments:
In `@packages/bridge/src/bridge.ts`:
- Around line 1058-1060: The redaction dispatch is currently using the redaction
event's own ID (event.eventId) instead of the ID of the event being redacted;
update the MatrixRedaction construction (the msg object) to use the redacted
target ID (e.g. event.raw.redacts) for MatrixRedaction.eventId so downstream
handlers receive the actual target event ID (ensure you reference the same
variable names: MatrixRedaction, msg, and event).
- Around line 981-992: The logout handler in `#handleLogoutCommand` removes the
client and login but leaves the in-memory `#loginStates` entry, causing stale
status in `#sendCurrentBridgeStatus`; update `#handleLogoutCommand` to also remove
the login's state (e.g., delete this.#loginStates[loginId] or
this.#loginStates.delete(loginId) depending on its type) after disconnecting and
before calling this.#sendCurrentBridgeStatus, and ensure any related per-login
state maps (such as this.#networkClients, this.#userLogins) are cleared
consistently and persisted by awaiting this.#deleteStoredUserLogin(loginId).
- Around line 1114-1119: The reschedule logic in `#scheduleDrain` currently runs
in the finally of this.#drainRemoteEvents() so it retries immediately even on
errors; change it to only reschedule immediately when the drain succeeds, and on
rejection avoid a hot loop by scheduling a delayed retry (e.g.,
setTimeout/backoff) or not rescheduling at all; update the implementation around
`#scheduleDrain` and the call to this.#drainRemoteEvents() so the success path
re-invokes `#scheduleDrain` immediately when this.#remoteEvents.length>0 while the
error path uses a controlled backoff delay before re-enqueueing scheduling.
- Around line 427-436: The code is persisting the raw incoming login object
instead of a canonicalized version; update async `#loadUserLogin` so you first
resolve the canonical login (via the connector or client normalization API —
e.g., call a canonicalize/resolveLogin helper on this.connector or client) and
then use that canonicalLogin everywhere: pass canonicalLogin into
client.connect, assign canonicalLogin.client and store canonicalLogin into
this.#userLogins and this.#networkClients, and call
this.#dataStore.setUserLogin(canonicalLogin) if present; ensure the original
incoming object is not persisted but replaced by the resolved canonicalLogin
before any map/store updates.
- Around line 1334-1345: The helpers appserviceBotUserId and
ghostUserIdFromRegistration currently interpolate options.homeserverDomain and
can produce ":undefined"; change them to derive a safe domain first (e.g. const
domain = options.homeserverDomain ?? (() => { try { return new
URL(options.registration.url).hostname } catch { return undefined } })() ??
throw new Error('missing homeserver domain')) and then use that domain variable
when building MXIDs; update appserviceBotUserId (use domain) and
ghostUserIdFromRegistration (use domain when returning the fallback MXID) so
they never interpolate undefined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e00c2152-4f20-40c4-9a6e-ccfcaa7b577f
📒 Files selected for processing (3)
packages/bridge/README.mdpackages/bridge/src/bridge.tspackages/bridge/src/types.ts
✅ Files skipped from review due to trivial changes (1)
- packages/bridge/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bridge/src/types.ts
📜 Review details
🧰 Additional context used
🪛 OpenGrep (1.20.0)
packages/bridge/src/bridge.ts
[ERROR] 770-770: 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] 780-780: 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] 1348-1348: 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] 1350-1350: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
1 participant
Summary
@beeper/pickle-bridgeworkspace package for building bridges on top of PickleTesting
pnpm --filter @beeper/pickle-bridge testpnpm --filter @beeper/pickle-bridge typecheckpnpm --filter @beeper/pickle-bridge build