feat(cli): add provider-agnostic channel bridge core#109
feat(cli): add provider-agnostic channel bridge core#109cdenneen wants to merge 1 commit intohappier-dev:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new provider-agnostic Channel Bridge worker and an extensive test suite plus docs. The worker maps external channel conversations to CLI sessions, handles slash commands, forwards inbound messages to sessions, relays agent replies to channels, persists cursors, deduplicates inbound messages, and runs via a tick-driven lifecycle with graceful shutdown. Changes
Sequence Diagram(s)sequenceDiagram
participant Adapter as Channel Adapter
participant Worker as Channel Bridge Worker
participant Store as Binding Store
participant Deps as CLI Session Deps
loop Periodic Tick
Adapter->>Worker: pullInboundMessages()
Worker->>Worker: parseSlashCommand / classify message
alt Slash command
Worker->>Deps: resolveSessionIdOrPrefix / listSessions
Worker->>Store: upsertBinding / removeBinding / listBindings
Worker->>Adapter: sendMessage (UI hint)
else User message (bound)
Worker->>Store: getBinding(ref)
Worker->>Deps: sendUserMessageToSession(sessionId, message)
else User message (unbound)
Worker->>Adapter: sendMessage (no-binding hint)
end
Worker->>Store: listBindings()
loop For each binding
Worker->>Deps: resolveLatestSessionSeq(sessionId)
Worker->>Deps: fetchAgentMessagesAfterSeq(sessionId, afterSeq)
Deps-->>Worker: agent messages
Worker->>Adapter: sendMessage (agent reply)
Worker->>Store: updateLastForwardedSeq(ref, newSeq)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR introduces the provider-agnostic channel bridge core — a single-file worker ( Two minor observability gaps remain:
Confidence Score: 4/5
Sequence DiagramsequenceDiagram
participant A as ChannelAdapter
participant W as ChannelBridgeWorker
participant S as ChannelBindingStore
participant D as ChannelBridgeDeps
W->>A: pullInboundMessages()
A-->>W: messages
loop per inbound message
W->>W: deduper.isDuplicate(message)
alt slash command
W->>D: authorizeCommand(commandName, actor)
W->>S: getBinding / upsertBinding / removeBinding
W->>A: sendMessage(reply)
else regular text with binding
W->>S: getBinding(ref)
W->>D: sendUserMessageToSession(sessionId, text)
else regular text without binding
W->>A: sendMessage("No session attached")
end
end
W->>S: listBindings()
S-->>W: bindings
loop per binding
W->>D: fetchAgentMessagesAfterSeq(sessionId, cursor)
D-->>W: rows sorted by seq
loop per row
W->>A: sendMessage(text)
W->>S: updateLastForwardedSeq(binding, seq)
end
end
Last reviewed commit: 9e9b0d8 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/cli/src/channels/core/channelBridgeWorker.test.ts (1)
41-203: Add lifecycle coverage forstartChannelBridgeWorker(triggerandstop).
executeChannelBridgeTickis covered well, but this PR also introduces a public worker lifecycle API. Add tests for initial trigger behavior, stop idempotency, and adapterstop()propagation.As per coding guidelines: "Write unit tests for all public functions and components".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/core/channelBridgeWorker.test.ts` around lines 41 - 203, Add unit tests for the new public lifecycle API startChannelBridgeWorker: write a test that calls startChannelBridgeWorker(...) then calls trigger() once to ensure the worker runs its first tick (you can assert by spying on executeChannelBridgeTick or deps called), a test that calls stop() twice to verify idempotency, and a test that ensures adapter.stop() is called when stop() is invoked (spy on the adapter harness' stop method). Use the existing createAdapterHarness(), in-memory store, and deps mocks from the file to locate where to hook spies and assertions; reference startChannelBridgeWorker, trigger, stop, and adapter.stop to find the right symbols to exercise and assert.apps/cli/src/channels/core/channelBridgeWorker.ts (1)
1-368: Add module and exported API JSDoc comments.This file defines public bridge contracts and complex flow control, but there’s no file header/API JSDoc. Please document responsibilities, expected adapter behavior, and cursor semantics to reduce integration errors.
As per coding guidelines: "Include comprehensive JSDoc comments as file header comments explaining file responsibilities" and "Add JSDoc comments for public APIs and complex logic".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/core/channelBridgeWorker.ts` around lines 1 - 368, Add a file-level JSDoc header describing the module responsibility (bridging channel adapters to sessions, command handling, and agent->channel cursor semantics) and add JSDoc comments for each public exported type/function: ChannelBridgeAdapter (describe pullInboundMessages/sendMessage/optional stop contract and expected error handling), ChannelBindingStore and ChannelSessionBinding (explain lastForwardedSeq meaning and that updateLastForwardedSeq stores the highest seq forwarded), ResolveSessionIdResult, createInMemoryChannelBindingStore (note now() usage and normalization of seqs), executeChannelBridgeTick (document flow: inbound pull -> commands -> forward user messages -> fetchAgentMessagesAfterSeq where afterSeq is exclusive and returned messages should have seq > afterSeq), and startChannelBridgeWorker (document tickMs bounds, SingleFlightIntervalLoop usage and trigger/stop behavior); ensure comments mention expected behavior of deps methods (resolveLatestSessionSeq, fetchAgentMessagesAfterSeq) and error/warning callbacks so integrators know contracts and cursor semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cli/src/channels/core/channelBridgeWorker.test.ts`:
- Around line 46-58: The test currently uses vi.fn() mocks for ChannelBridgeDeps
(listSessions, resolveSessionIdOrPrefix, fetchAgentMessagesAfterSeq, etc.);
replace these mocked spies with lightweight real/test implementations of the
ChannelBridgeDeps interface (for example in-memory or fixture implementations
that implement listSessions, resolveSessionIdOrPrefix, sendUserMessageToSession,
resolveLatestSessionSeq, fetchAgentMessagesAfterSeq, onWarning) so the test
exercises actual behavior instead of mocks, and update assertions to verify real
outcomes; additionally add one or more tests that call the public
startChannelBridgeWorker function (using the same real/test ChannelBridgeDeps)
to cover its lifecycle and observable side-effects (session resolution, message
fetching/sending, and warnings) rather than only testing internal helpers.
In `@apps/cli/src/channels/core/channelBridgeWorker.ts`:
- Around line 247-253: The handler currently trusts event.providerId from
inbound payload which can allow spoofing; normalize and override provider
identity with the adapter's trusted providerId before passing to command
handling and forwarding. Update the call to handleCommand in channelBridgeWorker
to pass event.payload/providerId (or any field named providerId) set to
adapter.providerId, and ensure any later forwarding logic in the same function
(the block around lines 257-279) also replaces uses of event.providerId with
adapter.providerId so routing/binding uses adapter.providerId throughout;
specifically adjust where handleCommand is invoked and where outbound send/bind
logic reads event.providerId.
- Around line 206-216: The current logic uses
deps.resolveLatestSessionSeq(...).catch(() => 0) which can persist a binding
with lastForwardedSeq=0 and replay full history; change the flow in the attach
path so you do not upsertBinding or confirm the attach when seq resolution
fails: call resolveLatestSessionSeq(resolved.sessionId) and if it throws or
returns an invalid cursor, send a failure reply via replyToConversation(adapter,
ref, ...) and return false (or abort) instead of calling store.upsertBinding;
only persist the binding and send the success message after a successful seq
resolution. Ensure you update the code paths around resolveLatestSessionSeq,
store.upsertBinding, and replyToConversation so the binding is only created when
a valid latestSeq is obtained.
- Around line 242-255: The current loop over params.adapters will abort the
whole tick if adapter.pullInboundMessages() or handleCommand() throws; wrap the
call to adapter.pullInboundMessages() in a try/catch so a failing adapter is
logged and the loop continues to the next adapter, and inside the inbound-events
loop wrap handleCommand(...) (and parseSlashCommand if you prefer) in a
try/catch so an error for one event is logged and skipped without stopping
processing of remaining events/adapters; reference the existing symbols:
params.adapters, adapter.pullInboundMessages(), parseSlashCommand(),
handleCommand({ command, event, adapter, store: params.store, deps: params.deps
}) and use the existing logger (e.g. params.deps.logger.error) or console.error
to record the adapter/event and error details.
---
Nitpick comments:
In `@apps/cli/src/channels/core/channelBridgeWorker.test.ts`:
- Around line 41-203: Add unit tests for the new public lifecycle API
startChannelBridgeWorker: write a test that calls startChannelBridgeWorker(...)
then calls trigger() once to ensure the worker runs its first tick (you can
assert by spying on executeChannelBridgeTick or deps called), a test that calls
stop() twice to verify idempotency, and a test that ensures adapter.stop() is
called when stop() is invoked (spy on the adapter harness' stop method). Use the
existing createAdapterHarness(), in-memory store, and deps mocks from the file
to locate where to hook spies and assertions; reference
startChannelBridgeWorker, trigger, stop, and adapter.stop to find the right
symbols to exercise and assert.
In `@apps/cli/src/channels/core/channelBridgeWorker.ts`:
- Around line 1-368: Add a file-level JSDoc header describing the module
responsibility (bridging channel adapters to sessions, command handling, and
agent->channel cursor semantics) and add JSDoc comments for each public exported
type/function: ChannelBridgeAdapter (describe
pullInboundMessages/sendMessage/optional stop contract and expected error
handling), ChannelBindingStore and ChannelSessionBinding (explain
lastForwardedSeq meaning and that updateLastForwardedSeq stores the highest seq
forwarded), ResolveSessionIdResult, createInMemoryChannelBindingStore (note
now() usage and normalization of seqs), executeChannelBridgeTick (document flow:
inbound pull -> commands -> forward user messages -> fetchAgentMessagesAfterSeq
where afterSeq is exclusive and returned messages should have seq > afterSeq),
and startChannelBridgeWorker (document tickMs bounds, SingleFlightIntervalLoop
usage and trigger/stop behavior); ensure comments mention expected behavior of
deps methods (resolveLatestSessionSeq, fetchAgentMessagesAfterSeq) and
error/warning callbacks so integrators know contracts and cursor semantics.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 135e570b-02cd-4832-9f9d-772aa7c7e57f
📒 Files selected for processing (2)
apps/cli/src/channels/core/channelBridgeWorker.test.tsapps/cli/src/channels/core/channelBridgeWorker.ts
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@coderabbitai please re-review this PR now that all bot threads have explicit resolution notes and follow-up hardening is complete. @greptile-apps please run a fresh pass as well. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cli/src/channels/core/channelBridgeWorker.ts`:
- Around line 565-570: The current Promise.all over params.adapters can abort if
any adapter.stop() throws; update the shutdown mapping in channelBridgeWorker.ts
to wrap each adapter.stop() call in a try/catch so every adapter.stop() is
attempted even if one fails, log the specific error (including adapter
identifier if available) inside the catch, and return a resolved value so
Promise.all waits for all wrappered stops to complete rather than rejecting on
the first error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05bcf020-8e3c-440d-9b84-d2c5072a6b1c
📒 Files selected for processing (2)
apps/cli/src/channels/core/channelBridgeWorker.test.tsapps/cli/src/channels/core/channelBridgeWorker.ts
|
Follow-up clarity pass — here are exact fix locations for the resolved bot items (commit
I added this map so reviewers can verify each previously-resolved thread against concrete code anchors quickly. |
|
@coderabbitai @greptile-apps follow-up fixes were pushed (commit 3a5d30a) for the latest unresolved threads; please re-review. |
|
✅ Actions performedFull review triggered. |
|
@coderabbitai @greptile-apps re-review |
|
✅ Actions performedFull review triggered. |
|
@greptile review |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cli/src/channels/core/channelBridgeWorker.test.ts`:
- Around line 700-711: Wrap each test that starts a channel worker with a
try/finally so worker.stop() always runs: for tests that call
startChannelBridgeWorker(...) and then await waitFor(...) / assert on
harness.sent, move assertions into the try block and call await worker.stop() in
the finally block to ensure timers and in-flight work are cleaned up (apply the
same pattern to other tests that startChannelBridgeWorker, reference the worker
variable and its stop() method in the finally).
- Around line 67-73: The test's dedupe verification is passing prematurely
because after calling worker.trigger() the first tick already makes
sentToSession.length === 1 true; update the assertions in createAdapterHarness
tests (and the other occurrences at the noted sites) to deterministically wait
for the second tick before asserting dedupe behavior: after the first
worker.trigger() await the next processing cycle (e.g., call worker.trigger() a
second time or await a Promise that resolves when sentToSession.length >= 2) and
then assert that the duplicate message was ignored (sentToSession length remains
1) or that a dedupe-specific indicator was emitted; reference the harness
functions createAdapterHarness, pushInbound, sent (or sentToSession) and
worker.trigger() when updating each failing assertion to ensure the test
verifies the second tick outcome.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b7334bcb-a690-4b9f-8698-5f4f931f4799
📒 Files selected for processing (2)
apps/cli/src/channels/core/channelBridgeWorker.test.tsapps/cli/src/channels/core/channelBridgeWorker.ts
✅ Files skipped from review due to trivial changes (1)
- apps/cli/src/channels/core/channelBridgeWorker.ts
…back (cherry picked from commit 51bb9ea)
|
Addressed remaining feedback in commit 7fb5b65 (test determinism + lifecycle cleanup guards in channel bridge worker tests). @coderabbitai review |
|
✅ Actions performedFull review triggered. |
|
✅ Actions performedFull review triggered. |
|
@greptile review |
|
@coderabbitai review |
|
@greptile review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
|
@greptile review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/cli/src/channels/core/channelBridgeWorker.ts (1)
210-230:⚠️ Potential issue | 🟠 MajorNormalize in-memory cursor values with
toNonNegativeIntto preventNaNcursor corruption.Line 218 and Line 229 can still persist/propagate non-finite cursor values (
NaN), which breaks monotonic cursor semantics and downstreamafterSeqcomparisons.🛠️ Proposed fix
upsertBinding: async (binding) => { const key = bindingKey(binding); const existing = byKey.get(key); + const normalizedLastForwardedSeq = toNonNegativeInt(binding.lastForwardedSeq) ?? 0; const next: ChannelSessionBinding = { providerId: binding.providerId, conversationId: binding.conversationId, threadId: binding.threadId, sessionId: binding.sessionId, - lastForwardedSeq: Math.max(0, Math.trunc(binding.lastForwardedSeq)), + lastForwardedSeq: normalizedLastForwardedSeq, createdAtMs: existing?.createdAtMs ?? now(), updatedAtMs: now(), }; byKey.set(key, next); return next; }, updateLastForwardedSeq: async (ref, seq) => { const key = bindingKey(ref); const existing = byKey.get(key); if (!existing) return; - const nextSeq = Math.max(existing.lastForwardedSeq, Math.max(0, Math.trunc(seq))); + const parsedSeq = toNonNegativeInt(seq); + if (parsedSeq === null) return; + const nextSeq = Math.max(existing.lastForwardedSeq, parsedSeq); byKey.set(key, { ...existing, lastForwardedSeq: nextSeq, updatedAtMs: now(), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/core/channelBridgeWorker.ts` around lines 210 - 230, The upsertBinding and updateLastForwardedSeq implementations can accept NaN/Infinity and corrupt in-memory cursors; replace the Math.max(0, Math.trunc(...)) sanitization with a call to toNonNegativeInt for both binding.lastForwardedSeq (in upsertBinding when building next.lastForwardedSeq) and seq and existing.lastForwardedSeq (in updateLastForwardedSeq when computing nextSeq) so you always store a finite non-negative integer; update uses of existing?.lastForwardedSeq and computed nextSeq to pass through toNonNegativeInt before persisting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/cli/src/channels/core/channelBridgeWorker.ts`:
- Around line 210-230: The upsertBinding and updateLastForwardedSeq
implementations can accept NaN/Infinity and corrupt in-memory cursors; replace
the Math.max(0, Math.trunc(...)) sanitization with a call to toNonNegativeInt
for both binding.lastForwardedSeq (in upsertBinding when building
next.lastForwardedSeq) and seq and existing.lastForwardedSeq (in
updateLastForwardedSeq when computing nextSeq) so you always store a finite
non-negative integer; update uses of existing?.lastForwardedSeq and computed
nextSeq to pass through toNonNegativeInt before persisting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4947242d-a9cc-4087-903d-96d0f652adb2
📒 Files selected for processing (2)
apps/cli/src/channels/core/channelBridgeWorker.test.tsapps/cli/src/channels/core/channelBridgeWorker.ts
cdenneen
force-pushed
the
feat/channel-bridge-core
branch
from
a83c471 to
6994e4a
Compare
cdenneen
force-pushed
the
feat/channel-bridge-core
branch
from
6994e4a to
7339299
Compare
|
@coderabbitai review |
|
@greptile review |
✅ Actions performedReview triggered.
|
cdenneen
force-pushed
the
feat/channel-bridge-core
branch
from
7339299 to
9e9b0d8
Compare
|
@coderabbitai review |
|
@greptile review |
✅ Actions performedReview triggered.
|
| threadId: event.threadId, | ||
| }); | ||
| } catch (error) { | ||
| params.deps.onWarning?.('Failed to forward channel message into session', error); |
There was a problem hiding this comment.
Inbound failure warning lacks identifying context.
'Failed to forward channel message into session' is emitted with no mention of which session, provider, or conversation was affected. In a multi-binding deployment this warning is hard to correlate to a specific conversation. The outbound counterpart at line 648 includes session=, provider=, and conversation= identifiers — the same treatment would make this warning equally actionable:
| params.deps.onWarning?.('Failed to forward channel message into session', error); | |
| params.deps.onWarning?.(`Failed to forward channel message into session ${binding.sessionId} (provider=${event.providerId} conversation=${event.conversationId})`, error); |
| params.deps.onWarning?.( | ||
| `No adapter registered for binding providerId=${binding.providerId} conversationId=${binding.conversationId}; skipping outbound forwarding`, | ||
| ); |
There was a problem hiding this comment.
Orphaned-binding warning fires on every tick without throttling.
When a binding exists for a provider with no registered adapter, this warning fires on every tick (default 2,500 ms). With even a handful of orphaned bindings — a natural operational state after an adapter is removed while its bindings remain in the store — the warning stream becomes a continuous flood with no signal about whether the situation is new or ongoing.
Consider tracking which orphaned bindings have already been warned within the current worker run (e.g. a Set maintained alongside the deduper in startChannelBridgeWorker and threaded into executeChannelBridgeTick), or documenting this as known behaviour so operators know to filter repeated messages.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/cli/src/channels/core/channelBridgeWorker.ts (1)
210-230:⚠️ Potential issue | 🟠 MajorPrevent
NaNcursor persistence in in-memory binding store.Line 218 and Line 229 can still persist
NaN(Math.trunc(NaN) => NaN), which breaks cursor monotonicity and downstreamafterSeqcomparisons. UsetoNonNegativeInt(...)here like the outbound path does.🛠️ Proposed fix
upsertBinding: async (binding) => { const key = bindingKey(binding); const existing = byKey.get(key); + const normalizedLastForwardedSeq = toNonNegativeInt(binding.lastForwardedSeq) ?? 0; const next: ChannelSessionBinding = { providerId: binding.providerId, conversationId: binding.conversationId, threadId: binding.threadId, sessionId: binding.sessionId, - lastForwardedSeq: Math.max(0, Math.trunc(binding.lastForwardedSeq)), + lastForwardedSeq: normalizedLastForwardedSeq, createdAtMs: existing?.createdAtMs ?? now(), updatedAtMs: now(), }; byKey.set(key, next); return next; }, updateLastForwardedSeq: async (ref, seq) => { const key = bindingKey(ref); const existing = byKey.get(key); if (!existing) return; - const nextSeq = Math.max(existing.lastForwardedSeq, Math.max(0, Math.trunc(seq))); + const parsedSeq = toNonNegativeInt(seq); + if (parsedSeq === null) return; + const nextSeq = Math.max(existing.lastForwardedSeq, parsedSeq); byKey.set(key, { ...existing, lastForwardedSeq: nextSeq, updatedAtMs: now(), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/core/channelBridgeWorker.ts` around lines 210 - 230, The in-memory binding store can persist NaN for lastForwardedSeq because Math.trunc(NaN) yields NaN; update the upsertBinding and updateLastForwardedSeq logic to sanitize seqs using the existing toNonNegativeInt helper (instead of Math.trunc/Math.max alone) when constructing ChannelSessionBinding and computing nextSeq; locate the upsertBinding and updateLastForwardedSeq functions (use bindingKey and ChannelSessionBinding identifiers) and replace the Math.trunc(...) / Math.max(...) usage with toNonNegativeInt(...) so lastForwardedSeq is always a non-negative integer.
🧹 Nitpick comments (1)
apps/cli/src/channels/core/channelBridgeWorker.ts (1)
390-413: Handle/attachresolver exceptions with an explicit user-facing reply.If
deps.resolveSessionIdOrPrefix(...)throws, the outer catch logs and drops the event flow, but the user gets no command-level failure message. Add a local try/catch in/attachto fail gracefully.🛠️ Proposed fix
- const resolved = await deps.resolveSessionIdOrPrefix(idOrPrefix); + let resolved: ResolveSessionIdResult; + try { + resolved = await deps.resolveSessionIdOrPrefix(idOrPrefix); + } catch (error) { + deps.onWarning?.('Failed to resolve session id/prefix for attach', error); + await replyToConversation( + adapter, + ref, + 'Unable to resolve that session right now. Please try again.', + ); + return true; + }As per coding guidelines, "Use proper error handling with try/catch blocks or Promise.catch()" and "Use graceful error handling with proper error messages, try-catch blocks with specific error logging".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/core/channelBridgeWorker.ts` around lines 390 - 413, The call to deps.resolveSessionIdOrPrefix(...) inside the /attach handler can throw and currently bubbles out; wrap that call in a local try/catch inside the /attach command flow (around the call that sets const resolved) so any thrown exception is caught, logged (use the existing logger), and a user-facing reply is sent via replyToConversation(adapter, ref, 'Failed to resolve session: <short message>'). Keep the existing branch logic for resolved.ok inside the try block and on catch return true after sending the error reply so the command fails gracefully.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cli/src/channels/core/channelBridgeWorker.ts`:
- Around line 728-736: The current shutdown logic builds adaptersToStop while
deduplicating by providerId (seenProviderIds) which prevents calling stop() on
multiple distinct ChannelBridgeAdapter instances that share the same providerId;
remove the providerId dedupe and instead push every adapter from params.adapters
(or dedupe by adapter instance identity if necessary) so each
ChannelBridgeAdapter instance gets its stop() invoked during shutdown; update
the loop that populates adaptersToStop (the code referencing params.adapters,
seenProviderIds, and adaptersToStop) to ensure stop() is called for every
adapter instance.
---
Duplicate comments:
In `@apps/cli/src/channels/core/channelBridgeWorker.ts`:
- Around line 210-230: The in-memory binding store can persist NaN for
lastForwardedSeq because Math.trunc(NaN) yields NaN; update the upsertBinding
and updateLastForwardedSeq logic to sanitize seqs using the existing
toNonNegativeInt helper (instead of Math.trunc/Math.max alone) when constructing
ChannelSessionBinding and computing nextSeq; locate the upsertBinding and
updateLastForwardedSeq functions (use bindingKey and ChannelSessionBinding
identifiers) and replace the Math.trunc(...) / Math.max(...) usage with
toNonNegativeInt(...) so lastForwardedSeq is always a non-negative integer.
---
Nitpick comments:
In `@apps/cli/src/channels/core/channelBridgeWorker.ts`:
- Around line 390-413: The call to deps.resolveSessionIdOrPrefix(...) inside the
/attach handler can throw and currently bubbles out; wrap that call in a local
try/catch inside the /attach command flow (around the call that sets const
resolved) so any thrown exception is caught, logged (use the existing logger),
and a user-facing reply is sent via replyToConversation(adapter, ref, 'Failed to
resolve session: <short message>'). Keep the existing branch logic for
resolved.ok inside the try block and on catch return true after sending the
error reply so the command fails gracefully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b2f9650-9b00-41c2-a582-67d717047ce3
📒 Files selected for processing (4)
apps/cli/src/channels/core/channelBridgeWorker.test.tsapps/cli/src/channels/core/channelBridgeWorker.tsdocs/README.mddocs/channel-bridge.md
✅ Files skipped from review due to trivial changes (1)
- apps/cli/src/channels/core/channelBridgeWorker.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/README.md
- docs/channel-bridge.md
| const adaptersToStop: ChannelBridgeAdapter[] = []; | ||
| const seenProviderIds = new Set<string>(); | ||
| for (const adapter of params.adapters) { | ||
| if (seenProviderIds.has(adapter.providerId)) { | ||
| continue; | ||
| } | ||
| seenProviderIds.add(adapter.providerId); | ||
| adaptersToStop.push(adapter); | ||
| } |
There was a problem hiding this comment.
Stop all unique adapter instances during shutdown (not just one per providerId).
Line 731 dedupes by providerId, which can skip stop() for distinct adapter instances that share the same provider id, leaving resources dangling.
🛠️ Proposed fix
- const adaptersToStop: ChannelBridgeAdapter[] = [];
- const seenProviderIds = new Set<string>();
+ const adaptersToStop: ChannelBridgeAdapter[] = [];
+ const seenAdapters = new Set<ChannelBridgeAdapter>();
for (const adapter of params.adapters) {
- if (seenProviderIds.has(adapter.providerId)) {
+ if (seenAdapters.has(adapter)) {
continue;
}
- seenProviderIds.add(adapter.providerId);
+ seenAdapters.add(adapter);
adaptersToStop.push(adapter);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/cli/src/channels/core/channelBridgeWorker.ts` around lines 728 - 736,
The current shutdown logic builds adaptersToStop while deduplicating by
providerId (seenProviderIds) which prevents calling stop() on multiple distinct
ChannelBridgeAdapter instances that share the same providerId; remove the
providerId dedupe and instead push every adapter from params.adapters (or dedupe
by adapter instance identity if necessary) so each ChannelBridgeAdapter instance
gets its stop() invoked during shutdown; update the loop that populates
adaptersToStop (the code referencing params.adapters, seenProviderIds, and
adaptersToStop) to ensure stop() is called for every adapter instance.
1 participant
Summary
This PR introduces a provider-agnostic channel bridge core for bi-directional session messaging.
It adds the reusable bridge worker and adapter contract, but intentionally does not include any provider-
specific adapter wiring (Telegram/Discord/etc). This keeps the core isolated and reviewable.
What’s Included
apps/cli/src/channels/core/channelBridgeWorker.tspullInboundMessages,sendMessage, optionalstop)/sessions/attach <session-id-or-prefix>/session/detachlastForwardedSeq)Tests
apps/cli/src/channels/core/channelBridgeWorker.test.tsOut of Scope (Intentional)
These are added in the follow-up stacked PR.
Summary by CodeRabbit
New Features
Tests
Documentation