feat(cli): add telegram channel adapter, scoped bridge config, and diagnostics#110
Draft
cdenneen wants to merge 4 commits intohappier-dev:devfrom
Draft
feat(cli): add telegram channel adapter, scoped bridge config, and diagnostics#110cdenneen wants to merge 4 commits intohappier-dev:devfrom
cdenneen wants to merge 4 commits intohappier-dev:devfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (33)
WalkthroughAdds a Channel Bridge (Telegram-first): bridge worker, Telegram adapter (polling/webhook), KV-backed server persistence, multi-level config resolution, CLI commands, daemon integration, validation helpers, tests, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Telegram as Telegram (API/Webhook)
participant Adapter as Telegram Adapter
participant Worker as Bridge Worker
participant Store as Binding Store
participant Session as Happier Session
participant Agent as AI Agent
rect rgba(100, 150, 200, 0.5)
note over Telegram,Agent: Inbound Message Flow
Telegram->>Adapter: Update (message in chat/topic)
Adapter->>Adapter: Parse & filter
Adapter->>Worker: Pull inbound messages
Worker->>Store: Get binding (provider,chatId,threadId)
Store-->>Worker: ChannelSessionBinding
Worker->>Session: Send user message to sessionId
Session->>Agent: Process user input
Agent-->>Session: Generate reply
end
rect rgba(150, 200, 100, 0.5)
note over Telegram,Agent: Outbound Message Flow
Worker->>Session: Fetch agent messages after lastForwardedSeq
Session-->>Worker: Agent messages
Worker->>Store: Update lastForwardedSeq
Store-->>Worker: Updated binding
Worker->>Adapter: Send message to conversation
Adapter->>Telegram: API sendMessage
Telegram-->>Adapter: Response
end
sequenceDiagram
participant CLI as CLI/Env
participant Config as Config Resolver
participant Settings as Local Settings
participant KV as Server KV
participant Runtime as Runtime Config
rect rgba(200, 150, 100, 0.5)
note over CLI,Runtime: Configuration resolution & overlay
Config->>CLI: Read env vars (HAPPIER_TELEGRAM_*)
Config->>Settings: Load scoped settings.json
Config->>KV: Fetch server KV record (if server/account)
KV-->>Config: Server record (non-secret)
Config->>Config: Overlay KV into scoped settings, merge secrets from local
Config->>Runtime: Merge env > account > server > global into final runtime config
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
cdenneen
changed the title
Contributor
Greptile SummaryThis PR adds a complete Telegram channel bridge on top of the provider-agnostic bridge core, including a polling + optional webhook relay adapter, account-scoped bridge configuration persisted across both local Key highlights from this review cycle:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| apps/cli/src/channels/startChannelBridgeWorker.ts | Core wiring for the channel bridge runtime. Contains two new issues: listSessions is limited to 100 sessions (users with larger session histories will have unreachable sessions), and there is no warning logged when webhookEnabled=true but webhookSecret is empty (silent polling fallback). Fallback-to-polling logic on relay startup failure and LRU-bounded session context cache look correct. |
| apps/cli/src/channels/telegram/telegramAdapter.ts | Telegram polling and webhook adapter. Long-polling (timeout: 25) is correctly configured. Stable-ID webhook queue drain prevents loss of in-flight updates. Self-message filtering works correctly after ensureSelfIdentity resolves. No new issues found. |
| apps/cli/src/channels/telegram/telegramWebhookRelay.ts | Fastify-based webhook relay. timingSafeEqual-backed header token comparison is correctly implemented. Path and header secrets are intentionally unified with a maintainer comment. Port and host validation are solid. No new issues found. |
| apps/cli/src/channels/channelBindingStore.server.ts | Server-backed KV binding store with optimistic-write retry. changed semantics prevent no-op writes. Conflict-payload decode errors are now guarded with a fallback to an empty bindings document. No new issues found. |
| apps/cli/src/ui/doctor.ts | Doctor diagnostics command with channel-bridge health checks. Settings are read once and reused. Stopped daemon is no longer marked as a critical failure. webhookHostForValidation and webhookPortForValidation still derive from raw env/config rather than from the already-resolved runtimeBridge values, which may cause false-positive critical failures when defaults are relied upon. |
| apps/cli/src/cli/commands/bridge.ts | CLI happier bridge command group. Local settings are now written before KV sync in both set and clear paths, preventing partial-failure state drift. No new issues found. |
| apps/cli/src/channels/channelBridgeAccountConfig.ts | Scoped Telegram bridge config helpers (upsert/remove/split). webhook.secret is no longer deleted during non-secret webhook field updates. splitScopedTelegramBridgeUpdate correctly partitions secrets from shared fields. No new issues found. |
| apps/cli/src/channels/channelBridgeConfig.ts | Runtime config resolution with layered env/server-KV/settings precedence. Default values (127.0.0.1 host, 8787 port) are applied correctly. mergeRecords shallow-merge is safe because webhook and secrets sub-trees are extracted and merged independently. No new issues found. |
| apps/cli/src/daemon/startDaemon.ts | Daemon startup and shutdown. Channel bridge worker is started during daemon init and stopped during shutdown with a configurable timeout (HAPPIER_DAEMON_CHANNEL_BRIDGE_STOP_TIMEOUT_MS, default 5 s). Timeout handling and best-effort teardown look correct. No new issues found. |
| apps/cli/src/channels/core/channelBridgeWorker.ts | Provider-agnostic bridge worker. lastForwardedSeq is now persisted incrementally per processed row, preventing duplicate delivery on partial batch failure. No new issues found. |
Sequence Diagram
sequenceDiagram
participant User as Telegram User
participant Relay as TelegramWebhookRelay (Fastify)
participant Adapter as TelegramAdapter
participant Worker as ChannelBridgeWorker
participant KV as Server KV (BindingStore)
participant Session as Happier Session
Note over Relay,Adapter: Webhook mode (optional)
User->>Relay: POST /telegram/webhook/{secret}
Relay->>Relay: Validate X-Telegram-Bot-Api-Secret-Token (timingSafeEqual)
Relay->>Adapter: enqueueWebhookUpdate(body)
Adapter->>Adapter: Push to queuedWebhookUpdates (stable ID)
Note over Worker: Tick loop (tickMs interval)
Worker->>Adapter: pullInboundMessages()
Adapter->>Adapter: Drain queue by stable IDs (webhook) or getUpdates (polling)
Adapter-->>Worker: ChannelBridgeInboundMessage[]
Worker->>KV: getBinding(conversationRef)
alt Binding found
Worker->>Session: forwardUserMessage(text)
Worker->>KV: updateLastForwardedSeq (incremental)
else No binding — slash command
Worker->>Adapter: sendMessage(/sessions list or /attach response)
Worker->>KV: upsertBinding (optimistic write + retry)
end
Session-->>Worker: fetchAgentMessagesAfterSeq(lastForwardedSeq)
Worker->>Adapter: sendMessage(agentText)
Worker->>KV: updateLastForwardedSeq
Last reviewed commit: f5567b3
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (6)
apps/cli/src/ui/doctor.test.ts (1)
97-109: Avoid exact copy-pinning for webhook issue assertions.This test currently hard-codes full user-facing messages. Prefer asserting stable substrings/field keys (e.g.,
webhook.secret,required when webhook.enabled=true) to reduce brittle failures on wording changes.Based on learnings, "Avoid brittle 'content policing' tests that pin ... exact ... user-facing copy ... Prefer stable identifiers ... and key substrings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/ui/doctor.test.ts` around lines 97 - 109, The test in apps/cli/src/ui/doctor.test.ts pins full user-facing messages from collectMissingRequiredWebhookFields; change it to assert stable identifiers/substrings instead of exact copy-pinned strings — for example, replace the toEqual assertion with assertions that the returned array includes entries containing 'webhook.secret', 'webhook.host', 'webhook.port' and the phrase 'required when webhook.enabled=true' (use expect.arrayContaining combined with expect.stringContaining or map the result to keys and assert those keys exist) so wording changes won't break the test.apps/cli/src/channels/telegram/telegramWebhookRelay.test.ts (1)
6-35: Add negative-path coverage for startup validation.Current tests only validate the happy path. Please add cases for invalid secret path token and out-of-range port so relay startup validation is locked in.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/telegram/telegramWebhookRelay.test.ts` around lines 6 - 35, Add two new tests in telegramWebhookRelay.test.ts that assert startup validation errors from startTelegramWebhookRelay: one that calls startTelegramWebhookRelay with an invalid secretPathToken (e.g., an empty string or a token containing a slash like '/bad-token') and expects the promise to reject/throw with a validation error, and another that calls startTelegramWebhookRelay with an out-of-range port (e.g., -1 or 70000) and expects rejection/throw; use async/await with expect(...).rejects or try/catch to assert the error and ensure you only call relay.stop() if the relay was successfully created. Reference startTelegramWebhookRelay, relay.port and relay.path when structuring the tests.apps/cli/src/cli/commands/bridge.test.ts (1)
46-70: Consolidate duplicatebeforeEachhooks into a single hook.The static analysis correctly identifies two separate
beforeEachhooks. While both will execute before each test, consolidating them improves readability and maintainability.♻️ Proposed consolidation
- beforeEach(() => { + beforeEach(async () => { vi.resetModules(); vi.clearAllMocks(); readCredentialsMock.mockResolvedValue({ token: 'token.jwt' }); decodeJwtPayloadMock.mockReturnValue({ sub: 'acct_123' }); readSettingsMock.mockResolvedValue({}); checkDaemonMock.mockResolvedValue(false); updateSettingsMock.mockImplementation(async (updater: (current: unknown) => Promise<unknown> | unknown) => { await updater({}); }); createKvClientMock.mockReturnValue({ get: vi.fn(), mutate: vi.fn() }); upsertKvConfigMock.mockResolvedValue(undefined); clearKvConfigMock.mockResolvedValue(undefined); - }); - beforeEach(async () => { homeDir = await mkdtemp(join(tmpdir(), 'happier-bridge-cmd-')); process.env.HAPPIER_HOME_DIR = homeDir; process.env.HAPPIER_SERVER_URL = 'http://127.0.0.1:3005'; process.env.HAPPIER_WEBAPP_URL = 'http://127.0.0.1:3006'; reloadConfiguration(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/cli/commands/bridge.test.ts` around lines 46 - 70, There are two beforeEach hooks; consolidate them into a single beforeEach that runs all setup steps: keep the mock resets and mockResolvedValue/mockReturnValue/setup implementations (readCredentialsMock, decodeJwtPayloadMock, readSettingsMock, checkDaemonMock, updateSettingsMock, createKvClientMock, upsertKvConfigMock, clearKvConfigMock) and also the async temp-dir setup (mkdtemp -> homeDir), environment variable assignments (process.env.HAPPIER_HOME_DIR, HAPPIER_SERVER_URL, HAPPIER_WEBAPP_URL) and reloadConfiguration() in the same hook so both synchronous mocks and async filesystem/env setup run together before each test; update any async handling so the combined beforeEach is declared async and awaits mkdtemp and any mock async initializers.apps/cli/src/channels/core/channelBridgeWorker.ts (1)
1-2: Add a file-level JSDoc header for worker lifecycle/ownership boundariesGiven this file contains core runtime orchestration, a top-level contract note will make maintenance and future adapters safer.
As per coding guidelines, "Include comprehensive JSDoc comments as file header comments explaining file responsibilities".
🤖 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 - 2, Add a file-level JSDoc header to channelBridgeWorker.ts above the imports that documents the file’s responsibility (core runtime orchestration for channel bridge workers), lifecycle boundaries (when and how startSingleFlightIntervalLoop is started/stopped), public API/handles (SingleFlightIntervalLoopHandle and any exported functions), ownership/maintainers, and key invariants or adapter constraints callers must respect; keep it concise but specific so future maintainers know contract expectations and to update the header when behavior or ownership changes.apps/cli/src/channels/channelBridgeAccountConfig.ts (1)
1-1: Add a file-level JSDoc header describing scoped config responsibilitiesThis file is a central config mutator/normalizer and would benefit from a concise top-level contract note.
As per coding guidelines, "Include comprehensive JSDoc comments as file header comments explaining file responsibilities".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/channelBridgeAccountConfig.ts` at line 1, Add a concise file-level JSDoc header at the top of this module describing that this file is the scoped config mutator/normalizer for channel bridge account settings: state its responsibilities (normalizing, validating, and merging scoped account config), the expected input shape (e.g., RecordLike), key exported utilities or functions provided by the module (named exports you see in this file), any side-effects (mutations, I/O, or global config changes), and the contract for callers (what guarantees normalized config will have). Keep it brief (2–4 sentences) and place it above the existing declarations so future readers understand the module's purpose and usage.apps/cli/src/channels/channelBridgeServerKv.ts (1)
1-6: Add a file-level JSDoc header for this KV moduleThis module defines cross-cutting persistence contracts and should have a top-level responsibility header for maintainability.
As per coding guidelines, "Include comprehensive JSDoc comments as file header comments explaining file responsibilities".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/channelBridgeServerKv.ts` around lines 1 - 6, Add a top-level JSDoc header to this KV module describing its responsibility as the cross-cutting persistence contract for channel bridge state, list the main exports/types it provides (e.g., ScopedTelegramBridgeUpdate or other KV interfaces), describe intended usage patterns (what data is stored, retention/consistency expectations, and who should call these APIs), and note any invariants or side-effects (e.g., synchronous vs async, error handling expectations). Place the comment at the very top of channelBridgeServerKv.ts so future readers immediately see the module purpose, public surface, and operational guidance.
🤖 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/channelBindingStore.server.ts`:
- Around line 101-130: The helper withOptimisticWrite currently always attempts
a KV write even when the operation produced no changes; modify it to compare
op.nextBindings with the loaded current.bindings (use deep equality or a
deterministic canonical comparison of ChannelSessionBinding arrays) and, if
identical, skip writeChannelBridgeBindingsToKv and simply return op.result (also
avoid updating version/cache), so no-op calls from updateLastForwardedSeq and
removeBinding won't bump the KV version; apply the same no-op-skip logic to the
other similar usage at the 169-195 region.
In `@apps/cli/src/channels/channelBridgeAccountConfig.ts`:
- Around line 127-192: readScopedTelegramBridgeConfig currently omits tickMs
from the returned normalized config causing persistence/round-trip bugs; update
the function to read tickMs from the account scope (e.g., accountScope?.tickMs
or telegram.tickMs depending on where it's stored) and, if it's a finite number,
set normalized.tickMs (use Math.trunc or Number.isFinite to normalize to an
integer) before returning normalized so the persisted tickMs is preserved by the
scoped read API.
- Around line 215-217: Assigning params.update.allowedChatIds to
telegram.allowedChatIds keeps a reference to caller-owned state; instead set
telegram.allowedChatIds to a shallow copy (e.g., Array.from(...) or spread copy)
when Array.isArray(params.update.allowedChatIds) is true so external mutations
won't affect the stored config; update the assignment in
channelBridgeAccountConfig.ts where telegram.allowedChatIds is set from
params.update.allowedChatIds.
In `@apps/cli/src/channels/channelBridgeConfig.test.ts`:
- Line 79: The test "applies env overrides over settings values" mixes valid env
overrides with an intentionally invalid port string '8_877'
(HAPPIER_TELEGRAM_WEBHOOK_PORT), which parseStrictInteger rejects so webhookPort
falls back to settings; either replace '8_877' with a valid numeric string like
'8877' to assert an actual environment override of webhookPort, or split/rename
the test into two: one that verifies env overrides (use '8877') and a separate
one that verifies fallback behavior when parseStrictInteger returns null (use
'8_877' and assert settings value is used). Ensure references to
HAPPIER_TELEGRAM_WEBHOOK_PORT, parseStrictInteger, and webhookPort are updated
accordingly.
In `@apps/cli/src/channels/channelBridgeServerKv.ts`:
- Around line 341-351: The catch block currently returns record: null but
preserves row.version, which lets later upserts merge from an empty baseline;
instead, when decodeBase64ToJson/parseTelegramConfigRecord fails, do not keep
the existing version—either rethrow the error or return a sentinel that prevents
merge (e.g. return { record: null, version: undefined }); update the catch in
the parse block around
parseTelegramConfigRecord(decodeBase64ToJson(row.valueBase64)) and apply the
same change to the similar catch at lines 426-460 so invalid payloads cannot be
used as a valid baseline for upsert/merge.
- Around line 279-289: The function decodeChannelBridgeBindingsDocFromBase64
currently swallows decode/parsing failures and returns an empty {
schemaVersion:1, bindings:[] }, which can hide corrupted payloads; change it to
fail fast: when decodeBase64ToJson throws or parseBindingsDocument returns
null/undefined, throw a descriptive Error (include the caught error message when
available and mention the offending base64 value or its truncated prefix) so
callers can detect and handle invalid payloads instead of silently treating it
as empty; update references to decodeChannelBridgeBindingsDocFromBase64 and
callers if they expect the previous empty fallback to handle errors.
In `@apps/cli/src/channels/core/channelBridgeWorker.ts`:
- Around line 67-69: The bindingKey function is collision-prone because it
conflates null threadId with the literal '-' and can be broken by IDs containing
the '::' delimiter; update bindingKey (and any consumers expecting its format)
to produce an unambiguous key—for example serialize the
ChannelBridgeConversationRef into a compact, reversible form such as
JSON.stringify or a deterministic base64 encoding of the concatenated parts,
ensure threadId null is represented explicitly (e.g. null vs empty string) and
avoid raw delimiter concatenation so providerId, conversationId, and threadId
cannot collide. Use the function name bindingKey and the type
ChannelBridgeConversationRef to locate and change the implementation and any
corresponding parsing logic.
- Around line 242-290: The current loop over params.adapters calls
adapter.pullInboundMessages() and processes each event without isolating
failures, so any thrown error aborts the entire tick; wrap the call to
pullInboundMessages() in a try/catch (log via params.deps.onWarning) and
continue to the next adapter on error, and also wrap the per-event processing
(including parseSlashCommand/handleCommand, params.store.getBinding,
params.deps.sendUserMessageToSession, and replyToConversation calls) in its own
try/catch so a failure for one event logs a warning and continues processing
remaining events and adapters; ensure you still await operations but never let
an exception bubble out of the adapter or event loops.
In `@apps/cli/src/channels/startChannelBridgeWorker.ts`:
- Around line 213-215: The debug log in startChannelBridgeWorker.ts currently
prints the full webhook URL including relayHandle.path (which contains the
secretPathToken); remove the secret from logs by logging only non-sensitive
parts (e.g., host and relayHandle.port) or by masking the token before logging.
Update the logger.debug call that references relayHandle.path so it omits or
redacts secretPathToken (or replace relayHandle.path with a maskedPath) while
keeping the rest of the message intact.
- Around line 254-261: The current stop() implementation may skip
relayHandle.stop() if await worker.stop() throws; change stop() (the exported
object's stop method that returns trigger and stop) to always attempt relay
cleanup by invoking relayHandle.stop() in a finally block (or equivalent
try/catch/finally) after calling worker.stop() (or if worker.stop() fails),
ensure relayHandle.stop() is awaited, and if both worker.stop() and
relayHandle.stop() error, surface the primary error or aggregate both so
resources aren't leaked while still propogating the failure.
In `@apps/cli/src/channels/telegram/telegramAdapter.ts`:
- Line 165: queuedWebhookUpdates is an unbounded array that can grow
indefinitely; fix by introducing a max capacity constant (e.g.,
MAX_QUEUED_WEBHOOKS) and enforce it wherever items are added to
queuedWebhookUpdates (drop oldest via shift() or reject new pushes and log/warn)
so the array never exceeds that capacity; update all places that push to
queuedWebhookUpdates (refer to the queuedWebhookUpdates symbol at its
declaration and the add/push sites around lines ~165 and ~190-192) to apply the
cap and record when items are dropped to aid monitoring.
- Around line 194-209: The code clears queuedWebhookUpdates and advances
updateOffset before calling parseUpdates, which can drop updates on transient
failures; fix by making local copies (e.g., const updates = webhookMode ?
queuedWebhookUpdates.slice() : await api.getUpdates(...)) and only mutate state
after parseUpdates succeeds: call parseUpdates(updates) first, and only if it
resolves, then clear queuedWebhookUpdates (queuedWebhookUpdates.length = 0) when
webhookMode is true and set updateOffset = maxUpdateId + 1 when
parseHighestUpdateOffset(updates) returns a value; ensure you still compute
maxUpdateId from the local updates variable so state changes happen
post-success.
In `@apps/cli/src/channels/telegram/telegramWebhookRelay.ts`:
- Around line 15-23: The webhook token and port need stricter validation: ensure
secretPathToken is URL-safe by encoding it (use encodeURIComponent on
secretPathToken when building path instead of interpolating raw) and keep the
existing empty-check; also validate requestedPort by rejecting values > 65535
(throw an Error if params.port is outside 0–65535) rather than silently
clamping, and continue to use the validated host and port when creating the path
and calling listen; update references to secretPathToken, path, requestedPort
(and params.port) so the encoded token and validated port are used everywhere.
In `@apps/cli/src/daemon/startDaemon.ts`:
- Around line 1382-1384: Make stopping the channel bridge worker best-effort by
wrapping the await channelBridgeWorker.stop() call in a try/catch so errors do
not abort the rest of the daemon shutdown/cleanup. Specifically, around the
channelBridgeWorker.stop() invocation (referencing the channelBridgeWorker
object and its stop() method), catch any thrown error, log the failure with the
existing daemon logger (e.g., processLogger or logger) including the error
details, and then continue with the remaining cleanup sequence.
- Around line 1197-1235: The three logger.warn calls in this startup block
(after readSettings.catch, inside the catch around
readChannelBridgeTelegramConfigFromKv, and the .catch on
startChannelBridgeFromEnv) currently log raw error objects which may contain
sensitive transport/token metadata; replace each logger.warn(..., error) with a
sanitized message that only includes a non-sensitive string (e.g. derive a
safeErrorMessage via (error && typeof error.message === 'string') ?
error.message : String(error) or error?.name + ': ' + String(error) and do not
include stack/headers/objects), keeping the original descriptive context
strings; update the warnings in the readSettings.catch, the readChannelBridge...
catch, and the startChannelBridgeFromEnv.catch accordingly (functions
referenced: readSettings, readChannelBridgeTelegramConfigFromKv,
startChannelBridgeFromEnv, createAxiosChannelBridgeKvClient,
overlayServerKvTelegramConfigInSettings).
In `@apps/cli/src/ui/doctor.ts`:
- Around line 316-320: The current webhookPortRaw parsing silently accepts
malformed numeric strings (e.g. "8080abc"); update the string branch to enforce
a digits-only check before parsing: for webhookPortRaw keep the number branch
using Math.trunc(webhookConfig.port) but for the string branch first test
webhookConfig.port against /^\d+$/ and only then call
Number.parseInt(webhookConfig.port, 10); otherwise set null. Apply the same
strict-digit validation change to the other similar port parsing block
referenced (lines 374–378) so both places only accept pure digit strings before
parsing.
- Around line 394-396: The catch block that currently swallows exceptions during
the channel-bridge diagnostics should mark the run as failed and surface a
user-facing error: inside the catch for the channel-bridge check (in the same
function where hasCriticalFailures is used) set hasCriticalFailures = true and
emit a user-visible error message using the same logger used elsewhere in this
function (e.g., the function's logger/processLogger or UI error helper) so
failures are not silently ignored and match existing error-handling behavior.
- Around line 279-366: The diagnostics block in apps/cli/src/ui/doctor.ts uses
many unsafe casts (channelBridgeRoot, byServerId, serverScope, byAccountId,
accountScope, providers, telegram, providerEntries) and silences errors with a
bare catch; replace those ad-hoc (as any) casts with a small typed
extractor/validator function that defines interfaces for ChannelBridgeSettings,
ProviderConfig, WebhookConfig, Secrets etc., uses runtime guards (typeof checks,
Array.isArray) to narrow unknown → typed objects, and returns typed values used
by the existing logic (token, webhookConfig, allowedChatIds, webhookPortRaw,
webhookIssues via collectMissingRequiredWebhookFields, and maskValue). Also
remove the silent catch and replace it with explicit error handling that logs
the error (use the same logger/console pattern used nearby) so failures in
extraction are visible and set hasCriticalFailures accordingly. Ensure
providerEntries is derived from the validated typed providers object rather than
from any casts.
In `@docs/channel-bridge-uat.md`:
- Line 85: Update the test/verification step that currently reads "If using
webhook mode, verify secret mismatch returns unauthorized." to reflect actual
relay behavior under path-token routing: change the expectation to "verify
secret mismatch results in a non-200 response (typically 404) or otherwise does
not forward the request" so the doc matches runtime behavior; locate the
sentence string "If using webhook mode, verify secret mismatch returns
unauthorized." in docs/channel-bridge-uat.md and replace it with the suggested
wording.
---
Nitpick comments:
In `@apps/cli/src/channels/channelBridgeAccountConfig.ts`:
- Line 1: Add a concise file-level JSDoc header at the top of this module
describing that this file is the scoped config mutator/normalizer for channel
bridge account settings: state its responsibilities (normalizing, validating,
and merging scoped account config), the expected input shape (e.g., RecordLike),
key exported utilities or functions provided by the module (named exports you
see in this file), any side-effects (mutations, I/O, or global config changes),
and the contract for callers (what guarantees normalized config will have). Keep
it brief (2–4 sentences) and place it above the existing declarations so future
readers understand the module's purpose and usage.
In `@apps/cli/src/channels/channelBridgeServerKv.ts`:
- Around line 1-6: Add a top-level JSDoc header to this KV module describing its
responsibility as the cross-cutting persistence contract for channel bridge
state, list the main exports/types it provides (e.g., ScopedTelegramBridgeUpdate
or other KV interfaces), describe intended usage patterns (what data is stored,
retention/consistency expectations, and who should call these APIs), and note
any invariants or side-effects (e.g., synchronous vs async, error handling
expectations). Place the comment at the very top of channelBridgeServerKv.ts so
future readers immediately see the module purpose, public surface, and
operational guidance.
In `@apps/cli/src/channels/core/channelBridgeWorker.ts`:
- Around line 1-2: Add a file-level JSDoc header to channelBridgeWorker.ts above
the imports that documents the file’s responsibility (core runtime orchestration
for channel bridge workers), lifecycle boundaries (when and how
startSingleFlightIntervalLoop is started/stopped), public API/handles
(SingleFlightIntervalLoopHandle and any exported functions),
ownership/maintainers, and key invariants or adapter constraints callers must
respect; keep it concise but specific so future maintainers know contract
expectations and to update the header when behavior or ownership changes.
In `@apps/cli/src/channels/telegram/telegramWebhookRelay.test.ts`:
- Around line 6-35: Add two new tests in telegramWebhookRelay.test.ts that
assert startup validation errors from startTelegramWebhookRelay: one that calls
startTelegramWebhookRelay with an invalid secretPathToken (e.g., an empty string
or a token containing a slash like '/bad-token') and expects the promise to
reject/throw with a validation error, and another that calls
startTelegramWebhookRelay with an out-of-range port (e.g., -1 or 70000) and
expects rejection/throw; use async/await with expect(...).rejects or try/catch
to assert the error and ensure you only call relay.stop() if the relay was
successfully created. Reference startTelegramWebhookRelay, relay.port and
relay.path when structuring the tests.
In `@apps/cli/src/cli/commands/bridge.test.ts`:
- Around line 46-70: There are two beforeEach hooks; consolidate them into a
single beforeEach that runs all setup steps: keep the mock resets and
mockResolvedValue/mockReturnValue/setup implementations (readCredentialsMock,
decodeJwtPayloadMock, readSettingsMock, checkDaemonMock, updateSettingsMock,
createKvClientMock, upsertKvConfigMock, clearKvConfigMock) and also the async
temp-dir setup (mkdtemp -> homeDir), environment variable assignments
(process.env.HAPPIER_HOME_DIR, HAPPIER_SERVER_URL, HAPPIER_WEBAPP_URL) and
reloadConfiguration() in the same hook so both synchronous mocks and async
filesystem/env setup run together before each test; update any async handling so
the combined beforeEach is declared async and awaits mkdtemp and any mock async
initializers.
In `@apps/cli/src/ui/doctor.test.ts`:
- Around line 97-109: The test in apps/cli/src/ui/doctor.test.ts pins full
user-facing messages from collectMissingRequiredWebhookFields; change it to
assert stable identifiers/substrings instead of exact copy-pinned strings — for
example, replace the toEqual assertion with assertions that the returned array
includes entries containing 'webhook.secret', 'webhook.host', 'webhook.port' and
the phrase 'required when webhook.enabled=true' (use expect.arrayContaining
combined with expect.stringContaining or map the result to keys and assert those
keys exist) so wording changes won't break the test.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17f0243c-9dd6-4b8d-b630-09d4bd5561eb
⛔ Files ignored due to path filters (3)
docs/assets/doctor-critical-summary.pngis excluded by!**/*.pngdocs/assets/doctor-missing-host-port-summary.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (29)
README.mdapps/cli/src/channels/channelBindingStore.server.test.tsapps/cli/src/channels/channelBindingStore.server.tsapps/cli/src/channels/channelBridgeAccountConfig.test.tsapps/cli/src/channels/channelBridgeAccountConfig.tsapps/cli/src/channels/channelBridgeConfig.test.tsapps/cli/src/channels/channelBridgeConfig.tsapps/cli/src/channels/channelBridgeServerConfigOverlay.test.tsapps/cli/src/channels/channelBridgeServerConfigOverlay.tsapps/cli/src/channels/channelBridgeServerKv.test.tsapps/cli/src/channels/channelBridgeServerKv.tsapps/cli/src/channels/core/channelBridgeWorker.test.tsapps/cli/src/channels/core/channelBridgeWorker.tsapps/cli/src/channels/startChannelBridgeWorker.test.tsapps/cli/src/channels/startChannelBridgeWorker.tsapps/cli/src/channels/telegram/telegramAdapter.test.tsapps/cli/src/channels/telegram/telegramAdapter.tsapps/cli/src/channels/telegram/telegramWebhookRelay.test.tsapps/cli/src/channels/telegram/telegramWebhookRelay.tsapps/cli/src/cli/commandRegistry.tsapps/cli/src/cli/commands/bridge.test.tsapps/cli/src/cli/commands/bridge.tsapps/cli/src/cli/dispatch.tsapps/cli/src/daemon/startDaemon.tsapps/cli/src/persistence.tsapps/cli/src/ui/doctor.test.tsapps/cli/src/ui/doctor.tsdocs/channel-bridge-uat.mddocs/telegram-channel-bridge.md
cdenneen
force-pushed
the
feat/telegram-channel-adapter
branch
from
8c60a3a to
9eb06ac
Compare
Author
|
@coderabbitai please re-review this PR after latest force-push (review-thread fixes and stack rebase onto #109 head). @greptile-apps please run a fresh pass too. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
apps/cli/src/channels/startChannelBridgeWorker.ts (1)
281-289: Ensure deps disposal still runs if relay shutdown fails.
defaultDepsHandle?.dispose()is currently skipped whenrelayHandle.stop()throws, leaving cache/state cleanup incomplete.Based on learnings: "Implement proper process lifecycle and cleanup handling".♻️ Cleanup ordering fix
return { trigger: worker.trigger, stop: async () => { + let primaryError: unknown = null; try { await worker.stop(); + } catch (error) { + primaryError = error; } finally { - if (relayHandle) { - await relayHandle.stop(); + try { + if (relayHandle) { + await relayHandle.stop(); + } + } catch (error) { + if (primaryError === null) { + primaryError = error; + } + } finally { + defaultDepsHandle?.dispose(); } - defaultDepsHandle?.dispose(); + } + if (primaryError !== null) { + throw primaryError; } }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/startChannelBridgeWorker.ts` around lines 281 - 289, The stop handler currently awaits relayHandle.stop() inside a finally which can throw and prevent defaultDepsHandle?.dispose() from running; update the stop implementation (the async stop closure that calls worker.stop()) so that cleanup always runs: call await worker.stop() in its try, then in the finally ensure relayHandle.stop() is invoked inside its own try/catch (or swallow/handle errors) so exceptions from relayHandle.stop() do not abort subsequent cleanup, and always call defaultDepsHandle?.dispose() after relay shutdown regardless of relay errors.
🤖 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/channelBindingStore.server.test.ts`:
- Around line 6-120: The tests use in-process test doubles
createInMemoryKvClient and createCountingKvClient instead of exercising the real
KV API; replace those helpers with the project's integration test fixture that
performs real API-backed KV calls (e.g., the shared fixture that returns an
actual ChannelBridgeKvClient connected to the test backend), update tests that
import or call createInMemoryKvClient/createCountingKvClient to instead obtain
the real client from the integration fixture, remove the in-memory helper
functions and any mutateCallCount logic, and ensure tests run against the real
backend client (using existing setup/teardown from the integration fixtures) so
they no longer rely on mock-driven behavior.
In `@apps/cli/src/channels/core/channelBridgeWorker.ts`:
- Around line 568-573: The shutdown currently uses Promise.all over
params.adapters which can reject early; replace that with Promise.allSettled (or
manually await each stop inside a loop) so every adapter.stop() is attempted
even if some reject, then collect any errors from the settled results and either
log them or throw an aggregated error; update the code where
params.adapters.map(async (adapter) => { if (typeof adapter.stop !== 'function')
return; await adapter.stop(); }) is used (in channelBridgeWorker.ts) to use
Promise.allSettled and handle rejected outcomes deterministically.
In `@apps/cli/src/channels/startChannelBridgeWorker.ts`:
- Around line 171-176: The onWarning handler currently logs raw error objects
(logger.warn in the onWarning callback), which can leak secrets; update the
handler used at lines around the onWarning definition to sanitize/normalize the
error before logging — e.g., implement or call a shared sanitizeError utility to
extract only safe fields (name, message, truncated stack) and redact known
sensitive keys (token, auth, cookie, session, apiKey) or stringify with a
redaction policy, then pass that sanitized object/string to logger.warn; apply
the same change to the other warning handler occurrences mentioned (around the
240-242 area) so all warning paths log only sanitized payloads.
In `@apps/cli/src/channels/telegram/telegramAdapter.test.ts`:
- Around line 7-31: The test currently uses mocked API stubs (the local "api"
object with getMe, getUpdates, sendMessage implemented via vi.fn) which violates
the guideline to use real Telegram integration fixtures; replace these vi.fn
mocks in telegramAdapter.test.ts (the "api" fixture and the repeated mocks at
the other occurrences) with real integration fixtures that perform actual API
calls or recorded wire fixtures provided by the project test helpers, ensuring
getMe(), getUpdates(), and sendMessage() use the real transport/fixture helpers
instead of vi.fn so the suite exercises real API behavior and avoids drift.
In `@apps/cli/src/channels/telegram/telegramWebhookRelay.ts`:
- Around line 14-15: The webhook handler is currently invoking the onUpdate
callback fire-and-forget which returns { ok: true } before onUpdate completes;
change the handler to await the onUpdate(update) call (wrap in try/catch) and
only send the success response after it completes, logging or returning an error
response when onUpdate throws so failed async handlers aren’t masked; look for
the onUpdate parameter and the HTTP request handler that sends the JSON response
(in the Telegram webhook relay implementation) and replace the fire-and-forget
invocation with an awaited call and proper error handling.
---
Nitpick comments:
In `@apps/cli/src/channels/startChannelBridgeWorker.ts`:
- Around line 281-289: The stop handler currently awaits relayHandle.stop()
inside a finally which can throw and prevent defaultDepsHandle?.dispose() from
running; update the stop implementation (the async stop closure that calls
worker.stop()) so that cleanup always runs: call await worker.stop() in its try,
then in the finally ensure relayHandle.stop() is invoked inside its own
try/catch (or swallow/handle errors) so exceptions from relayHandle.stop() do
not abort subsequent cleanup, and always call defaultDepsHandle?.dispose() after
relay shutdown regardless of relay errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 21104746-2621-42e1-bb00-3c5bdca7dc0c
📒 Files selected for processing (18)
apps/cli/src/channels/channelBindingStore.server.test.tsapps/cli/src/channels/channelBindingStore.server.tsapps/cli/src/channels/channelBridgeAccountConfig.test.tsapps/cli/src/channels/channelBridgeAccountConfig.tsapps/cli/src/channels/channelBridgeConfig.test.tsapps/cli/src/channels/channelBridgeServerKv.test.tsapps/cli/src/channels/channelBridgeServerKv.tsapps/cli/src/channels/core/channelBridgeWorker.test.tsapps/cli/src/channels/core/channelBridgeWorker.tsapps/cli/src/channels/startChannelBridgeWorker.tsapps/cli/src/channels/telegram/telegramAdapter.test.tsapps/cli/src/channels/telegram/telegramAdapter.tsapps/cli/src/channels/telegram/telegramWebhookRelay.test.tsapps/cli/src/channels/telegram/telegramWebhookRelay.tsapps/cli/src/daemon/startDaemon.tsapps/cli/src/ui/doctor.tsdocs/channel-bridge-uat.mddocs/telegram-channel-bridge.md
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/telegram-channel-bridge.md
- apps/cli/src/channels/channelBridgeConfig.test.ts
- apps/cli/src/channels/channelBridgeAccountConfig.test.ts
- apps/cli/src/channels/telegram/telegramWebhookRelay.test.ts
- apps/cli/src/channels/channelBridgeServerKv.test.ts
- docs/channel-bridge-uat.md
Author
|
Follow-up clarity pass — exact fix locations for resolved bot items after latest stack rebase ( Core carryover from #109 (
#110-specific hardening (
This is a reviewer navigation map so each resolved thread can be validated directly against the updated code paths. |
Contributor
Additional Comments (1)
When the settings dump is printed at line 267, these plaintext secrets are visible in the JSON output of the A dedicated bridge section (starting line 292) correctly uses function redactSettingsForDisplay(settings: SettingsForDisplay): SettingsForDisplay {
const redacted = JSON.parse(JSON.stringify(settings ?? {})) as SettingsForDisplay;
const redactedRecord = redacted as unknown as Record<string, unknown>;
if (Object.prototype.hasOwnProperty.call(redactedRecord, 'localEnvironmentVariables')) {
delete redactedRecord.localEnvironmentVariables;
}
// Redact channel bridge secrets so that botToken / webhookSecret
// are never emitted in plain text by the doctor command.
const cb = redactedRecord.channelBridge;
if (cb && typeof cb === 'object' && !Array.isArray(cb)) {
redactedRecord.channelBridge = '<redacted – use `happier bridge list` to inspect>';
}
return redacted;
} |
cdenneen
force-pushed
the
feat/telegram-channel-adapter
branch
from
9eb06ac to
562bee5
Compare
Author
|
@coderabbitai @greptile-apps follow-up fixes were pushed (commits 562bee5 and 1eead1d), and #110 remains stacked directly on #109 head; please re-review. |
|
The PR description images show:
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
Additional Comments (1)
Hoisting a single shared let settings: Awaited<ReturnType<typeof readSettings>> | null = null;
try {
settings = await readSettings();
} catch {
// ...
}
// then use `settings` in both the display block and the channel-bridge section |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
apps/cli/src/channels/core/channelBridgeWorker.test.ts (1)
573-575: Replace fixed sleeps with condition-based waits to reduce flakiness.These assertions currently depend on timing (
setTimeout(25/50ms)), which can intermittently fail on slower CI runners. ReusewaitFor(...)on observable conditions instead of fixed delays.Based on learnings: Tests must be deterministic and isolated with no shared global state and no ordering reliance.
Also applies to: 613-619
🤖 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 573 - 575, Replace the fixed setTimeout-based waits with condition-based waits: instead of awaiting new Promise(resolve => setTimeout(...)) then asserting sentToSession length, use a waitFor helper to poll until sentToSession has the expected length (e.g., waitFor(() => expect(sentToSession).toHaveLength(1))). Update both occurrences referenced (the block around sentToSession assertion at lines ~573–575 and the similar block around ~613–619) to import/use the test runner’s waitFor utility (from `@testing-library/react` or vitest) and wait on the observable condition rather than sleeping, ensuring tests become deterministic and resilient to CI slowness.apps/cli/src/channels/channelBridgeAccountConfig.ts (2)
1-1: Add a file-level JSDoc header describing module responsibility.This file exposes multiple public config APIs and normalization rules; a short header doc would make scope/precedence intent much easier to maintain.
As per coding guidelines: "Include comprehensive JSDoc comments as file header comments explaining file responsibilities".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/channelBridgeAccountConfig.ts` at line 1, Add a file-level JSDoc header at the top of this module that explains the file's responsibility (public config APIs and normalization rules), scope/precedence intent, and any important usage notes or side effects; place it above the existing type alias RecordLike and reference the primary exports (e.g., the exported config API functions and normalization helpers) so readers know what to expect and how to use the module.
258-264: Prune emptytelegram.webhookafter secret migration.When moving
webhook.secretintosecrets.webhookSecret, an emptytelegram.webhookobject can remain if it only containedsecret.Proposed cleanup
if (typeof params.update.webhookSecret === 'string') { secrets.webhookSecret = params.update.webhookSecret; const webhook = asRecord(telegram.webhook); - if (webhook && 'secret' in webhook) { - delete webhook.secret; + if (webhook) { + if ('secret' in webhook) { + delete webhook.secret; + } + if (isEmptyRecord(webhook)) { + delete telegram.webhook; + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/channelBridgeAccountConfig.ts` around lines 258 - 264, After migrating params.update.webhookSecret into secrets.webhookSecret and deleting webhook.secret on the local webhook object (in the block referencing params.update.webhookSecret, secrets.webhookSecret, telegram.webhook, asRecord and delete webhook.secret), add a check that if the resulting webhook object has no own enumerable keys (is empty) then remove telegram.webhook entirely (e.g., delete telegram.webhook or set it to undefined) so an empty object isn't left behind; ensure this runs immediately after the delete webhook.secret operation.
🤖 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/channelBridgeAccountConfig.ts`:
- Around line 287-304: The removal path in removeScopedTelegramBridgeConfig only
deletes providers.telegram but leaves accountScope.tickMs behind, which can
resurrect old polling intervals; update the removal logic (the block referencing
providers.telegram, accountScope, byAccountId, serverScope, byServerId) to also
delete accountScope.tickMs when removing the Telegram provider (e.g., add
"delete accountScope.tickMs" after "delete providers.telegram"), and keep the
existing empty-record cleanups (isEmptyRecord(accountScope) / byAccountId /
serverScope) so tickMs is removed whenever the Telegram config for that account
is deleted.
In `@apps/cli/src/channels/channelBridgeServerKv.test.ts`:
- Line 93: The test currently uses a broad cast "(config.record as any)" which
defeats type-safety; replace it by narrowing the type for config.record (or
casting to a targeted Partial type) and assert via the concrete property path
(config.record.telegram?.botToken) so the assertion keeps type checking.
Specifically, update the assertion around config.record and telegram.botToken in
channelBridgeServerKv.test.ts to cast config.record to a precise type (e.g.,
Partial<YourRecordType> or the existing ChannelRecord type) or use optional
chaining, rather than "as any", so the test remains type-safe while still
checking that telegram.botToken is undefined.
In `@apps/cli/src/channels/channelBridgeServerKv.ts`:
- Around line 175-202: The loop that decodes rawBindings in
channelBridgeServerKv.ts (variables rawBindings, asRecord, and bindings)
currently silently skips malformed rows which can cause partial documents and
data loss; instead, validate each row and fail fast by throwing a descriptive
error when asRecord(row) returns falsy or when required fields providerId,
conversationId, or sessionId are missing/empty or numeric fields
(lastForwardedSeq, createdAtMs, updatedAtMs) are invalid, including the
offending row data in the error message so the caller can surface and fix the
corrupt input rather than continuing and producing a partial bindings array.
In `@apps/cli/src/channels/startChannelBridgeWorker.ts`:
- Around line 218-247: The adapter is created with webhookMode: true before
startTelegramWebhookRelay, so if relay startup fails inbound delivery stops;
change the catch block to fall back to a polling adapter: either toggle the
existing telegramAdapter into polling mode (if it exposes a method/property) or
create a new adapter via createTelegramChannelAdapter({ botToken,
allowedChatIds, requireTopics, webhookMode: false }) and replace the entry in
adapters (and ensure enqueueWebhookUpdate binding is preserved if needed), then
log the fallback; reference createTelegramChannelAdapter, telegramAdapter,
startTelegramWebhookRelay, webhookMode, enqueueWebhookUpdate and adapters when
making the change.
In `@apps/cli/src/cli/commands/bridge.test.ts`:
- Around line 46-70: Combine the two beforeEach blocks into a single async
beforeEach so setup ordering is explicit: first call vi.resetModules() and
vi.clearAllMocks(), then configure all mocks (readCredentialsMock,
decodeJwtPayloadMock, readSettingsMock, checkDaemonMock, updateSettingsMock
implementation, createKvClientMock, upsertKvConfigMock, clearKvConfigMock), then
create the temp homeDir via mkdtemp, set process.env.HAPPIER_HOME_DIR /
HAPPIER_SERVER_URL / HAPPIER_WEBAPP_URL, and finally call reloadConfiguration();
ensure the updateSettingsMock implementation and other mock setups remain
unchanged and that the async mkdtemp/await happens after mocks are configured.
In `@apps/cli/src/cli/commands/bridge.ts`:
- Around line 78-79: Update the CLI usage strings in
apps/cli/src/cli/commands/bridge.ts so the help lines include the supported
webhook flags parsed by cmdTelegramSet (e.g., webhook-related flags like
--webhook-url, --webhook-cert, or whatever webhook flags cmdTelegramSet reads);
locate the usage declarations near the top examples (the line with "happier
bridge telegram set --bot-token ...") and the repeated block around the later
example (the area noted as also applies to 198-201) and append the webhook flag
options to those usage lines so the displayed help matches the flags parsed by
cmdTelegramSet.
- Around line 257-264: The code uses a broad "as any" to silence types inside
the settings mutation—remove the cast and make the mutation return the correct
Settings type by aligning upsertScopedTelegramBridgeConfig's return/type
signatures or by mapping its result to the expected Settings shape before
returning; update the call site in updateSettings (the lambda passed to
updateSettings that currently calls upsertScopedTelegramBridgeConfig with
serverId/accountId and split.localUpdate) to return a properly typed Settings
object (or cast a narrow, justified type) instead of "as any"; apply the same
fix for the other similar updateSettings/upsertScopedTelegramBridgeConfig usage
in this file so both mutation paths preserve compile-time type safety.
In `@apps/cli/src/daemon/startDaemon.ts`:
- Around line 1197-1245: The channel bridge startup is currently gated on
machine registration and therefore never runs if registration fails; refactor so
the bridge startup (the block that reads settings via readSettings(), optionally
fetches KV using createAxiosChannelBridgeKvClient() and
readChannelBridgeTelegramConfigFromKv(), computes channelBridgeRuntimeSettings
with overlayServerKvTelegramConfigInSettings(), and finally calls
startChannelBridgeFromEnv()) runs independently as a best-effort operation
regardless of machine-registration outcome: move or duplicate this block out of
the registration-success-only path, ensure it only attempts server KV fetch when
serverId and accountId are available (and silently continues on any errors using
existing logger.warn + serializeAxiosErrorForLog behavior), and always call
startChannelBridgeFromEnv() (catching/logging errors and returning null) so
bridge startup is attempted even if registration failed.
In `@apps/cli/src/ui/doctor.ts`:
- Around line 292-399: The doctor currently inspects account-scoped values
derived from readSettings() (see settingsRecord -> accountScope -> providers ->
telegram) but must instead use the runtime-resolved channel-bridge configuration
(respecting env and server KV overlays) when evaluating Telegram and other
providers; replace the usage of providers/telegram derived from accountScope
with the runtime-resolved providers for the active server/account (obtain this
by calling the existing runtime settings resolver or by merging precedence: env
> server KV > local settings based on configuration.activeServerId and
readSettings()), then feed that resolved providers object into the same checks
that use parseStrictWebhookPort, collectMissingRequiredWebhookFields and
maskValue so hasCriticalFailures reflects actual runtime behavior rather than
raw local scoped settings.
In `@README.md`:
- Around line 196-200: Replace the ambiguous section header "Channel
Integrations (A/1)" with a clear title like "Channel Integrations" and change
the code-formatted doc path to a proper Markdown link by updating the bullet so
the Telegram doc is linked as [Telegram bi-directional bridge
setup](docs/telegram-channel-bridge.md); ensure the header and the link text
match (e.g., "Telegram bi-directional bridge setup (BotFather, topics/DM
mapping, optional webhook relay)") and that the link points to
docs/telegram-channel-bridge.md so it renders clickable in the README.
---
Nitpick comments:
In `@apps/cli/src/channels/channelBridgeAccountConfig.ts`:
- Line 1: Add a file-level JSDoc header at the top of this module that explains
the file's responsibility (public config APIs and normalization rules),
scope/precedence intent, and any important usage notes or side effects; place it
above the existing type alias RecordLike and reference the primary exports
(e.g., the exported config API functions and normalization helpers) so readers
know what to expect and how to use the module.
- Around line 258-264: After migrating params.update.webhookSecret into
secrets.webhookSecret and deleting webhook.secret on the local webhook object
(in the block referencing params.update.webhookSecret, secrets.webhookSecret,
telegram.webhook, asRecord and delete webhook.secret), add a check that if the
resulting webhook object has no own enumerable keys (is empty) then remove
telegram.webhook entirely (e.g., delete telegram.webhook or set it to undefined)
so an empty object isn't left behind; ensure this runs immediately after the
delete webhook.secret operation.
In `@apps/cli/src/channels/core/channelBridgeWorker.test.ts`:
- Around line 573-575: Replace the fixed setTimeout-based waits with
condition-based waits: instead of awaiting new Promise(resolve =>
setTimeout(...)) then asserting sentToSession length, use a waitFor helper to
poll until sentToSession has the expected length (e.g., waitFor(() =>
expect(sentToSession).toHaveLength(1))). Update both occurrences referenced (the
block around sentToSession assertion at lines ~573–575 and the similar block
around ~613–619) to import/use the test runner’s waitFor utility (from
`@testing-library/react` or vitest) and wait on the observable condition rather
than sleeping, ensuring tests become deterministic and resilient to CI slowness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b402c91c-6319-4398-ac54-dadc223aec8d
⛔ Files ignored due to path filters (3)
docs/assets/doctor-critical-summary.pngis excluded by!**/*.pngdocs/assets/doctor-missing-host-port-summary.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (29)
README.mdapps/cli/src/channels/channelBindingStore.server.test.tsapps/cli/src/channels/channelBindingStore.server.tsapps/cli/src/channels/channelBridgeAccountConfig.test.tsapps/cli/src/channels/channelBridgeAccountConfig.tsapps/cli/src/channels/channelBridgeConfig.test.tsapps/cli/src/channels/channelBridgeConfig.tsapps/cli/src/channels/channelBridgeServerConfigOverlay.test.tsapps/cli/src/channels/channelBridgeServerConfigOverlay.tsapps/cli/src/channels/channelBridgeServerKv.test.tsapps/cli/src/channels/channelBridgeServerKv.tsapps/cli/src/channels/core/channelBridgeWorker.test.tsapps/cli/src/channels/core/channelBridgeWorker.tsapps/cli/src/channels/startChannelBridgeWorker.test.tsapps/cli/src/channels/startChannelBridgeWorker.tsapps/cli/src/channels/telegram/telegramAdapter.test.tsapps/cli/src/channels/telegram/telegramAdapter.tsapps/cli/src/channels/telegram/telegramWebhookRelay.test.tsapps/cli/src/channels/telegram/telegramWebhookRelay.tsapps/cli/src/cli/commandRegistry.tsapps/cli/src/cli/commands/bridge.test.tsapps/cli/src/cli/commands/bridge.tsapps/cli/src/cli/dispatch.tsapps/cli/src/daemon/startDaemon.tsapps/cli/src/persistence.tsapps/cli/src/ui/doctor.test.tsapps/cli/src/ui/doctor.tsdocs/channel-bridge-uat.mddocs/telegram-channel-bridge.md
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/cli/src/channels/channelBridgeAccountConfig.test.ts
- apps/cli/src/channels/channelBridgeServerConfigOverlay.test.ts
- docs/telegram-channel-bridge.md
- apps/cli/src/channels/channelBindingStore.server.ts
- apps/cli/src/ui/doctor.test.ts
- apps/cli/src/channels/channelBindingStore.server.test.ts
- apps/cli/src/persistence.ts
- apps/cli/src/channels/startChannelBridgeWorker.test.ts
- apps/cli/src/channels/channelBridgeConfig.test.ts
- docs/channel-bridge-uat.md
- apps/cli/src/channels/telegram/telegramWebhookRelay.test.ts
cdenneen
force-pushed
the
feat/telegram-channel-adapter
branch
from
1eead1d to
0aceec6
Compare
cdenneen
added a commit
to cdenneen/happier-dev
that referenced
this pull request
Mar 4, 2026
Author
|
@coderabbitai review\n@greptile-ai review\n\nFollow-up fixes are pushed in f448629 and f2879a4, and all previously open review threads have been addressed and resolved. |
|
The PR description images show:
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/cli/src/daemon/startDaemon.ts (1)
1153-1245:⚠️ Potential issue | 🟠 MajorDecouple channel bridge startup from machine registration success.
startChannelBridgeFromEnv(...)is still inside the success path afterensureMachineRegistered(...). If registration fails (temporary backend outage, timeout), the daemon keeps running but bridge startup is skipped entirely until restart.Suggested refactor direction
+const startChannelBridgeBestEffort = async (): Promise<ChannelBridgeRuntimeHandle | null> => { + const bridgeSettings = await readSettings().catch(() => null); + // optional KV overlay when serverId/accountId are available + return await startChannelBridgeFromEnv({ credentials, settings: bridgeSettings, ... }); +}; + +channelBridgeWorker = await startChannelBridgeBestEffort().catch(() => null); + void (async () => { try { const ensured = await ensureMachineRegistered(...); // machine sync wiring only - channelBridgeWorker = await startChannelBridgeFromEnv(...); } catch (error) { logger.warn('... continuing without machine sync ...'); } })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/daemon/startDaemon.ts` around lines 1153 - 1245, The channel bridge startup (startChannelBridgeFromEnv) is tied to the successful return of ensureMachineRegistered so if registration fails the bridge is never attempted; move the channel-bridge initialization out of the post-registration success path so it runs independently (e.g., after the registration block or in a finally/independent async task), but still use existing credentials/token and optional server/account ids derived via decodeJwtPayload and readSettings; update references to channelBridgeRuntimeSettings/channelBridgeServerId/channelBridgeAccountId and startChannelBridgeFromEnv so they are computed and invoked even when ensureMachineRegistered throws or returns a fallback, ensuring failures are caught and logged as they currently are (use the same serializeAxiosErrorForLog and warning behavior).
🧹 Nitpick comments (1)
apps/cli/src/ui/doctor.ts (1)
46-77: Add JSDoc for new exported webhook diagnostics helpers.
isMissingRequiredTelegramWebhookSecretandcollectMissingRequiredWebhookFieldsare public exports with core validation behavior; brief JSDoc would improve maintainability.As per coding guidelines "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/ui/doctor.ts` around lines 46 - 77, Add brief JSDoc comments for the two exported functions isMissingRequiredTelegramWebhookSecret and collectMissingRequiredWebhookFields: for isMissingRequiredTelegramWebhookSecret document the purpose (returns true when webhook.enabled is true and webhookSecret is empty/whitespace), describe the params object (webhookEnabled:boolean, webhookSecret:string) and the boolean return; for collectMissingRequiredWebhookFields document that it returns an array of human-readable issue strings when webhookEnabled is true, describe the params object (webhookEnabled:boolean, webhookSecret:string, webhookHost:string, webhookPort:number|null) and explain the validation rules it applies (empty secret/host, and port must be finite integer in 1..65535), and include `@param` and `@returns` tags so these public helpers are self-describing.
🤖 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/startChannelBridgeWorker.ts`:
- Around line 204-205: relayHandle and defaultDepsHandle are set up before
calling startChannelBridgeWorker(...) so if that call throws the created
resources aren't cleaned up; wrap the worker creation and any subsequent
initialization in a try/catch (or try/finally) so you only assign to
relayHandle/defaultDepsHandle after successful creation or ensure you
close/cleanup them in the catch, specifically around the call to
startChannelBridgeWorker and the code that constructs adapters (refer to
relayHandle, defaultDepsHandle, startChannelBridgeWorker, and params.adapters),
and on error call the appropriate shutdown/close methods for those handles to
guarantee no leaked resources.
- Around line 40-63: The extractAssistantText function uses broad "as any" casts
which bypass TS safety; replace them by defining discriminated interfaces for
the content shapes (e.g., TextContent { type: 'text'; text: string }, AcpContent
{ type: 'acp'; data: AcpData }, OutputContent { type: 'output'; data: OutputData
} and AcpData variants like MessageData { type: 'message' | 'reasoning';
message: string }) and implement type-guard functions (isTextContent(content),
isAcpContent(content), isOutputContent(content), isMessageData(data)) to narrow
the union; then access content.type, content.text, data.type and data.message
with proper typings instead of casting, keeping the existing empty/null checks
(and Array.isArray guards) but using the narrowed types for safe property access
inside extractAssistantText.
In `@apps/cli/src/ui/doctor.ts`:
- Around line 318-320: The runtimeConfigSource detection is using the wrapper
object returned by readChannelBridgeTelegramConfigFromKv (assigned to
serverRecord) which is always truthy; update the conditional to check the actual
record field instead (use if (serverRecord.record != null)) so
runtimeConfigSource is set to 'server KV overlay + local settings' only when an
actual KV record exists; change the check surrounding runtimeConfigSource
assignment (referencing variables runtimeConfigSource and serverRecord)
accordingly.
---
Duplicate comments:
In `@apps/cli/src/daemon/startDaemon.ts`:
- Around line 1153-1245: The channel bridge startup (startChannelBridgeFromEnv)
is tied to the successful return of ensureMachineRegistered so if registration
fails the bridge is never attempted; move the channel-bridge initialization out
of the post-registration success path so it runs independently (e.g., after the
registration block or in a finally/independent async task), but still use
existing credentials/token and optional server/account ids derived via
decodeJwtPayload and readSettings; update references to
channelBridgeRuntimeSettings/channelBridgeServerId/channelBridgeAccountId and
startChannelBridgeFromEnv so they are computed and invoked even when
ensureMachineRegistered throws or returns a fallback, ensuring failures are
caught and logged as they currently are (use the same serializeAxiosErrorForLog
and warning behavior).
---
Nitpick comments:
In `@apps/cli/src/ui/doctor.ts`:
- Around line 46-77: Add brief JSDoc comments for the two exported functions
isMissingRequiredTelegramWebhookSecret and collectMissingRequiredWebhookFields:
for isMissingRequiredTelegramWebhookSecret document the purpose (returns true
when webhook.enabled is true and webhookSecret is empty/whitespace), describe
the params object (webhookEnabled:boolean, webhookSecret:string) and the boolean
return; for collectMissingRequiredWebhookFields document that it returns an
array of human-readable issue strings when webhookEnabled is true, describe the
params object (webhookEnabled:boolean, webhookSecret:string, webhookHost:string,
webhookPort:number|null) and explain the validation rules it applies (empty
secret/host, and port must be finite integer in 1..65535), and include `@param`
and `@returns` tags so these public helpers are self-describing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90fb4f4d-a41b-4083-976f-d39b4d55d96c
⛔ Files ignored due to path filters (3)
docs/assets/doctor-critical-summary.pngis excluded by!**/*.pngdocs/assets/doctor-missing-host-port-summary.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (29)
README.mdapps/cli/src/channels/channelBindingStore.server.test.tsapps/cli/src/channels/channelBindingStore.server.tsapps/cli/src/channels/channelBridgeAccountConfig.test.tsapps/cli/src/channels/channelBridgeAccountConfig.tsapps/cli/src/channels/channelBridgeConfig.test.tsapps/cli/src/channels/channelBridgeConfig.tsapps/cli/src/channels/channelBridgeServerConfigOverlay.test.tsapps/cli/src/channels/channelBridgeServerConfigOverlay.tsapps/cli/src/channels/channelBridgeServerKv.test.tsapps/cli/src/channels/channelBridgeServerKv.tsapps/cli/src/channels/core/channelBridgeWorker.test.tsapps/cli/src/channels/core/channelBridgeWorker.tsapps/cli/src/channels/startChannelBridgeWorker.test.tsapps/cli/src/channels/startChannelBridgeWorker.tsapps/cli/src/channels/telegram/telegramAdapter.test.tsapps/cli/src/channels/telegram/telegramAdapter.tsapps/cli/src/channels/telegram/telegramWebhookRelay.test.tsapps/cli/src/channels/telegram/telegramWebhookRelay.tsapps/cli/src/cli/commandRegistry.tsapps/cli/src/cli/commands/bridge.test.tsapps/cli/src/cli/commands/bridge.tsapps/cli/src/cli/dispatch.tsapps/cli/src/daemon/startDaemon.tsapps/cli/src/persistence.tsapps/cli/src/ui/doctor.test.tsapps/cli/src/ui/doctor.tsdocs/channel-bridge-uat.mddocs/telegram-channel-bridge.md
✅ Files skipped from review due to trivial changes (1)
- docs/telegram-channel-bridge.md
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/cli/src/channels/telegram/telegramWebhookRelay.test.ts
- apps/cli/src/ui/doctor.test.ts
- apps/cli/src/channels/channelBridgeAccountConfig.test.ts
- apps/cli/src/channels/channelBridgeServerKv.test.ts
- apps/cli/src/persistence.ts
- apps/cli/src/channels/telegram/telegramAdapter.ts
- README.md
- docs/channel-bridge-uat.md
- apps/cli/src/channels/startChannelBridgeWorker.test.ts
- apps/cli/src/cli/commandRegistry.ts
- apps/cli/src/cli/dispatch.ts
cdenneen
added a commit
to cdenneen/happier-dev
that referenced
this pull request
Mar 4, 2026
Author
|
@coderabbitai review\n@greptile-ai review\n\nAddressed all newly opened #110 comments (including nitpick/duplicate handling), pushed in f730cb2, and resolved the corresponding threads with per-thread notes. |
|
The PR description images show:
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/cli/src/daemon/startDaemon.ts (1)
1197-1245:⚠️ Potential issue | 🟠 MajorStart channel bridge independently of machine registration.
At Line 1197 this startup path is still nested under successful
ensureMachineRegistered(...). If registration fails, daemon continues but channel bridge never starts for the process lifetime.🔧 Suggested refactor direction
+ const startChannelBridgeBestEffort = async (): Promise<ChannelBridgeRuntimeHandle | null> => { + const bridgeSettings = await readSettings().catch(() => null); + // optional KV overlay... + return await startChannelBridgeFromEnv({ credentials, ...(bridgeSettings ? { settings: bridgeSettings } : {}) }) + .catch(() => null); + }; + + channelBridgeWorker = await startChannelBridgeBestEffort(); + void (async () => { try { const ensured = await ensureMachineRegistered({ ... }); ... - const bridgeSettings = await readSettings()... - channelBridgeWorker = await startChannelBridgeFromEnv(...) } catch (error) { ... } })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/daemon/startDaemon.ts` around lines 1197 - 1245, The channel bridge startup (call to startChannelBridgeFromEnv using channelBridgeRuntimeSettings, channelBridgeServerId, channelBridgeAccountId) is currently executed only inside the successful ensureMachineRegistered flow, so a failed ensureMachineRegistered prevents the bridge from ever starting; refactor by extracting the bridge-start logic (the readSettings -> overlayServerKvTelegramConfigInSettings -> startChannelBridgeFromEnv sequence) into a separate path that runs even if ensureMachineRegistered throws or returns failure, ensuring credentials/tokenPayload are available, preserving existing error handling/log messages (logger.warn and serializeAxiosErrorForLog), and keeping the createAxiosChannelBridgeKvClient/readChannelBridgeTelegramConfigFromKv fetch only guarded by presence of channelBridgeServerId and channelBridgeAccountId.
🧹 Nitpick comments (2)
apps/cli/src/ui/doctor.ts (1)
67-75: Minor: Redundant null check in port validation.
Number.isFinite(null)returnsfalse, so the|| params.webhookPort === nullcheck on line 69 is redundant—the first condition already handlesnull. This doesn't affect correctness but could be simplified.♻️ Optional simplification
if ( !Number.isFinite(params.webhookPort) - || params.webhookPort === null || !Number.isInteger(params.webhookPort) || params.webhookPort <= 0 || params.webhookPort > 65_535 ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/ui/doctor.ts` around lines 67 - 75, The null check is redundant in the webhook port validation: remove the `params.webhookPort === null` branch and rely on Number.isFinite(params.webhookPort) and the other integer/range checks to validate the port; update the conditional in the block that references params.webhookPort (the if that pushes 'webhook.port: <empty/invalid>...') to drop the null check so the logic is simpler but functionally identical.apps/cli/src/channels/core/channelBridgeWorker.test.ts (1)
643-646: Replace fixed sleeps with explicit synchronization to reduce flaky tests.Using
setTimeout(50/25)for correctness sequencing can intermittently fail under CI load. Prefer deterministic gates (e.g., pull-call counters and deferred “tick entered” promises).Proposed refactor
function createAdapterHarness(providerId: string = 'telegram'): { adapter: ChannelBridgeAdapter; pushInbound: (event: ChannelBridgeInboundMessage) => void; sent: SentConversationMessage[]; failPullOnce: (error: Error) => void; stopCalls: () => number; + pullCalls: () => number; } { @@ let stopCallCount = 0; + let pullCallCount = 0; @@ pullInboundMessages: async () => { + pullCallCount += 1; if (pullError) { @@ stopCalls: () => stopCallCount, + pullCalls: () => pullCallCount, }; } @@ - worker.trigger(); - - await new Promise((resolve) => setTimeout(resolve, 50)); + worker.trigger(); + await waitFor(() => harness.pullCalls() >= 2); expect(sentToSession).toHaveLength(1); @@ - const gate = createDeferredPromise<void>(); + const gate = createDeferredPromise<void>(); + const tickEntered = createDeferredPromise<void>(); const { deps } = createDepsHarness({ sendUserMessageToSession: async () => { + tickEntered.resolve(); await gate.promise; }, }); @@ - await new Promise((resolve) => setTimeout(resolve, 25)); + await tickEntered.promise; @@ - await new Promise((resolve) => setTimeout(resolve, 25)); + await Promise.resolve(); expect(harness.stopCalls()).toBe(0);Also applies to: 685-691
🤖 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 643 - 646, Replace the fragile setTimeout-based waits after calling worker.trigger() with an explicit synchronization primitive: create a deferred promise (e.g., tickEntered) that is resolved from the code path that indicates the worker tick has started or completed (for example inside the same place that will eventually call the session send used in this test), then await that promise in the test instead of setTimeout; specifically, change the test that calls worker.trigger() to await the new tickEntered promise (or await until the sentToSession spy/handler is invoked) before asserting expect(sentToSession).toHaveLength(1), and apply the same pattern for the other occurrence around lines 685-691 so the test deterministically waits for worker tick completion rather than sleeping.
🤖 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/channelBridgeServerKv.ts`:
- Around line 175-208: The code is currently coercing non-string threadId values
to null which can merge distinct bindings; update the parsing loop that
processes rawBindings (inside the function that builds
ChannelBridgeServerBindingRecord using asRecord) to validate threadId the same
way as providerId/conversationId/sessionId: if item.threadId is defined but not
a string, throw an Error indicating an invalid threadId at the current index;
otherwise set threadId to the trimmed string (or undefined if you prefer
consistent typing) instead of silently assigning null; ensure consumers
expecting threadId (and the creation of the binding object where threadId and
lastForwardedSeq are set) use the validated value.
In `@apps/cli/src/channels/startChannelBridgeWorker.ts`:
- Around line 316-324: The current stop implementation's finally block calls
relayHandle.stop() directly so any exception there prevents
defaultDepsHandle?.dispose() from running; update the finally block in the stop
function to ensure defaultDepsHandle?.dispose() always runs by isolating
relayHandle.stop() in its own try/catch (or using an inner try/finally) and
swallowing or logging relay stop errors, then calling
defaultDepsHandle?.dispose() in the outer finally; refer to the stop function
and the symbols worker.stop(), relayHandle.stop(), and
defaultDepsHandle?.dispose() when making the change.
In `@apps/cli/src/channels/telegram/telegramWebhookRelay.test.ts`:
- Around line 41-67: The test name says it covers both missing and invalid
header tokens but only checks the missing-header case; add a second POST to the
same relay URL that sends the header 'x-telegram-bot-api-secret-token' (or the
header key your relay expects) with an incorrect value (e.g., 'invalid-token')
using the same validateStatus: () => true, then assert response.status === 401
and that received remains empty; keep using startTelegramWebhookRelay
(secretHeaderToken: 'header-token-abc'), relay.port/relay.path, the received
array populated by onUpdate, and ensure relay.stop() runs in the finally block.
In `@apps/cli/src/cli/commands/bridge.ts`:
- Around line 239-241: The CLI currently allows persisting webhook port 0;
update the validation at the assignment to update.webhookPort so
parseIntegerInput('--webhook-port') enforces the valid TCP port range 1..65535
by changing the minimum from 0 to 1 (i.e., call
parseIntegerInput(webhookPortRaw, '--webhook-port', 1, 65535)); ensure the same
1..65535 constraint is applied wherever user-specified bridge webhook ports are
validated so a saved config can never be 0.
---
Duplicate comments:
In `@apps/cli/src/daemon/startDaemon.ts`:
- Around line 1197-1245: The channel bridge startup (call to
startChannelBridgeFromEnv using channelBridgeRuntimeSettings,
channelBridgeServerId, channelBridgeAccountId) is currently executed only inside
the successful ensureMachineRegistered flow, so a failed ensureMachineRegistered
prevents the bridge from ever starting; refactor by extracting the bridge-start
logic (the readSettings -> overlayServerKvTelegramConfigInSettings ->
startChannelBridgeFromEnv sequence) into a separate path that runs even if
ensureMachineRegistered throws or returns failure, ensuring
credentials/tokenPayload are available, preserving existing error handling/log
messages (logger.warn and serializeAxiosErrorForLog), and keeping the
createAxiosChannelBridgeKvClient/readChannelBridgeTelegramConfigFromKv fetch
only guarded by presence of channelBridgeServerId and channelBridgeAccountId.
---
Nitpick comments:
In `@apps/cli/src/channels/core/channelBridgeWorker.test.ts`:
- Around line 643-646: Replace the fragile setTimeout-based waits after calling
worker.trigger() with an explicit synchronization primitive: create a deferred
promise (e.g., tickEntered) that is resolved from the code path that indicates
the worker tick has started or completed (for example inside the same place that
will eventually call the session send used in this test), then await that
promise in the test instead of setTimeout; specifically, change the test that
calls worker.trigger() to await the new tickEntered promise (or await until the
sentToSession spy/handler is invoked) before asserting
expect(sentToSession).toHaveLength(1), and apply the same pattern for the other
occurrence around lines 685-691 so the test deterministically waits for worker
tick completion rather than sleeping.
In `@apps/cli/src/ui/doctor.ts`:
- Around line 67-75: The null check is redundant in the webhook port validation:
remove the `params.webhookPort === null` branch and rely on
Number.isFinite(params.webhookPort) and the other integer/range checks to
validate the port; update the conditional in the block that references
params.webhookPort (the if that pushes 'webhook.port: <empty/invalid>...') to
drop the null check so the logic is simpler but functionally identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb788a04-beed-491b-b6cb-99ad9c48857a
⛔ Files ignored due to path filters (3)
docs/assets/doctor-critical-summary.pngis excluded by!**/*.pngdocs/assets/doctor-missing-host-port-summary.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (30)
README.mdapps/cli/src/channels/channelBindingStore.server.test.tsapps/cli/src/channels/channelBindingStore.server.tsapps/cli/src/channels/channelBridgeAccountConfig.test.tsapps/cli/src/channels/channelBridgeAccountConfig.tsapps/cli/src/channels/channelBridgeConfig.test.tsapps/cli/src/channels/channelBridgeConfig.tsapps/cli/src/channels/channelBridgeServerConfigOverlay.test.tsapps/cli/src/channels/channelBridgeServerConfigOverlay.tsapps/cli/src/channels/channelBridgeServerKv.test.tsapps/cli/src/channels/channelBridgeServerKv.tsapps/cli/src/channels/core/channelBridgeWorker.test.tsapps/cli/src/channels/core/channelBridgeWorker.tsapps/cli/src/channels/startChannelBridgeWorker.startupFailure.test.tsapps/cli/src/channels/startChannelBridgeWorker.test.tsapps/cli/src/channels/startChannelBridgeWorker.tsapps/cli/src/channels/telegram/telegramAdapter.test.tsapps/cli/src/channels/telegram/telegramAdapter.tsapps/cli/src/channels/telegram/telegramWebhookRelay.test.tsapps/cli/src/channels/telegram/telegramWebhookRelay.tsapps/cli/src/cli/commandRegistry.tsapps/cli/src/cli/commands/bridge.test.tsapps/cli/src/cli/commands/bridge.tsapps/cli/src/cli/dispatch.tsapps/cli/src/daemon/startDaemon.tsapps/cli/src/persistence.tsapps/cli/src/ui/doctor.test.tsapps/cli/src/ui/doctor.tsdocs/channel-bridge-uat.mddocs/telegram-channel-bridge.md
✅ Files skipped from review due to trivial changes (1)
- apps/cli/src/channels/channelBridgeConfig.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/cli/src/ui/doctor.test.ts
- apps/cli/src/cli/commands/bridge.test.ts
- apps/cli/src/channels/channelBridgeAccountConfig.test.ts
- apps/cli/src/channels/startChannelBridgeWorker.test.ts
- apps/cli/src/channels/channelBridgeConfig.test.ts
- apps/cli/src/channels/channelBindingStore.server.ts
- apps/cli/src/persistence.ts
- apps/cli/src/cli/dispatch.ts
- apps/cli/src/channels/telegram/telegramAdapter.test.ts
- docs/channel-bridge-uat.md
- apps/cli/src/channels/channelBridgeServerKv.test.ts
- apps/cli/src/channels/channelBindingStore.server.test.ts
Author
|
@greptile review |
cdenneen
added a commit
to cdenneen/happier-dev
that referenced
this pull request
Mar 4, 2026
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/cli/src/ui/doctor.test.ts (1)
97-109: Loosen copy-pinned assertions for webhook issue messages.This test currently hard-locks exact user-facing wording. Prefer stable checks (issue count + key substrings) so harmless copy updates don’t break behavior tests.
✅ Suggested assertion style
- ).toEqual([ - 'webhook.secret: <empty> (required when webhook.enabled=true)', - 'webhook.host: <empty> (required when webhook.enabled=true)', - 'webhook.port: <empty/invalid> (required when webhook.enabled=true)', - ]); + ); + expect(issues).toHaveLength(3); + expect(issues).toEqual(expect.arrayContaining([ + expect.stringContaining('webhook.secret'), + expect.stringContaining('webhook.host'), + expect.stringContaining('webhook.port'), + ]));Based on learnings "Avoid brittle 'content policing' tests that pin default config values, exact Markdown wording/format/length, or user-facing copy unless copy itself is the product requirement."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/ui/doctor.test.ts` around lines 97 - 109, The test "flags missing required webhook fields when enabled" pins exact user-facing strings from collectMissingRequiredWebhookFields; change it to assert the stable behavior instead: replace the exact toEqual assertion with checks that the returned array has length 3 (expect(result).toHaveLength(3)) and that it contains entries including the key substrings 'webhook.secret', 'webhook.host', and 'webhook.port' (use expect(result.some(s => s.includes(...))).toBe(true) or expect.stringContaining in matcher) so the test is resilient to harmless copy edits while still verifying the required fields are reported.apps/cli/src/ui/doctor.ts (1)
46-77: Consolidate webhook-secret validation to avoid rule drift.
isMissingRequiredTelegramWebhookSecretandcollectMissingRequiredWebhookFieldsencode the same “secret required when enabled” rule separately. Consider using one source of truth (call the helper from the collector, or drop the extra export if it is only test-facing).♻️ Proposed refactor
export function collectMissingRequiredWebhookFields(params: Readonly<{ webhookEnabled: boolean; webhookSecret: string; webhookHost: string; webhookPort: number | null; }>): string[] { if (!params.webhookEnabled) return []; const issues: string[] = []; - if (params.webhookSecret.trim().length === 0) { + if (isMissingRequiredTelegramWebhookSecret({ + webhookEnabled: params.webhookEnabled, + webhookSecret: params.webhookSecret, + })) { issues.push('webhook.secret: <empty> (required when webhook.enabled=true)'); }As per coding guidelines "Apply DRY, SOLID, and KISS principles - avoid speculative or YAGNI code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/ui/doctor.ts` around lines 46 - 77, The webhook-secret rule is duplicated between isMissingRequiredTelegramWebhookSecret and collectMissingRequiredWebhookFields; refactor so collectMissingRequiredWebhookFields reuses isMissingRequiredTelegramWebhookSecret (call it instead of re-checking params.webhookSecret) to maintain a single source of truth, and if isMissingRequiredTelegramWebhookSecret exists only for internal use or tests consider removing its separate export or document its test-only usage; update the signature/use if needed so isMissingRequiredTelegramWebhookSecret accepts the same params shape (or make a small adapter) and delete the duplicate secret-check in collectMissingRequiredWebhookFields.apps/cli/src/cli/commands/bridge.ts (1)
1-24: Add a file-level JSDoc responsibility header.This new module is substantial and should include the required top-level responsibility comment for maintainability and onboarding.
As per coding guidelines, “Include comprehensive JSDoc comments as file header comments explaining file responsibilities”.Proposed header
+/** + * Bridge CLI command handlers. + * + * Responsibilities: + * - Parse and validate `happier bridge` subcommands/flags + * - Resolve active auth/server scope + * - Read/write scoped Telegram bridge config + * - Sync non-secret bridge settings with server KV + * - Render effective bridge diagnostics for CLI users + */ import chalk from 'chalk';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/cli/commands/bridge.ts` around lines 1 - 24, Add a file-level JSDoc header at the top of this module describing its responsibilities: state that this file implements CLI bridge command handling (using CommandContext) and coordinates token handling (decodeJwtPayload), daemon checks (checkIfDaemonRunningAndCleanupStaleState), channel bridge config read/modify routines (hasSharedTelegramBridgeUpdate, readScopedTelegramBridgeConfig, upsertScopedTelegramBridgeConfig, removeScopedTelegramBridgeConfig, splitScopedTelegramBridgeUpdate), server KV interactions (createAxiosChannelBridgeKvClient, readChannelBridgeTelegramConfigFromKv, upsertChannelBridgeTelegramConfigInKv, clearChannelBridgeTelegramConfigInKv), config overlaying (overlayServerKvTelegramConfigInSettings), and persistence usage (readCredentials, readSettings, updateSettings) along with expected side effects and inputs/outputs; ensure the JSDoc mentions configuration coupling (configuration) and any daemon or network side effects so maintainers know responsibilities and invariants.docs/channel-bridge.md (1)
21-21: Tighten wording in the agent-forwarding bullet.Minor readability tweak: “forwards agent replies back” can be simplified to “forwards agent replies”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/channel-bridge.md` at line 21, Update the agent-forwarding bullet by replacing the phrase "forwards agent replies back to the bound conversation/thread" with the simpler "forwards agent replies"; locate the exact bullet that currently reads "forwards agent replies back to the bound conversation/thread" and edit that line to improve readability while preserving the rest of the sentence and meaning.apps/cli/src/channels/channelBridgeAccountConfig.ts (1)
232-234: NormalizeallowedChatIdson write for canonical persisted config.Write-path currently copies raw values, while read-path trims/filters. Normalizing at write-time keeps stored settings consistent and avoids churn.
♻️ Suggested fix
if (Array.isArray(params.update.allowedChatIds)) { - telegram.allowedChatIds = [...params.update.allowedChatIds]; + telegram.allowedChatIds = parseStringArray(params.update.allowedChatIds); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/channelBridgeAccountConfig.ts` around lines 232 - 234, The write path currently copies params.update.allowedChatIds directly into telegram.allowedChatIds causing inconsistent stored values; change the assignment in the channelBridgeAccountConfig update logic to normalize the array before persisting: ensure you map each entry to a trimmed string, filter out empty/invalid values (e.g., falsy or non-string), and deduplicate the list (preserve order) when setting telegram.allowedChatIds from params.update.allowedChatIds so stored config matches the read-path normalization behavior.
🤖 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 186-202: listBindings, getBinding, and upsertBinding are returning
live objects from the in-memory Map (byKey) which allows external mutation to
corrupt state; fix by returning and storing defensive copies instead of original
references: change listBindings to return Array.from(byKey.values()).map(v => ({
...v })), change getBinding to return v ? { ...v } : null, and in upsertBinding
ensure you set a copy into byKey (byKey.set(key, { ...next })) and return a
fresh copy (return { ...next }); keep use of bindingKey and
ChannelSessionBinding and preserve createdAtMs/updatedAtMs logic.
In `@apps/cli/src/channels/telegram/telegramAdapter.ts`:
- Around line 51-56: The sendMessage implementation in telegramAdapter
(sendMessage async function) should validate threadId before adding
message_thread_id to the payload: parse threadId to a number (e.g., Number or
parseInt), ensure it is a finite integer and within Telegram's valid range
(positive integer), and only include message_thread_id when that check passes;
otherwise omit the field (or log/throw a clear validation error). Update the
payload construction around the current ...(threadId ? { message_thread_id:
Number(threadId) } : {}) to perform this validation (using
Number.isFinite/Number.isInteger or similar) so malformed threadId values are
not sent to axios.
---
Nitpick comments:
In `@apps/cli/src/channels/channelBridgeAccountConfig.ts`:
- Around line 232-234: The write path currently copies
params.update.allowedChatIds directly into telegram.allowedChatIds causing
inconsistent stored values; change the assignment in the
channelBridgeAccountConfig update logic to normalize the array before
persisting: ensure you map each entry to a trimmed string, filter out
empty/invalid values (e.g., falsy or non-string), and deduplicate the list
(preserve order) when setting telegram.allowedChatIds from
params.update.allowedChatIds so stored config matches the read-path
normalization behavior.
In `@apps/cli/src/cli/commands/bridge.ts`:
- Around line 1-24: Add a file-level JSDoc header at the top of this module
describing its responsibilities: state that this file implements CLI bridge
command handling (using CommandContext) and coordinates token handling
(decodeJwtPayload), daemon checks (checkIfDaemonRunningAndCleanupStaleState),
channel bridge config read/modify routines (hasSharedTelegramBridgeUpdate,
readScopedTelegramBridgeConfig, upsertScopedTelegramBridgeConfig,
removeScopedTelegramBridgeConfig, splitScopedTelegramBridgeUpdate), server KV
interactions (createAxiosChannelBridgeKvClient,
readChannelBridgeTelegramConfigFromKv, upsertChannelBridgeTelegramConfigInKv,
clearChannelBridgeTelegramConfigInKv), config overlaying
(overlayServerKvTelegramConfigInSettings), and persistence usage
(readCredentials, readSettings, updateSettings) along with expected side effects
and inputs/outputs; ensure the JSDoc mentions configuration coupling
(configuration) and any daemon or network side effects so maintainers know
responsibilities and invariants.
In `@apps/cli/src/ui/doctor.test.ts`:
- Around line 97-109: The test "flags missing required webhook fields when
enabled" pins exact user-facing strings from
collectMissingRequiredWebhookFields; change it to assert the stable behavior
instead: replace the exact toEqual assertion with checks that the returned array
has length 3 (expect(result).toHaveLength(3)) and that it contains entries
including the key substrings 'webhook.secret', 'webhook.host', and
'webhook.port' (use expect(result.some(s => s.includes(...))).toBe(true) or
expect.stringContaining in matcher) so the test is resilient to harmless copy
edits while still verifying the required fields are reported.
In `@apps/cli/src/ui/doctor.ts`:
- Around line 46-77: The webhook-secret rule is duplicated between
isMissingRequiredTelegramWebhookSecret and collectMissingRequiredWebhookFields;
refactor so collectMissingRequiredWebhookFields reuses
isMissingRequiredTelegramWebhookSecret (call it instead of re-checking
params.webhookSecret) to maintain a single source of truth, and if
isMissingRequiredTelegramWebhookSecret exists only for internal use or tests
consider removing its separate export or document its test-only usage; update
the signature/use if needed so isMissingRequiredTelegramWebhookSecret accepts
the same params shape (or make a small adapter) and delete the duplicate
secret-check in collectMissingRequiredWebhookFields.
In `@docs/channel-bridge.md`:
- Line 21: Update the agent-forwarding bullet by replacing the phrase "forwards
agent replies back to the bound conversation/thread" with the simpler "forwards
agent replies"; locate the exact bullet that currently reads "forwards agent
replies back to the bound conversation/thread" and edit that line to improve
readability while preserving the rest of the sentence and meaning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76ae3698-3c60-4242-9e1f-e41d3707c574
⛔ Files ignored due to path filters (3)
docs/assets/doctor-critical-summary.pngis excluded by!**/*.pngdocs/assets/doctor-missing-host-port-summary.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (32)
README.mdapps/cli/src/channels/channelBindingStore.server.test.tsapps/cli/src/channels/channelBindingStore.server.tsapps/cli/src/channels/channelBridgeAccountConfig.test.tsapps/cli/src/channels/channelBridgeAccountConfig.tsapps/cli/src/channels/channelBridgeConfig.test.tsapps/cli/src/channels/channelBridgeConfig.tsapps/cli/src/channels/channelBridgeServerConfigOverlay.test.tsapps/cli/src/channels/channelBridgeServerConfigOverlay.tsapps/cli/src/channels/channelBridgeServerKv.test.tsapps/cli/src/channels/channelBridgeServerKv.tsapps/cli/src/channels/core/channelBridgeWorker.test.tsapps/cli/src/channels/core/channelBridgeWorker.tsapps/cli/src/channels/startChannelBridgeWorker.startupFailure.test.tsapps/cli/src/channels/startChannelBridgeWorker.test.tsapps/cli/src/channels/startChannelBridgeWorker.tsapps/cli/src/channels/telegram/telegramAdapter.test.tsapps/cli/src/channels/telegram/telegramAdapter.tsapps/cli/src/channels/telegram/telegramWebhookRelay.test.tsapps/cli/src/channels/telegram/telegramWebhookRelay.tsapps/cli/src/cli/commandRegistry.tsapps/cli/src/cli/commands/bridge.test.tsapps/cli/src/cli/commands/bridge.tsapps/cli/src/cli/dispatch.tsapps/cli/src/daemon/startDaemon.tsapps/cli/src/persistence.tsapps/cli/src/ui/doctor.test.tsapps/cli/src/ui/doctor.tsdocs/README.mddocs/channel-bridge-uat.mddocs/channel-bridge.mddocs/telegram-channel-bridge.md
✅ Files skipped from review due to trivial changes (1)
- docs/channel-bridge-uat.md
🚧 Files skipped from review as they are similar to previous changes (12)
- README.md
- apps/cli/src/channels/channelBridgeServerKv.test.ts
- apps/cli/src/channels/channelBridgeAccountConfig.test.ts
- apps/cli/src/cli/commands/bridge.test.ts
- apps/cli/src/channels/channelBridgeServerConfigOverlay.test.ts
- apps/cli/src/channels/channelBridgeConfig.test.ts
- apps/cli/src/daemon/startDaemon.ts
- apps/cli/src/channels/channelBindingStore.server.test.ts
- apps/cli/src/persistence.ts
- apps/cli/src/channels/channelBridgeConfig.ts
- apps/cli/src/cli/dispatch.ts
- docs/telegram-channel-bridge.md
Author
|
@coderabbitai review |
Author
|
@greptile review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/channel-bridge.md (1)
7-14: Make key file references clickable links.These paths are currently plain code spans, which makes doc navigation and link validation slower. Converting them to Markdown links improves maintainability and reviewer UX.
Suggested doc tweak
-- Worker/runtime: `apps/cli/src/channels/core/channelBridgeWorker.ts` -- Runtime bootstrapping: `apps/cli/src/channels/startChannelBridgeWorker.ts` -- Scoped bridge config + KV overlay: `apps/cli/src/channels/channelBridgeConfig.ts` +- Worker/runtime: [`apps/cli/src/channels/core/channelBridgeWorker.ts`](../apps/cli/src/channels/core/channelBridgeWorker.ts) +- Runtime bootstrapping: [`apps/cli/src/channels/startChannelBridgeWorker.ts`](../apps/cli/src/channels/startChannelBridgeWorker.ts) +- Scoped bridge config + KV overlay: [`apps/cli/src/channels/channelBridgeConfig.ts`](../apps/cli/src/channels/channelBridgeConfig.ts)As per coding guidelines "
**/*.md: Review documentation for ... Working links".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/channel-bridge.md` around lines 7 - 14, Replace the plain code-span file paths in docs/channel-bridge.md with Markdown links so they are clickable; specifically convert `apps/cli/src/channels/core/channelBridgeWorker.ts`, `apps/cli/src/channels/startChannelBridgeWorker.ts`, `apps/cli/src/channels/channelBridgeConfig.ts` and `docs/telegram-channel-bridge.md` into [label](relative/path) style links (keep labels concise, e.g. file basename) and remove the surrounding backticks so the links render and validate in the repository.apps/cli/src/ui/doctor.ts (1)
46-58: Add JSDoc + named interfaces for exported helper contracts.
isMissingRequiredTelegramWebhookSecretandcollectMissingRequiredWebhookFieldsare exported APIs but currently use inline object shapes and no API docs. Please extract parameter contracts intointerfaces and add brief JSDoc for each export.As per coding guidelines, "Add JSDoc comments for public APIs and complex logic" and "Prefer
interfaceovertypefor defining object shapes in TypeScript".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/ui/doctor.ts` around lines 46 - 58, Extract the inline object parameter types into exported interfaces (e.g., TelegramWebhookSecretParams for isMissingRequiredTelegramWebhookSecret and CollectWebhookFieldsParams for collectMissingRequiredWebhookFields), update the function signatures to accept those interface types instead of inline shapes, and add brief JSDoc comments above each exported interface and function describing the purpose, expected properties, and return value; ensure interfaces use the interface keyword and are exported so the public API is documented and types are reusable.
🤖 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 513-531: The loop uses Math.trunc(row.seq) without validating
row.seq, so NaN can break cursor advancement and cause replay loops; before
computing nextSeq in the messages loop use a guarded numeric conversion (e.g.,
check Number.isFinite(row.seq) or parseInt fallback) and only consider row.seq
for Math.trunc when it yields a valid finite integer; ensure nextSeq is computed
from a valid number (default to maxSeq or 0 when invalid) and only call
params.store.updateLastForwardedSeq(binding, maxSeq) when maxSeq actually
advances to a higher finite value so adapter.sendMessage and
updateLastForwardedSeq use safe, validated seq values.
In `@apps/cli/src/ui/doctor.ts`:
- Around line 371-380: The current validation builds webhookHostForValidation
and webhookPortForValidation from runtimeBridge.telegram.webhookHost/webhookPort
(which are runtime-defaulted) and then calls
collectMissingRequiredWebhookFields, masking missing explicit config; change
those assignments to use the explicit/raw webhook inputs (the CLI/config values
provided earlier in this file — e.g., webhookHost/webhookPort or
webhookHostInput/webhookPortInput) instead of runtimeBridge.telegram.* so
missing host/port are detected, applying the same trimming and >0 check (empty
string -> null for host, non‑positive -> null for port) before passing them into
collectMissingRequiredWebhookFields along with webhookEnabled and webhookSecret.
---
Nitpick comments:
In `@apps/cli/src/ui/doctor.ts`:
- Around line 46-58: Extract the inline object parameter types into exported
interfaces (e.g., TelegramWebhookSecretParams for
isMissingRequiredTelegramWebhookSecret and CollectWebhookFieldsParams for
collectMissingRequiredWebhookFields), update the function signatures to accept
those interface types instead of inline shapes, and add brief JSDoc comments
above each exported interface and function describing the purpose, expected
properties, and return value; ensure interfaces use the interface keyword and
are exported so the public API is documented and types are reusable.
In `@docs/channel-bridge.md`:
- Around line 7-14: Replace the plain code-span file paths in
docs/channel-bridge.md with Markdown links so they are clickable; specifically
convert `apps/cli/src/channels/core/channelBridgeWorker.ts`,
`apps/cli/src/channels/startChannelBridgeWorker.ts`,
`apps/cli/src/channels/channelBridgeConfig.ts` and
`docs/telegram-channel-bridge.md` into [label](relative/path) style links (keep
labels concise, e.g. file basename) and remove the surrounding backticks so the
links render and validate in the repository.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12ee4229-91b7-45b4-ba7a-cc8df4bf12af
📒 Files selected for processing (5)
apps/cli/src/channels/core/channelBridgeWorker.test.tsapps/cli/src/channels/core/channelBridgeWorker.tsapps/cli/src/channels/telegram/telegramAdapter.tsapps/cli/src/ui/doctor.tsdocs/channel-bridge.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cli/src/channels/telegram/telegramAdapter.ts
cdenneen
force-pushed
the
feat/telegram-channel-adapter
branch
from
4894e5b to
793fea0
Compare
Author
|
@coderabbitai review |
Author
|
@greptile review |
✅ Actions performedReview triggered.
|
Author
|
@coderabbitai review |
Author
|
@greptile review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/cli/src/daemon/startDaemon.ts (1)
1197-1245:⚠️ Potential issue | 🟠 MajorChannel bridge startup is still coupled to machine registration success.
Line 1197 runs bridge initialization inside the machine-registration
trypath. If registration fails and execution goes to the catch path (Line 1312), bridge startup is skipped for the daemon lifetime even though daemon startup continues.Suggested refactor direction
+const startChannelBridgeBestEffort = async () => { + // existing bridgeSettings/KV overlay/startChannelBridgeFromEnv block +}; + +await startChannelBridgeBestEffort(); + void (async () => { try { const ensured = await ensureMachineRegistered({...}); // machine sync wiring... - // remove bridge startup from this success-only path } catch (error) { logger.warn(...); } })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/daemon/startDaemon.ts` around lines 1197 - 1245, The channel bridge startup (startChannelBridgeFromEnv + readSettings/readChannelBridgeTelegramConfigFromKv/overlayServerKvTelegramConfigInSettings) is currently executed inside the machine-registration try path so it is skipped whenever registration fails; move the entire bridge initialization block out of the machine-registration try/catch so it always runs regardless of registration outcome, ensuring it still uses existing locals such as credentials and configuration, and wrap the moved logic in its own try/catch that logs failures (using logger.warn and serializeAxiosErrorForLog) and returns null on error to preserve the previous best-effort behavior.
🧹 Nitpick comments (6)
apps/cli/src/channels/channelBindingStore.server.test.ts (1)
6-120: Consider extracting shared KV logic to reduce duplication.
createInMemoryKvClientandcreateCountingKvClientshare nearly identicalgetandmutateimplementations. You could extract the core logic into a shared factory and compose the counting variant on top.♻️ Suggested consolidation approach
-function createInMemoryKvClient(): ChannelBridgeKvClient { - const byKey = new Map<string, { value: string | null; version: number }>(); - - return { - get: async (key) => { - const row = byKey.get(key); - if (!row || row.value === null) { - return { status: 404, body: { error: 'Key not found' } }; - } - return { - status: 200, - body: { - key, - value: row.value, - version: row.version, - }, - }; - }, - mutate: async (mutations) => { - // ... full mutate logic - }, - }; -} - -function createCountingKvClient(): Readonly<{ - kv: ChannelBridgeKvClient; - mutateCallCount: () => number; -}> { - const byKey = new Map<string, { value: string | null; version: number }>(); - let mutateCalls = 0; - // ... duplicated logic -} +function createInMemoryKvClient(options?: { trackMutations?: boolean }): Readonly<{ + kv: ChannelBridgeKvClient; + mutateCallCount: () => number; +}> { + const byKey = new Map<string, { value: string | null; version: number }>(); + let mutateCalls = 0; + + const kv: ChannelBridgeKvClient = { + get: async (key) => { /* shared logic */ }, + mutate: async (mutations) => { + mutateCalls += 1; + /* shared logic */ + }, + }; + + return { kv, mutateCallCount: () => mutateCalls }; +}This removes ~50 lines of duplication while keeping the specialized
createConflictPayloadKvClientseparate for its unique first-call conflict behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/channelBindingStore.server.test.ts` around lines 6 - 120, The two functions createInMemoryKvClient and createCountingKvClient duplicate the same get/mutate logic; extract that core into a shared factory (e.g., makeBaseKvClient or buildKvClient) that encapsulates the byKey map and returns get and mutate implementations, then reimplement createInMemoryKvClient by returning the base client and reimplement createCountingKvClient by composing the base client with a counter hook (incrementing mutateCalls inside the shared mutate or by wrapping mutate) and exposing mutateCallCount; keep the same return shapes and preserve semantics of get, mutate, and mutateCallCount.apps/cli/src/daemon/startDaemon.ts (1)
1398-1404: Clear the stop-timeout when worker stop completes first.The timeout handle created for the race is never cleared on the fast path. Clearing it avoids a needless delayed callback during shutdown.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/daemon/startDaemon.ts` around lines 1398 - 1404, The Promise.race timeout created for channelBridgeWorker.stop() isn't being cleared on the fast path; capture the timeout handle (created with setTimeout and stored in a variable) and clear it when the stop promise resolves so the delayed timeout callback is cancelled—modify the race so the channelBridgeWorker.stop().then(...) handler calls clearTimeout(timeoutHandle) before returning 'stopped' (keep using channelBridgeStopTimeoutMs and the existing Promise.race/stopResult flow).apps/cli/src/channels/channelBridgeAccountConfig.ts (2)
232-234: NormalizeallowedChatIdson write to keep persisted state canonical.At Line 233, raw array values are stored as-is. Reusing
parseStringArrayhere avoids persisting empty/whitespace entries and keeps read/write normalization symmetric.♻️ Proposed change
if (Array.isArray(params.update.allowedChatIds)) { - telegram.allowedChatIds = [...params.update.allowedChatIds]; + telegram.allowedChatIds = parseStringArray(params.update.allowedChatIds); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/channelBridgeAccountConfig.ts` around lines 232 - 234, The write path currently assigns raw arrays from params.update.allowedChatIds directly to telegram.allowedChatIds; change it to normalize using the existing parseStringArray helper so empty/whitespace entries are removed and canonicalization mirrors reads—i.e., call parseStringArray(params.update.allowedChatIds) and assign the resulting array to telegram.allowedChatIds (use spread or direct assignment as appropriate) instead of persisting the raw input.
1-2: Add a file-level JSDoc header for scoped config responsibilities.This file now owns split/read/upsert/remove semantics for scoped bridge state, so a short top-level contract doc would help future changes stay consistent.
As per coding guidelines, "Include comprehensive JSDoc comments as file header comments explaining file responsibilities".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/channelBridgeAccountConfig.ts` around lines 1 - 2, Add a file-level JSDoc header at the top of this file describing its responsibility for managing scoped bridge state and the contract for split/read/upsert/remove semantics; the header should explain the scope of configuration handled here, expected inputs/outputs (e.g., keys and shape implied by the RecordLike type), concurrency/consistency expectations, and any invariants callers must maintain when using the split, read, upsert, and remove operations so future changes remain consistent with this contract.apps/cli/src/channels/channelBridgeServerKv.ts (2)
1-4: Add a file-level JSDoc header describing responsibilities.This module is a public integration boundary (KV transport + schema decode + optimistic writes), so a top-level header would improve maintainability.
📝 Proposed addition
+/** + * Channel bridge server-KV integration for Telegram config and bindings. + * Responsibilities: + * - Encode/decode versioned KV payloads + * - Read/write with optimistic concurrency handling + * - Parse and validate persisted schema documents + */ import axios from 'axios';As per coding guidelines, "Include comprehensive JSDoc comments as file header comments explaining file responsibilities".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/channelBridgeServerKv.ts` around lines 1 - 4, Add a file-level JSDoc header at the top of this module describing its responsibilities as the public KV transport/integration boundary: note it performs HTTP calls (uses axios), resolves the server base URL via resolveServerHttpBaseUrl, handles schema decoding, and implements optimistic writes; document what this module exports or side-effects, expected input/output shapes or schemas, error/ retry semantics, and any important invariants or security considerations so callers know how to use it and what guarantees it provides.
5-5: Use alias import instead of relative path for internal module.At Line 5, switch the local relative import to an
@/alias to keep import conventions consistent.♻️ Proposed change
-import type { ScopedTelegramBridgeUpdate } from './channelBridgeAccountConfig'; +import type { ScopedTelegramBridgeUpdate } from '@/channels/channelBridgeAccountConfig';As per coding guidelines, "Use
@/alias for src imports (e.g., import { logger } from '@/ui/logger')".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/channelBridgeServerKv.ts` at line 5, The import uses a local relative path for ScopedTelegramBridgeUpdate; update the import statement to use the project alias (e.g., change the path to the '@/...' alias) so it reads from the aliased module instead of a relative path — locate the import of ScopedTelegramBridgeUpdate in channelBridgeServerKv and replace the './channelBridgeAccountConfig' path with the corresponding '@/channels/channelBridgeAccountConfig' alias to match import conventions.
🤖 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/channelBridgeAccountConfig.ts`:
- Around line 122-124: The current check in hasSharedTelegramBridgeUpdate relies
on hasAnyUpdateValue (which currently treats any key presence as an update),
causing false positives for payloads like { webhookPort: undefined }; update
hasAnyUpdateValue to inspect the values (from the Record passed in) and return
true only if at least one value is actually present/defined (e.g., value !==
undefined — optionally also !== null or non-empty string per your domain rules).
Keep the call site in hasSharedTelegramBridgeUpdate the same but change the
implementation of hasAnyUpdateValue to iterate Object.values(update) and
validate real values rather than just keys.
In `@apps/cli/src/channels/channelBridgeServerKv.ts`:
- Around line 323-326: In createAxiosChannelBridgeKvClient, validate the
resolved baseUrl (from resolveServerHttpBaseUrl()) before using it: parse the
URL and allow only https: or hosts that are explicit localhost/127.0.0.1 (and
their IPv6 equivalents) for non-https; otherwise throw an error to fail closed
so bearer token/payload are never sent over plain http. Update the code paths
that construct the Axios client (using baseUrl and token) to perform this check
first and reject unsafe schemes/hosts.
In `@apps/cli/src/ui/doctor.ts`:
- Around line 329-337: The webhook required-field validation currently only
reads host/port from the account-scoped variables (providers/telegram) and thus
misses explicit host/port set at server or global scope; update the validation
to compute the effective webhook config by merging scopes with correct
precedence (account -> server -> global) so host/port are read from serverScope
(byServerId / serverScope?.byAccountId -> providers?.telegram) or
channelBridgeRoot/global when absent at account level, and then run the existing
webhook.enabled and required-field checks against that merged/effective config
(use the existing variables channelBridgeRoot, byServerId, serverScope,
byAccountId, accountScope, providers, telegram to locate and merge values).
---
Duplicate comments:
In `@apps/cli/src/daemon/startDaemon.ts`:
- Around line 1197-1245: The channel bridge startup (startChannelBridgeFromEnv +
readSettings/readChannelBridgeTelegramConfigFromKv/overlayServerKvTelegramConfigInSettings)
is currently executed inside the machine-registration try path so it is skipped
whenever registration fails; move the entire bridge initialization block out of
the machine-registration try/catch so it always runs regardless of registration
outcome, ensuring it still uses existing locals such as credentials and
configuration, and wrap the moved logic in its own try/catch that logs failures
(using logger.warn and serializeAxiosErrorForLog) and returns null on error to
preserve the previous best-effort behavior.
---
Nitpick comments:
In `@apps/cli/src/channels/channelBindingStore.server.test.ts`:
- Around line 6-120: The two functions createInMemoryKvClient and
createCountingKvClient duplicate the same get/mutate logic; extract that core
into a shared factory (e.g., makeBaseKvClient or buildKvClient) that
encapsulates the byKey map and returns get and mutate implementations, then
reimplement createInMemoryKvClient by returning the base client and reimplement
createCountingKvClient by composing the base client with a counter hook
(incrementing mutateCalls inside the shared mutate or by wrapping mutate) and
exposing mutateCallCount; keep the same return shapes and preserve semantics of
get, mutate, and mutateCallCount.
In `@apps/cli/src/channels/channelBridgeAccountConfig.ts`:
- Around line 232-234: The write path currently assigns raw arrays from
params.update.allowedChatIds directly to telegram.allowedChatIds; change it to
normalize using the existing parseStringArray helper so empty/whitespace entries
are removed and canonicalization mirrors reads—i.e., call
parseStringArray(params.update.allowedChatIds) and assign the resulting array to
telegram.allowedChatIds (use spread or direct assignment as appropriate) instead
of persisting the raw input.
- Around line 1-2: Add a file-level JSDoc header at the top of this file
describing its responsibility for managing scoped bridge state and the contract
for split/read/upsert/remove semantics; the header should explain the scope of
configuration handled here, expected inputs/outputs (e.g., keys and shape
implied by the RecordLike type), concurrency/consistency expectations, and any
invariants callers must maintain when using the split, read, upsert, and remove
operations so future changes remain consistent with this contract.
In `@apps/cli/src/channels/channelBridgeServerKv.ts`:
- Around line 1-4: Add a file-level JSDoc header at the top of this module
describing its responsibilities as the public KV transport/integration boundary:
note it performs HTTP calls (uses axios), resolves the server base URL via
resolveServerHttpBaseUrl, handles schema decoding, and implements optimistic
writes; document what this module exports or side-effects, expected input/output
shapes or schemas, error/ retry semantics, and any important invariants or
security considerations so callers know how to use it and what guarantees it
provides.
- Line 5: The import uses a local relative path for ScopedTelegramBridgeUpdate;
update the import statement to use the project alias (e.g., change the path to
the '@/...' alias) so it reads from the aliased module instead of a relative
path — locate the import of ScopedTelegramBridgeUpdate in channelBridgeServerKv
and replace the './channelBridgeAccountConfig' path with the corresponding
'@/channels/channelBridgeAccountConfig' alias to match import conventions.
In `@apps/cli/src/daemon/startDaemon.ts`:
- Around line 1398-1404: The Promise.race timeout created for
channelBridgeWorker.stop() isn't being cleared on the fast path; capture the
timeout handle (created with setTimeout and stored in a variable) and clear it
when the stop promise resolves so the delayed timeout callback is
cancelled—modify the race so the channelBridgeWorker.stop().then(...) handler
calls clearTimeout(timeoutHandle) before returning 'stopped' (keep using
channelBridgeStopTimeoutMs and the existing Promise.race/stopResult flow).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b472550-811a-4e4f-ab33-50ee0f9633eb
⛔ Files ignored due to path filters (3)
docs/assets/doctor-critical-summary.pngis excluded by!**/*.pngdocs/assets/doctor-missing-host-port-summary.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (32)
README.mdapps/cli/src/channels/channelBindingStore.server.test.tsapps/cli/src/channels/channelBindingStore.server.tsapps/cli/src/channels/channelBridgeAccountConfig.test.tsapps/cli/src/channels/channelBridgeAccountConfig.tsapps/cli/src/channels/channelBridgeConfig.test.tsapps/cli/src/channels/channelBridgeConfig.tsapps/cli/src/channels/channelBridgeServerConfigOverlay.test.tsapps/cli/src/channels/channelBridgeServerConfigOverlay.tsapps/cli/src/channels/channelBridgeServerKv.test.tsapps/cli/src/channels/channelBridgeServerKv.tsapps/cli/src/channels/core/channelBridgeWorker.test.tsapps/cli/src/channels/core/channelBridgeWorker.tsapps/cli/src/channels/startChannelBridgeWorker.startupFailure.test.tsapps/cli/src/channels/startChannelBridgeWorker.test.tsapps/cli/src/channels/startChannelBridgeWorker.tsapps/cli/src/channels/telegram/telegramAdapter.test.tsapps/cli/src/channels/telegram/telegramAdapter.tsapps/cli/src/channels/telegram/telegramWebhookRelay.test.tsapps/cli/src/channels/telegram/telegramWebhookRelay.tsapps/cli/src/cli/commandRegistry.tsapps/cli/src/cli/commands/bridge.test.tsapps/cli/src/cli/commands/bridge.tsapps/cli/src/cli/dispatch.tsapps/cli/src/daemon/startDaemon.tsapps/cli/src/persistence.tsapps/cli/src/ui/doctor.test.tsapps/cli/src/ui/doctor.tsdocs/README.mddocs/channel-bridge-uat.mddocs/channel-bridge.mddocs/telegram-channel-bridge.md
✅ Files skipped from review due to trivial changes (1)
- apps/cli/src/cli/commands/bridge.test.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/cli/src/cli/commandRegistry.ts
- docs/README.md
- apps/cli/src/channels/channelBridgeServerConfigOverlay.test.ts
- apps/cli/src/channels/channelBridgeServerConfigOverlay.ts
- apps/cli/src/channels/channelBindingStore.server.ts
- docs/channel-bridge-uat.md
- apps/cli/src/channels/telegram/telegramWebhookRelay.test.ts
- apps/cli/src/channels/startChannelBridgeWorker.test.ts
- apps/cli/src/cli/dispatch.ts
- apps/cli/src/channels/channelBridgeAccountConfig.test.ts
- README.md
- apps/cli/src/channels/telegram/telegramAdapter.test.ts
- apps/cli/src/persistence.ts
Author
|
@coderabbitai review |
Author
|
@greptile review |
✅ Actions performedReview triggered.
|
Comment on lines
+229
to
+237
| const requireTopics = runtimeConfig.telegram.requireTopics; | ||
|
|
||
| const webhookModeRequested = webhookEnabled && webhookSecret.length > 0; | ||
| let telegramAdapter = createTelegramChannelAdapter({ | ||
| botToken, | ||
| allowedChatIds, | ||
| requireTopics, | ||
| webhookMode: webhookModeRequested, | ||
| }); |
Contributor
There was a problem hiding this comment.
Silent polling fallback with no warning when webhook secret is missing
webhookModeRequested is false when webhookEnabled=true but webhookSecret is empty, so the adapter silently starts in polling mode. A user who sets webhook.enabled: true (e.g. via happier bridge telegram set --webhook-enabled true) but has not yet set the secret will get polling mode with no log entry indicating that their intended webhook config is being ignored. This can be especially confusing because happier doctor would flag the missing secret as a critical failure, yet the bridge would still appear to work (in polling mode).
Consider emitting a warning before creating the adapter:
Suggested change
| const requireTopics = runtimeConfig.telegram.requireTopics; | |
| const webhookModeRequested = webhookEnabled && webhookSecret.length > 0; | |
| let telegramAdapter = createTelegramChannelAdapter({ | |
| botToken, | |
| allowedChatIds, | |
| requireTopics, | |
| webhookMode: webhookModeRequested, | |
| }); | |
| const webhookModeRequested = webhookEnabled && webhookSecret.length > 0; | |
| if (webhookEnabled && !webhookModeRequested) { | |
| logger.warn( | |
| '[channelBridge] webhook.enabled=true but webhook.secret is empty — starting Telegram adapter in polling mode. Set a webhook secret to enable webhook relay.', | |
| ); | |
| } | |
| let telegramAdapter = createTelegramChannelAdapter({ |
Comment on lines
+106
to
+118
| deps: { | ||
| listSessions: async () => { | ||
| const page = await fetchSessionsPage({ token: credentials.token, activeOnly: false, limit: 100 }); | ||
| return page.sessions.map((row) => { | ||
| const meta = tryDecryptSessionMetadata({ credentials, rawSession: row }); | ||
| const tagRaw = meta?.tag; | ||
| const tag = typeof tagRaw === 'string' ? tagRaw.trim() : ''; | ||
| return { | ||
| sessionId: row.id, | ||
| label: tag.length > 0 ? tag : null, | ||
| }; | ||
| }); | ||
| }, |
Contributor
There was a problem hiding this comment.
listSessions is bounded to 100 sessions
fetchSessionsPage is called with a hard-coded limit: 100, so any sessions beyond the first 100 returned by the API are invisible to the bridge worker. If the bridge worker relies on listSessions to resolve session identifiers during /sessions, /attach, or /session commands, users with more than 100 sessions may find their later sessions unreachable through the channel bridge with no error or indication.
Consider increasing the limit toward the API maximum or implementing multi-page fetching to ensure all sessions are covered, so the bridge doesn't silently omit sessions for accounts with larger session histories.
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.
This PR is stacked on top of PR1 (channel bridge core).
Please review against base branch:
feat/channel-bridge-core.Summary
This PR adds Telegram support on top of the provider-agnostic channel bridge core, including:
What’s Included
Telegram adapter + runtime wiring
apps/cli/src/channels/telegram/telegramAdapter.tsapps/cli/src/channels/telegram/telegramAdapter.test.tsapps/cli/src/channels/telegram/telegramWebhookRelay.tsapps/cli/src/channels/telegram/telegramWebhookRelay.test.tsapps/cli/src/channels/startChannelBridgeWorker.tsapps/cli/src/channels/startChannelBridgeWorker.test.tsScoped bridge configuration + persistence
apps/cli/src/channels/channelBridgeAccountConfig.tsapps/cli/src/channels/channelBridgeConfig.tsapps/cli/src/channels/channelBridgeServerKv.tsapps/cli/src/channels/channelBridgeServerConfigOverlay.tsapps/cli/src/channels/channelBindingStore.server.tsCLI management
happier bridgecommand group:apps/cli/src/cli/commands/bridge.tsapps/cli/src/cli/commands/bridge.test.tsapps/cli/src/cli/commandRegistry.tsapps/cli/src/cli/dispatch.tsSecurity / Secret Handling
Implemented local-only secret policy for bridge credentials:
botToken/webhookSecretstay local (settings.json/ env)This establishes the same pattern for future adapters (Discord/Slack/WhatsApp/etc).
Doctor Diagnostics Improvements
Updated doctor behavior to surface bridge health as real pass/fail:
✅ Doctor diagnosis complete!only when no critical failures❌ Doctor diagnosis complete!when any critical failure existswebhook.enabled=true:webhook.secretwebhook.hostwebhook.portDocumentation
docs/telegram-channel-bridge.mddocs/channel-bridge-uat.mdREADME.mdScreenshot evidence
Tests
Added/updated focused tests across:
Representative files:
apps/cli/src/channels/channelBridgeAccountConfig.test.tsapps/cli/src/channels/channelBridgeConfig.test.tsapps/cli/src/channels/channelBridgeServerKv.test.tsapps/cli/src/channels/channelBridgeServerConfigOverlay.test.tsapps/cli/src/channels/channelBindingStore.server.test.tsapps/cli/src/channels/startChannelBridgeWorker.test.tsapps/cli/src/channels/telegram/telegramAdapter.test.tsapps/cli/src/channels/telegram/telegramWebhookRelay.test.tsapps/cli/src/cli/commands/bridge.test.tsapps/cli/src/ui/doctor.test.tsSuggestions:
Summary by CodeRabbit
New Features
Documentation
Improvements
Tests