refactor(remote-bridge): rewrite the Go bridge as a TypeScript port#1336
Merged
Conversation
Replace the standalone Go module (cc-connect sidecar) with a faithful TypeScript port that lives natively in the Node/Electron runtime, dropping the Go toolchain and cross-language plumbing while preserving the bridge's hard-won semantics: - session pointers: remoteKey<->session map, parent chains, atomic file writes via a process-unique temp name + rename - PawWork HTTP/SSE client: fatal-vs-transient classification, Last-Event-ID cursor persistence, per-directory hydration that skips transient blips but surfaces fatal/malformed responses - event dispatch: advance the cursor before reconciling on an undecodable critical frame, so one bad event never wedges the stream or hides a prompt - engine: a single active blocker per root, delivered-before-answerable ordering, bounded delivery retry, child->root conversation routing - gateway run-loop: connect -> stream ready -> hydrate -> start platforms; retry non-fatal stream errors; audience gating; the chat platform sits behind a `Platform` seam fed by an injected factory 71 unit/contract tests ported from the Go *_test.go suites; typecheck clean. The live Lark adapter binding (@larksuite/vercel-chat-adapter) and the Electron utilityProcess worker wiring land in the desktop-integration follow-up, where they are validated by a live Feishu run.
Contributor
|
Warning Review limit reached
More reviews will be available in 30 minutes and 50 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe entire Changesremote-bridge Go → TypeScript port
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
github-actions
Bot
added
ci
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the remote-bridge package from Go to TypeScript/Bun, introducing the @opencode-ai/remote-bridge workspace package along with comprehensive test suites. The code review highlights several critical areas for improvement in the new TypeScript implementation, specifically addressing potential TypeError crashes when handling empty JSON responses in the PawWork client, a potential connection leak in the SSE stream parser if errors occur during reading, and the need for proper AbortSignal propagation to support request cancellation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- ci: replace the deleted Go test steps in build.yml with bun test - hydrate: surface an undecodable /external-result question instead of dropping a pending one (listQuestions throws on incomplete) - events: treat a wrong-typed questions field as undecodable and reconcile, never surface an empty-question prompt - pointers: restore the legacy bare-map fallback so an upgrade keeps its chat-to-session bindings - gateway: a platform start() that resolves on its own no longer tears the bridge down; stay up until abort or a fatal error - tests: cover each behavior; fix a tsgo null-narrowing typecheck break in the hydration test via a holder object
…ing start - build-workflow.test.ts asserted the deleted Go test/race steps; update it to the bun step and assert the race step is gone (both reviewers: CI was red) - gateway: add the missing test for a platform whose start() resolves on its own — run() must stay up until abort, the behavior the prior fix introduced
github-actions
Bot
added
the
harness
Astro-Han
added
the
task
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/remote-bridge/src/engine.ts`:
- Around line 328-347: In the `restoreDelivery` method, after awaiting the
`platform.reconstructReplyCtx(key)` call and checking for
`this.active.get(sessionID)`, add an additional check for the root active
target. If a root active target exists (which may have been set while the
reconstruction was pending), return that active target instead of proceeding to
create a new proactive delivery with the reconstructed context. This ensures
that if a user message established a fresh active conversation at the root level
during the await, that takes precedence over the proactively reconstructed
target.
- Around line 405-410: The ensureSession method has a race condition where
multiple concurrent calls with the same key can both observe no existing session
and both call createSession(), splitting the conversation across multiple
sessions. Add serialization using a per-key locking mechanism (such as a Map
that tracks in-flight session creation promises) so that only one thread creates
a session for each key. After acquiring the lock, check again if a session
already exists (in case another thread created it while waiting), and only
proceed to call this.sidecar.createSession() and this.setCurrent() if no session
was created by another thread.
In `@packages/remote-bridge/src/gateway.ts`:
- Around line 222-225: The handleEventRepairRefresh handler needs to be wired to
trigger hydration when parseSSE() skips an undecodable critical event and
advances the cursor. Currently, the repair refresh handler only hydrates when
server.connected is true, which delays reconciliation of state changes until a
later reconnect. Find the handleEventRepairRefresh handler definition and ensure
it calls this.hydrate(signal) immediately when invoked, similar to how
handleReplayRefresh is already doing so in the diff context, to ensure pending
state from skipped critical frames is reconciled without waiting for a
reconnection.
In `@packages/remote-bridge/src/pawwork-events.ts`:
- Around line 86-90: The validation in the pawwork-events.ts file at lines 86-90
and 205-213 only checks if patterns and questions are arrays, but does not
validate the types of their contents. Add deeper validation after the array type
check to ensure that patterns contains only strings and questions contains
properly typed objects with string header and description fields. When invalid
nested types are detected (e.g., patterns containing numbers or questions with
non-string descriptions), throw a RepairableEventError with an appropriate
message instead of allowing the malformed data to reach the Engine where it will
cause rendering failures when .trim() is called on non-string values.
- Around line 260-265: The try-catch block around the
`handler.handleEventRepairRefresh()` call swallows the error by only logging it
with console.warn, which prevents the failure from propagating up to the stream
loop for reconnection and retry. Since the cursor has already advanced past the
repairable event, suppressing this error can leave pending state hidden
indefinitely. Remove the catch block that swallows the error or rethrow it
instead so that failures in handleEventRepairRefresh propagate to the caller and
trigger stream loop reconnection and hydration retry.
In `@packages/remote-bridge/src/session-pointers.ts`:
- Around line 123-131: The writeFile call in the writeSnapshot method relies on
process umask for file permissions, which could make the session state file
readable by other local users. Add an explicit chmod call with mode 0o600 on the
tempPath after the writeFile call and before the rename call to ensure the
temporary file has private read-write-only permissions for the owner, preventing
other users from accessing sensitive session data like remote keys and cursor
state.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 257e2c7b-cfb3-4626-9f99-fcb70589eb98
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackages/remote-bridge/go.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
.github/workflows/build.ymlpackages/opencode/test/github/build-workflow.test.tspackages/remote-bridge/cmd/pawwork-remote-bridge/main.gopackages/remote-bridge/go.modpackages/remote-bridge/internal/bridge/engine.gopackages/remote-bridge/internal/bridge/engine_test.gopackages/remote-bridge/internal/bridge/session_pointers.gopackages/remote-bridge/internal/bridge/session_pointers_test.gopackages/remote-bridge/internal/gateway/gateway.gopackages/remote-bridge/internal/gateway/gateway_test.gopackages/remote-bridge/internal/pawwork/client.gopackages/remote-bridge/internal/pawwork/client_test.gopackages/remote-bridge/internal/pawwork/events.gopackages/remote-bridge/internal/pawwork/events_test.gopackages/remote-bridge/internal/platforms/platforms.gopackages/remote-bridge/internal/platforms/platforms_test.gopackages/remote-bridge/package.jsonpackages/remote-bridge/src/engine.test.tspackages/remote-bridge/src/engine.tspackages/remote-bridge/src/gateway.test.tspackages/remote-bridge/src/gateway.tspackages/remote-bridge/src/pawwork-client-http.test.tspackages/remote-bridge/src/pawwork-client-hydration.test.tspackages/remote-bridge/src/pawwork-client.test.tspackages/remote-bridge/src/pawwork-client.tspackages/remote-bridge/src/pawwork-events.test.tspackages/remote-bridge/src/pawwork-events.tspackages/remote-bridge/src/session-pointers.test.tspackages/remote-bridge/src/session-pointers.tspackages/remote-bridge/src/types.tspackages/remote-bridge/tsconfig.json
💤 Files with no reviewable changes (14)
- packages/remote-bridge/cmd/pawwork-remote-bridge/main.go
- packages/remote-bridge/go.mod
- packages/remote-bridge/internal/bridge/engine.go
- packages/remote-bridge/internal/pawwork/events.go
- packages/remote-bridge/internal/bridge/session_pointers.go
- packages/remote-bridge/internal/gateway/gateway.go
- packages/remote-bridge/internal/platforms/platforms_test.go
- packages/remote-bridge/internal/pawwork/client.go
- packages/remote-bridge/internal/pawwork/events_test.go
- packages/remote-bridge/internal/bridge/engine_test.go
- packages/remote-bridge/internal/bridge/session_pointers_test.go
- packages/remote-bridge/internal/platforms/platforms.go
- packages/remote-bridge/internal/pawwork/client_test.go
- packages/remote-bridge/internal/gateway/gateway_test.go
Validated each finding against current code and the Go original. Fidelity/crash fixes (TS diverged from Go or could crash): - client: default list responses to [] — doJSON returns undefined on an empty 2xx body, where Go's nil slice ranged zero times (listSessions/Permissions/ Questions) - events: cancel the stream reader on any error path, matching Go's deferred Body.Close, so a thrown dispatch/reconcile cannot leak the connection - events: reject wrong-typed nested permission/question fields (patterns:[123], non-string header/option) — Go's typed unmarshal rejected these; coercion would crash prompt rendering on .trim() and wedge the blocker - pointers: write the state file 0o600 (Go's os.CreateTemp default), not umask - thread an AbortSignal through Sidecar.listSessions and hydrate (Go took ctx) Deliberate divergence from Go (races confirmed real by /codex, opt-in hardening): - engine: coalesce concurrent first-session creation per key onto one in-flight promise — fire-and-forget dispatch let two messages split a new conversation - engine: restoreDelivery re-checks the root active target too after reconstruct Tests cover each behavior; the two race fixes are labeled stricter-than-Go. Pushed back (no change): handleEventRepairRefresh is already wired via ClientEventHandler (matches Go's clientEventHandler); a failed mid-stream repair refresh is logged-and-continued in Go too (only reconnect propagates); and title/parentID stay "" since Session.title is string and sessionLabel calls .trim().
Boolean("false") is true, so a malformed multiple:"false" silently flipped a
single-select question to multi-select. Go's `Multiple bool` unmarshal rejected
it; questionHasWrongTypes now does too (reconcile), and the mapper reads
multiple as q?.multiple ?? false. Completes the strict-decode parity for
nested question fields.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/remote-bridge/src/pawwork-events.test.ts (1)
2-2: ⚡ Quick winTighten error assertions to validate the intended failure mode.
These cases use
rejects.toThrow()without checking error type, so unrelated runtime throws can still pass. Assert the specific error classes (RepairableEventErrorfor reconciliation cases,ReplayRefreshErrorfor replay-refresh) to prevent false positives.Suggested test hardening
-import { dispatchEvent, type EventHandler, parseSSE, ReplayRefreshError } from "./pawwork-events.ts" +import { dispatchEvent, type EventHandler, parseSSE, ReplayRefreshError, RepairableEventError } from "./pawwork-events.ts" - ).rejects.toThrow() + ).rejects.toThrow(RepairableEventError) - ).rejects.toThrow() + ).rejects.toThrow(ReplayRefreshError)Also applies to: 144-149, 157-163, 169-175, 182-188, 207-208
🤖 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/remote-bridge/src/pawwork-events.test.ts` at line 2, The test assertions in the pawwork-events test file use generic `rejects.toThrow()` calls without validating specific error types, which allows unrelated runtime errors to pass tests incorrectly. Update all these error assertions to check for the specific error classes: use `.rejects.toThrow(RepairableEventError)` for reconciliation-related test cases and `.rejects.toThrow(ReplayRefreshError)` for replay-refresh test cases. First, ensure that both `RepairableEventError` and `ReplayRefreshError` are imported in the test file at the top (they may already be exported from pawwork-events.ts); then locate each `rejects.toThrow()` call throughout the test file and replace it with the appropriate specific error class assertion based on the test's intent.
🤖 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/remote-bridge/src/engine.test.ts`:
- Line 737: Replace the fixed sleep at line 737 and line 771 in
packages/remote-bridge/src/engine.test.ts with deterministic synchronization
instead of using setTimeout with an arbitrary delay. Rather than awaiting a
promise that resolves after 5ms, use explicit synchronization primitives like
"entered barrier" promises returned from createSession or reconstructReplyCtx
helper functions to ensure operations have actually completed before proceeding,
making the test ordering deterministic and preventing scheduler-dependent
flakiness.
---
Nitpick comments:
In `@packages/remote-bridge/src/pawwork-events.test.ts`:
- Line 2: The test assertions in the pawwork-events test file use generic
`rejects.toThrow()` calls without validating specific error types, which allows
unrelated runtime errors to pass tests incorrectly. Update all these error
assertions to check for the specific error classes: use
`.rejects.toThrow(RepairableEventError)` for reconciliation-related test cases
and `.rejects.toThrow(ReplayRefreshError)` for replay-refresh test cases. First,
ensure that both `RepairableEventError` and `ReplayRefreshError` are imported in
the test file at the top (they may already be exported from pawwork-events.ts);
then locate each `rejects.toThrow()` call throughout the test file and replace
it with the appropriate specific error class assertion based on the test's
intent.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 416bb697-8702-4498-9129-2a4ced192221
📒 Files selected for processing (10)
packages/remote-bridge/src/engine.test.tspackages/remote-bridge/src/engine.tspackages/remote-bridge/src/gateway.tspackages/remote-bridge/src/pawwork-client-hydration.test.tspackages/remote-bridge/src/pawwork-client.tspackages/remote-bridge/src/pawwork-events.test.tspackages/remote-bridge/src/pawwork-events.tspackages/remote-bridge/src/session-pointers.test.tspackages/remote-bridge/src/session-pointers.tspackages/remote-bridge/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/remote-bridge/src/pawwork-client-hydration.test.ts
- packages/remote-bridge/src/types.ts
- packages/remote-bridge/src/session-pointers.test.ts
- packages/remote-bridge/src/gateway.ts
- packages/remote-bridge/src/engine.ts
- packages/remote-bridge/src/pawwork-events.ts
- packages/remote-bridge/src/session-pointers.ts
Add a unit-remote-bridge job to ci.yml that runs the package's 83 tests on every pull request, mirroring unit-desktop. Previously the tests ran only in build.yml, which is workflow_dispatch-only, so TS test regressions could merge unseen. Typecheck was already covered by the gating `bun turbo typecheck` job. - package.json: add test:ci (junit reporter, scoped to ./src to bypass the root bunfig test root); fix test to `bun test ./src` - turbo.json: declare @opencode-ai/remote-bridge#test:ci - ci.yml: new job + wire into the `check` result gate - ci-workflow.test.ts: extend the workflow contract in lockstep
…iers The two concurrency regression tests coordinated their gates with a fixed 5ms setTimeout, which is scheduler-dependent. Replace each with an "entered" barrier resolved from inside the gated async (createSession / reconstructReplyCtx): the test awaits the signal that the first operation has parked before releasing the gate, so ordering no longer relies on timing. Behavior under test is unchanged.
Go decodes every SSE event and REST hydration row into a typed struct, so a wrong-typed scalar is an unmarshal error, not a lenient coercion. The TS port used `?? ""`/falsy extraction, letting a number/bool/object in a top-level field flow into the engine — where it could crash prompt rendering on .trim(), surface a bogus question (externalResultReady: "false" read as true), or clear the wrong blocker (numeric status read as resolved). Add small strict scalar decoders (decodeString/Boolean/StringArray/ OptionalNumber) — the single shared definition of a valid scalar — and route permission, session, assistant-text, and question decoding plus the listSessions/listPermissions hydration rows through them. A wrong-typed critical SSE event now reconciles (RepairableEventError); a wrong-typed assistant-text part is skipped; a wrong-typed hydration row fails the list. This replaces the scattered ad-hoc guards with one boundary and matches Go's typed-unmarshal semantics. Zero new dependencies. Adds focused malformed-field tests for permission id/sessionID/permission, session info.id/title/parentID, assistant sessionID/text, question status/externalResultReady/messageID/callID, and wrong-typed REST rows.
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Rewrites
packages/remote-bridgefrom a standalone Go module (a cc-connect sidecar binary) into a faithful TypeScript port that lives natively in the Node/Electron runtime. Drops the Go toolchain, the compiled sidecar binary, and all cross-language plumbing.This supersedes the Go implementation merged in #1275. Part of #1188 (mobile companion).
Why
The bridge talks to PawWork's opencode server over local HTTP + SSE — code we already have in TypeScript. The Go module existed only because cc-connect is Go. Vercel's Chat SDK +
@larksuite/vercel-chat-adapterprovide the same messaging-bridge capability as a TypeScript library (outbound WebSocket long-connection, no public IP/webhook; native cardkit streaming; interactive cards), so the bridge no longer needs to be a separate-language sidecar. It can live in-process with the engine it already shares a runtime with.What's preserved (the hard semantics)
.tmpwriter hit on reconnect)Last-Event-IDcursor persistence, per-directory hydration that skips transient blips but surfaces fatal/malformed responses (never silently drops a pending permission)allow_from, or Feishu/Larkallow_chat+group_only). The chat platform sits behind aPlatformseam fed by an injected factory.Testing
*_test.gosuites (session-pointers, client HTTP/SSE, event dispatch, engine, gateway run-loop), all green.tsgo --noEmittypecheck clean.Deliberately deferred to the desktop-integration follow-up (supersedes #1334)
The chat platform sits behind a
Platforminterface + injected factory, so the testable core ships independently. The two pieces whose only real validation is a live Feishu run land next:@larksuite/vercel-chat-adapteras aPlatform(inbound text → handler; outboundthread.post;reconstructReplyCtxvia the thread-id codec; audience filter viaisDM/channelIdFromThreadId).utilityProcess.forkhost — per a codex architecture review: the embedded opencode server runs in-process in Electron main, so the bridge's lifecycle tracks the engine, but the beta chat adapter is isolated into a main-owned utility process (crash isolation without external-Node packaging pain). Started fire-and-monitor after the health-wait; stopped bridge-then-server inkillSidecar.Summary by CodeRabbit
Release Notes
New Features
/new, session management commands, ordered permission/question prompting, and delivery retries.Refactor
Chores