feat(cli): add telegram channel adapter, scoped config, and diagnostics (replacement)#115
feat(cli): add telegram channel adapter, scoped config, and diagnostics (replacement)#115cdenneen wants to merge 31 commits intohappier-dev:devfrom
Conversation
|
@coderabbitai review |
|
@greptile review |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Channel Bridge subsystem: provider-agnostic worker runtime, Telegram adapter and webhook relay, server KV transport and cache-backed binding store, scoped account config and overlays, CLI and daemon wiring, diagnostics/redaction utilities, extensive tests, and documentation including a Telegram guide and README "Channel Integrations" entry. Changes
Sequence Diagram(s)sequenceDiagram
participant Adapter as Telegram Adapter
participant Worker as Channel Bridge Worker
participant Store as Binding Store
participant KV as Server KV
participant Session as Session Service
rect rgba(0, 100, 200, 0.5)
Note over Worker: Tick begins
Adapter->>Worker: pullInboundMessages()
Worker->>Worker: deduplicateInbound()
alt Slash command (attach/session)
Worker->>Store: upsertBinding(conversation→session)
Store->>KV: write(binding)
else User message
Worker->>Store: getBinding(conversation)
Store->>KV: read(binding)
Worker->>Session: forwardUserMessage(sessionId, text)
Session-->>Worker: responseRows
Worker->>Adapter: sendMessage(conversation, agentText)
Adapter-->>Worker: deliveryAck
Worker->>Store: updateLastForwardedSeq(binding)
Store->>KV: write(binding.lastForwardedSeq)
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Greptile SummaryThis PR adds a full Telegram Channel Bridge to the Happier CLI: a provider-agnostic bridge worker ( The implementation is substantial (~12k lines added) and has received extensive prior review. Many hard correctness issues — permanent-delivery cursor stalls, reply-guard hardening, surrogate-safe truncation, typed error contracts, loopback validation at every write path, and daemon shutdown races — were addressed across multiple fix commits. Three issues remain unaddressed:
Confidence Score: 2/5
Last reviewed commit: 74ae08b |
![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: 11
🧹 Nitpick comments (9)
apps/cli/src/channels/startChannelBridgeWorker.startupFailure.test.ts (1)
139-142: Loosen this warning assertion to a stable substring/prefix.This pins the full log copy, so harmless rewording will fail the test even if shutdown behavior is still correct. Assert on the error object plus a key substring or prefix instead. As per coding guidelines, "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. Prefer stable identifiers, error codes/types, shapes, statuses, and key substrings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/startChannelBridgeWorker.startupFailure.test.ts` around lines 139 - 142, The test pins the full log message in the assertion for warnSpy; replace the exact-string assertion in the expect(warnSpy).toHaveBeenCalledWith(...) call with a looser check that verifies the Error object and a stable substring or prefix (e.g., use expect.stringContaining('Error stopping webhook relay') or a regex like expect.stringMatching(/^\\[channelBridge\\] Error stopping webhook relay/)) so the assertion still checks the right log intent without brittle copy-level matching; update the assertion in the startupFailure test that currently references warnSpy accordingly.apps/cli/src/persistence.ts (1)
99-102: ReplacechannelBridge: unknownwith a typed settings envelope.This persisted subtree is now a first-class part of
Settings, and leaving it asunknownis already forcing downstreamas anyaccess in the new channel-bridge tests/helpers. Even a narrow interface for the top-level shape would keep settings reads/writes type-safe without freezing every provider-specific field. As per coding guidelines, "No untyped code in TypeScript - use strict typing with explicit parameter and return types" and "Preferinterfaceovertypefor 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/persistence.ts` around lines 99 - 102, The channelBridge field currently typed as unknown should be replaced with a concrete settings envelope interface to avoid downstream any casts: define an interface (e.g., ChannelBridgeSettingsEnvelope) describing the top-level persisted shape (optional provider-specific nested fields allowed) and use it for the channelBridge property on the Settings/persistence type instead of unknown; update any reads/writes (places referencing channelBridge) to use the new interface so TypeScript enforces proper typing while still allowing provider-specific optional entries.apps/cli/src/channels/startChannelBridgeWorker.test.ts (1)
440-442: Assert the warning by stable substrings, not the full sentence.This locks the test to user-facing copy, so harmless wording tweaks will fail it. Matching the invariant fragments is enough to keep the fallback behavior covered.
♻️ Suggested assertion
- expect(warnSpy).toHaveBeenCalledWith( - '[channelBridge] Telegram webhook.enabled=true but webhook.secret is missing; falling back to polling mode', - ); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringMatching( + /webhook\.enabled=true.*webhook\.secret is missing.*falling back to polling mode/i, + ), + );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. Prefer stable identifiers, error codes/types, shapes, statuses, and key substrings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/startChannelBridgeWorker.test.ts` around lines 440 - 442, The test currently asserts the full warning message string via warnSpy; change it to assert only stable substrings so wording tweaks don't break the test—update the assertion in startChannelBridgeWorker.test to use jest matchers (e.g. expect.stringContaining) with key fragments such as 'Telegram', 'webhook.enabled=true', and 'webhook.secret' when asserting warnSpy instead of the entire sentence.apps/cli/src/channels/channelBridgeConfig.ts (1)
1-1: Add the missing file/API JSDoc for the precedence rules.This module is now the public entry point for env/global/server/account channel-bridge resolution, but the file and exported resolver do not document that merge order or the runtime defaults. A short header plus resolver doc block would make future config changes much safer.
As per coding guidelines "Include comprehensive JSDoc comments as file header comments explaining file responsibilities" and "Add JSDoc comments for public APIs and complex logic".
Also applies to: 85-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/channelBridgeConfig.ts` at line 1, Add a short file-level JSDoc header to channelBridgeConfig.ts describing its responsibility as the public entry point for env/global/server/account channel-bridge resolution and listing the merge/precedence rules and runtime defaults; then add a JSDoc block above the exported resolver function (resolveChannelBridgeConfig) documenting the exact merge order (env > global > server > account or whatever the implementation uses), expected input shape (referencing the RecordLike type), return shape, and any default behavior for missing keys, so future changes to precedence are clear and discoverable.apps/cli/src/channels/channelBridgeServerConfigOverlay.ts (1)
18-23: Add JSDoc to the exported overlay helper.This function bakes in a non-obvious merge contract—server KV overlays only non-secret Telegram fields and may intentionally return the original settings object. A short JSDoc would make that behavior much easier to preserve.
As per coding guidelines "Add JSDoc comments for public APIs and complex logic".Suggested doc block
+/** + * Overlays server-KV Telegram fields onto scoped local settings. + * Secrets remain local-only; if the record has no usable non-secret fields, + * the original settings object is returned unchanged. + */ export function overlayServerKvTelegramConfigInSettings(params: Readonly<{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/channelBridgeServerConfigOverlay.ts` around lines 18 - 23, Add a concise JSDoc to the exported helper overlayServerKvTelegramConfigInSettings describing its merge contract: explain that it overlays only non-secret Telegram fields from ChannelBridgeServerTelegramConfigRecord into the provided settings, that it may return the original settings object unchanged when no overlay is applied, and document the params (settings, serverId, accountId, record) and the unknown return type so callers understand side-effects and when identity is preserved. Include a short note about secrecy handling (secret fields are intentionally not merged) and reference the function name and ChannelBridgeServerTelegramConfigRecord type.apps/cli/src/cli/commands/bridge.test.ts (1)
55-57: Persist the updated settings state in this harness.This mock calls the updater and throws its return value away, so the suite never checks the scoped settings tree that
bridge.tswrites and falls back to asserting mock call order instead. Keep an in-memorysettingsStatehere and assert the resulting object after each command.Suggested harness tweak
+let settingsState: unknown; + beforeEach(async () => { vi.resetModules(); vi.clearAllMocks(); + settingsState = {}; … - updateSettingsMock.mockImplementation(async (updater: (current: unknown) => Promise<unknown> | unknown) => { - await updater({}); - }); + updateSettingsMock.mockImplementation(async (updater: (current: unknown) => Promise<unknown> | unknown) => { + settingsState = await updater(settingsState); + return settingsState; + });Based on learnings "Assert on observable outcomes (return values, state changes, HTTP responses), not internal call sequences."
🤖 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 55 - 57, The mock updateSettingsMock currently calls the updater and discards its return so the test never observes the bridge.ts settings changes; create an in-memory settingsState in the test harness and change updateSettingsMock.mockImplementation to call updater with the current settingsState, await its result, and assign the returned value back to settingsState (or merge if updater mutates), then update assertions to check settingsState after each command invocation instead of relying on mock call order—refer to updateSettingsMock, updater, settingsState and the bridge.ts behavior when making the change.apps/cli/src/channels/channelBridgeServerKv.ts (3)
510-548: Add backoff delay between retry attempts.The retry loop has no delay between attempts. Under contention, this could cause rapid-fire retries where multiple concurrent writers repeatedly collide. Adding exponential or jittered backoff would improve resilience.
Proposed fix with exponential backoff
+const sleep = (ms: number): Promise<void> => new Promise((resolve) => setTimeout(resolve, ms)); + export async function upsertChannelBridgeTelegramConfigInKv(params: Readonly<{ kv: ChannelBridgeKvClient; serverId: string; update: ScopedTelegramBridgeUpdate; }>): Promise<void> { if (!hasNonSecretTelegramConfigUpdate(params.update)) { return; } const key = telegramConfigKvKey(params.serverId); let current: Readonly<{ record: ChannelBridgeServerTelegramConfigRecord | null; version: number; }> | null = null; for (let attempt = 0; attempt < 4; attempt += 1) { + if (attempt > 0) { + await sleep(50 * Math.pow(2, attempt - 1)); // 50ms, 100ms, 200ms + } if (current === null) {🤖 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 510 - 548, The retry loop in upserting the Telegram config (the for-loop that calls readChannelBridgeTelegramConfigFromKv, applyTelegramConfigUpdate, and writeJsonValue) needs an exponential-with-jitter backoff between attempts to avoid hot-looping on ChannelBridgeKvVersionMismatchError; after catching a ChannelBridgeKvVersionMismatchError and updating current (inside the catch branch that sets conflictRecord/current), compute a delay based on attempt (e.g., baseDelay * 2**attempt with random jitter) and await that delay before continuing the loop, ensuring you do not block the event loop (use a promise-based sleep) and keep the maximum delay capped to a sensible upper bound.
564-583: Apply the same backoff pattern here.This retry loop has the same no-backoff concern as
upsertChannelBridgeTelegramConfigInKv. Apply consistent backoff logic across both retry loops.🤖 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 564 - 583, The retry loop that calls writeJsonValue should include the same exponential/backoff delay used in upsertChannelBridgeTelegramConfigInKv: when a transient failure occurs (i.e., in the catch branch that does not throw ChannelBridgeKvVersionMismatchError), wait before retrying, and increase delay per attempt; keep the existing special-case handling for ChannelBridgeKvVersionMismatchError (return when currentVersion < 0 and update expectedVersion otherwise). Update the loop around writeJsonValue to sleep/backoff between attempts (using the same backoff helper or pattern used by upsertChannelBridgeTelegramConfigInKv) so retries are spaced instead of immediate.
21-70: Consider usinginterfaceinstead oftypefor object shapes.Per coding guidelines,
interfaceis preferred overtypefor defining object shapes. This provides better error messages and supports declaration merging when needed.Example conversion for one type
-export type ChannelBridgeServerTelegramConfigRecord = Readonly<{ - schemaVersion: 1; - tickMs?: number; - telegram: Readonly<{ - allowedChatIds?: string[]; - requireTopics?: boolean; - webhook?: Readonly<{ - enabled?: boolean; - host?: string; - port?: number; - }>; - }>; - updatedAtMs: number; -}>; +export interface ChannelBridgeServerTelegramConfigRecord { + readonly schemaVersion: 1; + readonly tickMs?: number; + readonly telegram: Readonly<{ + allowedChatIds?: string[]; + requireTopics?: boolean; + webhook?: Readonly<{ + enabled?: boolean; + host?: string; + port?: number; + }>; + }>; + readonly updatedAtMs: number; +}🤖 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 21 - 70, Several exported object-shaped type aliases should be converted to interfaces per the coding guideline; replace the type aliases ChannelBridgeServerTelegramConfigRecord, ChannelBridgeServerBindingRecord, ChannelBridgeServerBindingsDocument, ChannelBridgeKvGetResponse, ChannelBridgeKvMutateResponse, ChannelBridgeKvMutation, and ChannelBridgeKvClient with equivalent interface declarations, preserving readonly fields and optional properties (use readonly modifiers and ? where used) and keep method signatures exactly for get/mutate on ChannelBridgeKvClient; ensure exported names and structural shape remain identical so callers are unaffected.
🤖 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 229-240: The code unconditionally calls ensureRecord() for
providers and telegram, materializing a Telegram node even for tickMs-only
updates; update the logic in the channelBridge/account update path (around
channelBridge, byServerId, serverScope, byAccountId, accountScope) to only
create the providers subtree (and the telegram record) when the incoming
params.update contains Telegram-related fields (e.g., token or other provider
properties) or otherwise leave providers undefined; alternatively ensure
readScopedTelegramBridgeConfig() treats an empty providers.telegram node as
unconfigured—preferably perform a check for Telegram fields in params.update
before calling ensureRecord(providers,'telegram') so tickMs-only updates do not
create the provider node.
In `@apps/cli/src/channels/startChannelBridgeWorker.ts`:
- Around line 196-219: Both resolveLatestSessionSeq and
fetchAgentMessagesAfterSeq assume encrypted transcripts by calling
fetchEncryptedTranscriptPageLatest / fetchEncryptedTranscriptPageAfterSeq and
decryptTranscriptRows; change them to branch on the session's runtime.mode (use
resolveSessionContext(sessionId) or ctx.runtime.mode) and: for mode 'encrypted'
keep the current encrypted fetch+decrypt path, but for mode 'plain' call the
plain transcript fetchers (or the non-encrypted equivalents) and skip
decryptTranscriptRows — directly filter/map content.t/plain messages and
extractAssistantText. Ensure resolveLatestSessionSeq also acquires the session
context first and uses the mode to pick the correct fetch function and seq
parsing so plain sessions are handled without decryption.
In `@apps/cli/src/channels/telegram/telegramAdapter.ts`:
- Around line 176-185: The process-local update cursor (updateOffset) and the
5-minute TTL deduper (createChannelBridgeInboundDeduper) leave a window where
Telegram can replay updates after daemon restarts; persist and restore the
polling cursor or extend the deduper TTL to match Telegram's retention. Modify
the Telegram adapter to store updateOffset (and selfBotId if needed) to durable
storage on each successful poll/ack and reload it on startup, or increase the
deduper TTL in createChannelBridgeInboundDeduper to cover Telegram's retention
window (whichever strategy you choose), ensuring the adapter uses the persisted
offset before calling getUpdates so duplicates are not reprocessed after
restarts.
In `@apps/cli/src/channels/telegram/telegramWebhookRelay.ts`:
- Around line 33-36: The code currently sets secretHeaderToken =
String(params.secretHeaderToken ?? secretPathToken).trim(), which falls back to
the path token and nullifies the header check; change it to require an explicit
header token by using only params.secretHeaderToken (e.g.,
String(params.secretHeaderToken).trim()) and throw the existing Error if it's
empty, or alternatively derive/store a distinct header secret instead of reusing
secretPathToken; update references to secretHeaderToken,
params.secretHeaderToken, and secretPathToken accordingly so the header cannot
be satisfied by leaking the path token.
In `@apps/cli/src/cli/commands/bridge.ts`:
- Around line 257-275: The local settings are being persisted (via
updateSettings + upsertScopedTelegramBridgeConfig) before the server KV write
completes, causing possible divergence because split.localUpdate still contains
shared fields; change the flow to first perform the shared KV operation (use
hasSharedTelegramBridgeUpdate -> createAxiosChannelBridgeKvClient ->
upsertChannelBridgeTelegramConfigInKv) and only after it succeeds perform
updateSettings with a sanitized local-only payload (derive local-only update
from split via splitScopedTelegramBridgeUpdate or strip shared fields), and add
a compensating rollback: if the subsequent updateSettings fails after a
successful KV write, retry the settings write and if it continues to fail,
revert the KV change (or mark a reconciliation flag) so server and local remain
consistent; apply same change to the other block referenced (lines ~297-309).
In `@apps/cli/src/cli/dispatch.ts`:
- Around line 49-50: The guard that blocks disallowed commands uses subcommand
=== args[0] which fails when global flags like --tmux appear first; update the
logic that constructs subcommand (or add a small normalization step before the
disallowed check) to strip global flags/options from args and find the first
non-flag token (e.g., first arg not starting with '-' or matched against known
global flags) and assign that to subcommand before testing against the
disallowed Set; change the check around the disallowed Set (the variable named
disallowed and the existing if (subcommand && disallowed.has(subcommand))) to
use the normalized/resolved command so "bridge" is caught even when "--tmux" is
passed first.
In `@apps/cli/src/daemon/startDaemon.ts`:
- Around line 1154-1205: Current bridge startup (calls to readSettings,
readChannelBridgeTelegramConfigFromKv and startChannelBridgeFromEnv) runs
synchronously on the daemon readiness path and can block
ensureMachineRegistered; move the bridge bootstrap off the critical path by
invoking it after ensureMachineRegistered or as a fire-and-forget background
task with its own short timeout and error handling: remove awaits that block
readiness and instead spawn a detached promise (or schedule via setTimeout) that
performs readSettings, optional KV fetch (readChannelBridgeTelegramConfigFromKv)
and startChannelBridgeFromEnv while logging errors via
serializeAxiosErrorForLog; keep the same inputs (credentials, serverId,
accountId, settings) and preserve the existing catch/log-return-null logic
inside that detached task so daemon registration and writing of
daemon.state.json are not gated on bridge startup.
In `@apps/cli/src/ui/doctor.ts`:
- Around line 130-167: The redactSettingsForDisplay function only redacts
providers under channelBridge.byServerId[*].byAccountId[*].providers; extend it
to also walk and redact provider entries in channelBridge.providers (global
scope) and channelBridge.byServerId[*].providers (server scope) using the same
logic that replaces secrets.*, botToken, and webhook.secret with '<redacted>'
(refer to channelBridge, byServerId, byAccountId, providers, secrets, botToken,
webhook.secret to locate code paths), and add regression tests that exercise
global (channelBridge.providers) and server-scoped
(channelBridge.byServerId[*].providers) provider redaction so the CLI output
never prints raw credentials.
In `@docs/channel-bridge-uat.md`:
- Around line 83-85: Update the webhook auth section to explicitly document both
failure statuses: state that a header-token (Authorization or custom header)
mismatch returns 401, while an incorrect path token (path-based secret) returns
404; replace the phrase "typically 404 with path-token mismatch" with clear
sentences distinguishing the two cases and keep the existing steps about
changing allowedChatIds and using --allow-all unchanged.
In `@docs/channel-bridge.md`:
- Around line 53-57: The phrase "follow-up stacked PRs" is stale; update the
paragraph under the "Out of scope for this PR" section to remove that wording
and instead point readers to the companion docs that now cover the Telegram
adapter, scoped config sources, and bootstrapping/runtime details; specifically,
replace the sentence referencing "follow-up stacked PRs" with a concise pointer
to the companion documentation (or list the included topics) so readers know
where to find transport details, bridge runtime bootstrapping, scoped config
sources, and server-relay deployment modes.
In `@docs/README.md`:
- Around line 12-13: The docs index (README) is missing the new UAT guide entry;
update the docs index list in README.md to include "channel-bridge-uat.md"
alongside the existing entries (e.g., near "channel-bridge.md" and
"telegram-channel-bridge.md") so the new UAT validation flow is discoverable by
readers.
---
Nitpick comments:
In `@apps/cli/src/channels/channelBridgeConfig.ts`:
- Line 1: Add a short file-level JSDoc header to channelBridgeConfig.ts
describing its responsibility as the public entry point for
env/global/server/account channel-bridge resolution and listing the
merge/precedence rules and runtime defaults; then add a JSDoc block above the
exported resolver function (resolveChannelBridgeConfig) documenting the exact
merge order (env > global > server > account or whatever the implementation
uses), expected input shape (referencing the RecordLike type), return shape, and
any default behavior for missing keys, so future changes to precedence are clear
and discoverable.
In `@apps/cli/src/channels/channelBridgeServerConfigOverlay.ts`:
- Around line 18-23: Add a concise JSDoc to the exported helper
overlayServerKvTelegramConfigInSettings describing its merge contract: explain
that it overlays only non-secret Telegram fields from
ChannelBridgeServerTelegramConfigRecord into the provided settings, that it may
return the original settings object unchanged when no overlay is applied, and
document the params (settings, serverId, accountId, record) and the unknown
return type so callers understand side-effects and when identity is preserved.
Include a short note about secrecy handling (secret fields are intentionally not
merged) and reference the function name and
ChannelBridgeServerTelegramConfigRecord type.
In `@apps/cli/src/channels/channelBridgeServerKv.ts`:
- Around line 510-548: The retry loop in upserting the Telegram config (the
for-loop that calls readChannelBridgeTelegramConfigFromKv,
applyTelegramConfigUpdate, and writeJsonValue) needs an exponential-with-jitter
backoff between attempts to avoid hot-looping on
ChannelBridgeKvVersionMismatchError; after catching a
ChannelBridgeKvVersionMismatchError and updating current (inside the catch
branch that sets conflictRecord/current), compute a delay based on attempt
(e.g., baseDelay * 2**attempt with random jitter) and await that delay before
continuing the loop, ensuring you do not block the event loop (use a
promise-based sleep) and keep the maximum delay capped to a sensible upper
bound.
- Around line 564-583: The retry loop that calls writeJsonValue should include
the same exponential/backoff delay used in
upsertChannelBridgeTelegramConfigInKv: when a transient failure occurs (i.e., in
the catch branch that does not throw ChannelBridgeKvVersionMismatchError), wait
before retrying, and increase delay per attempt; keep the existing special-case
handling for ChannelBridgeKvVersionMismatchError (return when currentVersion < 0
and update expectedVersion otherwise). Update the loop around writeJsonValue to
sleep/backoff between attempts (using the same backoff helper or pattern used by
upsertChannelBridgeTelegramConfigInKv) so retries are spaced instead of
immediate.
- Around line 21-70: Several exported object-shaped type aliases should be
converted to interfaces per the coding guideline; replace the type aliases
ChannelBridgeServerTelegramConfigRecord, ChannelBridgeServerBindingRecord,
ChannelBridgeServerBindingsDocument, ChannelBridgeKvGetResponse,
ChannelBridgeKvMutateResponse, ChannelBridgeKvMutation, and
ChannelBridgeKvClient with equivalent interface declarations, preserving
readonly fields and optional properties (use readonly modifiers and ? where
used) and keep method signatures exactly for get/mutate on
ChannelBridgeKvClient; ensure exported names and structural shape remain
identical so callers are unaffected.
In `@apps/cli/src/channels/startChannelBridgeWorker.startupFailure.test.ts`:
- Around line 139-142: The test pins the full log message in the assertion for
warnSpy; replace the exact-string assertion in the
expect(warnSpy).toHaveBeenCalledWith(...) call with a looser check that verifies
the Error object and a stable substring or prefix (e.g., use
expect.stringContaining('Error stopping webhook relay') or a regex like
expect.stringMatching(/^\\[channelBridge\\] Error stopping webhook relay/)) so
the assertion still checks the right log intent without brittle copy-level
matching; update the assertion in the startupFailure test that currently
references warnSpy accordingly.
In `@apps/cli/src/channels/startChannelBridgeWorker.test.ts`:
- Around line 440-442: The test currently asserts the full warning message
string via warnSpy; change it to assert only stable substrings so wording tweaks
don't break the test—update the assertion in startChannelBridgeWorker.test to
use jest matchers (e.g. expect.stringContaining) with key fragments such as
'Telegram', 'webhook.enabled=true', and 'webhook.secret' when asserting warnSpy
instead of the entire sentence.
In `@apps/cli/src/cli/commands/bridge.test.ts`:
- Around line 55-57: The mock updateSettingsMock currently calls the updater and
discards its return so the test never observes the bridge.ts settings changes;
create an in-memory settingsState in the test harness and change
updateSettingsMock.mockImplementation to call updater with the current
settingsState, await its result, and assign the returned value back to
settingsState (or merge if updater mutates), then update assertions to check
settingsState after each command invocation instead of relying on mock call
order—refer to updateSettingsMock, updater, settingsState and the bridge.ts
behavior when making the change.
In `@apps/cli/src/persistence.ts`:
- Around line 99-102: The channelBridge field currently typed as unknown should
be replaced with a concrete settings envelope interface to avoid downstream any
casts: define an interface (e.g., ChannelBridgeSettingsEnvelope) describing the
top-level persisted shape (optional provider-specific nested fields allowed) and
use it for the channelBridge property on the Settings/persistence type instead
of unknown; update any reads/writes (places referencing channelBridge) to use
the new interface so TypeScript enforces proper typing while still allowing
provider-specific optional entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 759a4d35-8071-4de3-9196-26ffee860ae8
⛔ 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 (33)
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.automation.integration.test.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
|
Addressed the latest review items in commit 8514428. Highlights:
Targeted regression tests passed locally:
@coderabbitai review |
|
@greptile review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
|
Added a follow-up fix in commit 1714afa to address daemon startup coupling feedback. Change:
Validation rerun after this patch:
All passed. @coderabbitai review |
|
@greptile review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
|
Addressed the latest Greptile findings in commit a264754 and resolved the corresponding threads. Summary:
@coderabbitai review |
|
@greptile review |
|
Triggering an incremental review of commit 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
apps/cli/src/cli/commands/bridge.ts (1)
266-282:⚠️ Potential issue | 🟠 MajorHandle the server-ahead-of-local rollback path too.
The ordering fix removed the local-ahead-of-server case, but these flows still diverge if the KV mutation succeeds and the subsequent
updateSettings()call fails. Capture the previous KV state and revert it, or persist an explicit reconciliation marker, before returning an error.Also applies to: 304-316
🤖 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 266 - 282, When performing the KV upsert in the branch guarded by hasSharedTelegramBridgeUpdate (using createAxiosChannelBridgeKvClient and upsertChannelBridgeTelegramConfigInKv) you must capture the previous KV value before writing and, if the subsequent updateSettings call that runs upsertScopedTelegramBridgeConfig fails, revert the KV to that previous value (or write an explicit reconciliation marker) and then propagate the error; apply the same pattern in the other similar block that does a KV upsert followed by updateSettings so you avoid the server-ahead-of-local divergence.
🧹 Nitpick comments (2)
apps/cli/src/cli/commands/bridge.test.ts (1)
19-38: Run at least one bridge-command happy path against real persistence.Right now the suite mostly proves mocked call ordering. Because
@/persistenceis stubbed, it will not catch regressions in the actualsettings.jsonshape or local write behavior that this PR changes. Since you already create a tempHAPPIER_HOME_DIR, one higher-fidelity test using the real persistence module would give much better coverage here.Based on learnings "The ChannelBridgeKvClient is treated as a true system boundary (network/HTTP KV backend), so local in-memory doubles are acceptable here for deterministic unit tests under CI runtime constraints."
Also applies to: 46-68
🤖 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 19 - 38, Remove the '@/persistence' mock (the vi.mock that stubs readCredentials, readSettings, updateSettings) so the test uses the real persistence module and exercise at least one bridge command happy-path against the temp HAPPIER_HOME_DIR already created in the spec; keep the ChannelBridgeKvClient-related mocks (createAxiosChannelBridgeKvClient, upsertChannelBridgeTelegramConfigInKv, clearChannelBridgeTelegramConfigInKv, readChannelBridgeTelegramConfigFromKv) as-is so the KV boundary remains mocked, and ensure the test asserts actual settings.json reads/writes by invoking the code paths that call readSettings/updateSettings (e.g., the bridge command execution flow) to validate real persistence behavior.apps/cli/src/channels/channelBridgeAccountConfig.ts (1)
46-64: Promote the normalized scoped config to a named interface.The exported update aliases, the accumulator literals in
splitScopedTelegramBridgeUpdate(), and theRecordLike | nullreturn fromreadScopedTelegramBridgeConfig()are all describing closely related parts of the same public config surface with anonymous shapes. AScopedTelegramBridgeConfiginterface (plus a nested webhook interface) would make downstream usage type-safe and reduce drift between the read/split paths.As per coding guidelines, "Prefer
interfaceovertypefor defining object shapes in TypeScript" and "Always use types for function parameters and return values in TypeScript."Also applies to: 74-91, 136-205
🤖 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 46 - 64, The update and shared types (ScopedTelegramBridgeUpdate, SharedTelegramBridgeUpdate) and the anonymous accumulator/return shapes used in splitScopedTelegramBridgeUpdate() and readScopedTelegramBridgeConfig() should be promoted to a named interface (e.g., ScopedTelegramBridgeConfig with a nested WebhookConfig interface) so downstream code gets a single authoritative shape; change the exported type aliases to interfaces, replace the anonymous accumulator literals in splitScopedTelegramBridgeUpdate() with the new interface types, and update readScopedTelegramBridgeConfig() to return the named interface (or null) and use the named webhook interface for webhook fields to keep type-safety and consistency across the read/split paths.
🤖 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/channelBridgeConfig.ts`:
- Around line 45-52: parseStringArray currently turns an input like
[-1001234567890] into [] which the runtime treats as "allow all"; update
parseStringArray so it preserves numeric chat ids instead of discarding them: in
the mapping step, accept finite numbers by coercing them to strings (e.g., if
typeof entry === 'number' && Number.isFinite(entry) -> String(entry)), keep
trimming for string entries, and treat other types as invalid; then filter out
empty strings and return null only for completely invalid inputs (or keep
returning [] only when the final filtered array is truly empty), ensuring
parseStringArray (and any consumer of allowedChatIds) no longer collapses a
non-empty invalid array into the allow-all sentinel.
In `@apps/cli/src/channels/channelBridgeServerKv.ts`:
- Around line 95-98: The JSON parse in decodeBase64ToJson currently lets
malformed Base64 payloads throw raw SyntaxErrors; update decodeBase64ToJson to
catch Buffer/JSON.parse errors and throw or return a well-typed, recoverable
error (e.g., throw new ChannelBridgePayloadError or return null) so callers like
readChannelBridgeTelegramConfigFromKv (with allowUnsupportedSchema: true),
upsertChannelBridgeTelegramConfigInKv, clearChannelBridgeTelegramConfigInKv and
the bindings decoder can detect corrupted KV payloads and handle them (treat as
recoverable / overwriteable) instead of aborting; ensure the bindings decoder
maps the caught parse error to the channel-bridge payload error type so upstream
code can proceed with recovery.
- Around line 229-266: The parser is currently fabricating defaults for required
persisted fields (lastForwardedSeq, createdAtMs, updatedAtMs) instead of
failing; change the validation in the parsing logic so that if lastForwardedSeq,
createdAtMs, or updatedAtMs are missing or not finite numbers you throw
ChannelBridgeBadPayloadError (do not default to 0 or Date.now()). Keep the
existing type/finite checks but add presence checks (!== undefined) and remove
the fallbacks when building lastForwardedSeq, createdAtMs, and updatedAtMs so
the constructor/record creation for ChannelBridgeServerBindingRecord fails fast
on incomplete rows. Use the same error messages you already create for invalid
fields.
In `@apps/cli/src/channels/telegram/telegramAdapter.ts`:
- Around line 223-245: pullInboundMessages currently removes webhook items from
queuedWebhookUpdates and advances updateOffset as soon as parsing succeeds,
which causes drops if downstream handling fails; change it to only parse and
return the updates (including their original ids or raw update objects) without
mutating queuedWebhookUpdates or updating updateOffset, and add a separate
acknowledgment path (e.g., a new ackInboundMessages(ids) or have the caller call
a provided confirm function) that the worker invokes after attach/binding and
sendUserMessageToSession succeed; reference pullInboundMessages, parseUpdates,
queuedWebhookUpdates, updateOffset, parseHighestUpdateOffset, and
sendUserMessageToSession when making this change so consumption only happens
after successful processing.
In `@apps/cli/src/cli/commands/bridge.ts`:
- Around line 233-237: The current branch sets update.allowedChatIds = []
(meaning "allow all") when parseCsvList(allowedChatIdsRaw) returns an empty
array, which can happen for malformed inputs like ",,,"; change the logic in
bridge.ts so that when allowedChatIdsRaw is provided but
parseCsvList(allowedChatIdsRaw) yields an empty array you fail fast (e.g., throw
a CLI error or exit with a non-zero status) instead of assigning [], keep the
existing allowAll branch (when allowAll is true) that intentionally sets
update.allowedChatIds = [], and reference the symbols allowedChatIdsRaw,
parseCsvList, allowAll, and update.allowedChatIds when making the check and
error path.
In `@apps/cli/src/cli/dispatch.ts`:
- Around line 52-53: The disallowed Set used to guard tmux invocation (variable
disallowed and the if that checks disallowed.has(tmuxSubcommand)) is missing the
controller commands "bug-report" and "self-update", so those still bypass the
--tmux guard; update the Set initializer to include 'bug-report' and
'self-update' so the tmuxSubcommand check correctly rejects them (i.e., extend
the existing new Set([...]) used where disallowed is defined to contain
'bug-report' and 'self-update').
In `@docs/channel-bridge-uat.md`:
- Around line 5-10: The checklist's "clean local state" section only restarts
the daemon; update it to either require switching to a fresh HAPPIER_HOME_DIR or
add explicit cleanup commands: stop the daemon (`happier daemon stop`), remove
or rename the existing settings.json in HAPPIER_HOME_DIR, clear any bridge
config/bindings (delete bridge-related files or run the project-specific
unbind/cleanup command if available), then confirm no active servers (`happier
server list`) and start the daemon (`happier daemon start`) so the steps produce
deterministic results.
---
Duplicate comments:
In `@apps/cli/src/cli/commands/bridge.ts`:
- Around line 266-282: When performing the KV upsert in the branch guarded by
hasSharedTelegramBridgeUpdate (using createAxiosChannelBridgeKvClient and
upsertChannelBridgeTelegramConfigInKv) you must capture the previous KV value
before writing and, if the subsequent updateSettings call that runs
upsertScopedTelegramBridgeConfig fails, revert the KV to that previous value (or
write an explicit reconciliation marker) and then propagate the error; apply the
same pattern in the other similar block that does a KV upsert followed by
updateSettings so you avoid the server-ahead-of-local divergence.
---
Nitpick comments:
In `@apps/cli/src/channels/channelBridgeAccountConfig.ts`:
- Around line 46-64: The update and shared types (ScopedTelegramBridgeUpdate,
SharedTelegramBridgeUpdate) and the anonymous accumulator/return shapes used in
splitScopedTelegramBridgeUpdate() and readScopedTelegramBridgeConfig() should be
promoted to a named interface (e.g., ScopedTelegramBridgeConfig with a nested
WebhookConfig interface) so downstream code gets a single authoritative shape;
change the exported type aliases to interfaces, replace the anonymous
accumulator literals in splitScopedTelegramBridgeUpdate() with the new interface
types, and update readScopedTelegramBridgeConfig() to return the named interface
(or null) and use the named webhook interface for webhook fields to keep
type-safety and consistency across the read/split paths.
In `@apps/cli/src/cli/commands/bridge.test.ts`:
- Around line 19-38: Remove the '@/persistence' mock (the vi.mock that stubs
readCredentials, readSettings, updateSettings) so the test uses the real
persistence module and exercise at least one bridge command happy-path against
the temp HAPPIER_HOME_DIR already created in the spec; keep the
ChannelBridgeKvClient-related mocks (createAxiosChannelBridgeKvClient,
upsertChannelBridgeTelegramConfigInKv, clearChannelBridgeTelegramConfigInKv,
readChannelBridgeTelegramConfigFromKv) as-is so the KV boundary remains mocked,
and ensure the test asserts actual settings.json reads/writes by invoking the
code paths that call readSettings/updateSettings (e.g., the bridge command
execution flow) to validate real persistence behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34124a2a-e2f8-4707-9851-6fd863a3eb6f
⛔ 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 (34)
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.tmuxDisallowed.test.tsapps/cli/src/cli/dispatch.tsapps/cli/src/daemon/startDaemon.automation.integration.test.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
|
Addressed all current open threads in commit a54e4b5 and resolved each thread with per-item notes. Highlights:
@coderabbitai review |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/cli/src/channels/startChannelBridgeWorker.ts (1)
1-35: Add module/API docs for the new bridge entry point.This file now owns webhook fallback, store selection, transcript behavior, and shutdown cleanup, but callers have to infer that contract from the implementation. A file header plus JSDoc on
startChannelBridgeFromEnv()would make the public surface much easier to use safely.As per coding guidelines "Include comprehensive JSDoc comments as file header comments explaining file responsibilities" and "Add JSDoc comments for public APIs and complex logic".
Also applies to: 349-358
🤖 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 1 - 35, Add a clear file-level JSDoc header describing this module's responsibilities (webhook fallback behavior, channel binding store selection, transcript fetching/decryption behavior, and shutdown/cleanup guarantees) and add a JSDoc comment on the public entry startChannelBridgeFromEnv() that documents its parameters/expected environment variables, its return type ChannelBridgeRuntimeHandle, the contract around webhook fallback vs webhook relay, how it chooses between createInMemoryChannelBindingStore and createServerBackedChannelBindingStore, what transcript behavior callers should expect (use of fetchEncryptedTranscriptPageAfterSeq/fetchEncryptedTranscriptPageLatest and decryptTranscriptRows), and the shutdown/cleanup semantics (what the caller must call/await to stop background tasks such as startTelegramWebhookRelay). Include references to the unique symbols startChannelBridgeFromEnv, ChannelBridgeRuntimeHandle, createInMemoryChannelBindingStore, createServerBackedChannelBindingStore, fetchEncryptedTranscriptPageAfterSeq/fetchEncryptedTranscriptPageLatest, decryptTranscriptRows, and startTelegramWebhookRelay to make the contract discoverable.
🤖 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 208-233: The current deserialization of telegram.webhook (via
asRecord(telegram.webhook) into webhook and outWebhook) silently drops malformed
or empty webhook state; update the parser to fail closed by first asserting
webhook is a plain object (not null/array) before proceeding, validate that host
when present is non-empty after trim and otherwise throw
ChannelBridgeBadPayloadError('Invalid telegram.webhook.host payload'), and
validate port as you already do; additionally ensure that if telegram.webhook is
present but none of the expected fields (enabled, host, port) produced valid
values then throw a ChannelBridgeBadPayloadError so you don’t silently return an
empty outWebhook to out.telegram.webhook.
In `@apps/cli/src/channels/core/channelBridgeWorker.ts`:
- Around line 313-357: createInMemoryChannelBindingStore currently keys bindings
directly from raw providerId/conversationId/threadId which can differ from the
server-backed store; update createInMemoryChannelBindingStore to normalize keys
the same way as channelBindingStore.server.ts before using bindingKey: trim
providerId and conversationId strings and treat empty or all-whitespace threadId
as null (the same normalization used by the server-backed store) when computing
key for getBinding, upsertBinding, updateLastForwardedSeq, and removeBinding;
reuse or replicate the normalization logic that produces the same inputs for
bindingKey so lookups, updates, and deletes behave identically to the
server-backed implementation.
In `@apps/cli/src/channels/startChannelBridgeWorker.ts`:
- Around line 441-456: The code in startChannelBridgeWorker (the block that
builds store from params.store, params.serverId and params.accountId) silently
falls back to createInMemoryChannelBindingStore when only one of
serverId/accountId is provided; detect the partial configuration and treat it as
misconfiguration instead of degrading: if one of params.serverId or
params.accountId is present but the other is missing, log/throw a clear error
(or return a failure) indicating both are required, otherwise proceed to
createAxiosChannelBridgeKvClient and createServerBackedChannelBindingStore when
both are present, and only use createInMemoryChannelBindingStore when neither ID
nor params.store is provided; update the logic around store, serverId,
accountId, createAxiosChannelBridgeKvClient,
createServerBackedChannelBindingStore, and createInMemoryChannelBindingStore
accordingly.
- Around line 52-60: The function createStableBridgeLocalId currently returns
randomUUID() when params.messageId is absent, causing instability; instead make
the fallback deterministic by deriving a stable ID from immutable event fields.
Update createStableBridgeLocalId to require or prefer params.messageId but if
missing compute a stable ID using a deterministic hash/UUID v5 of the
concatenation of params.providerId, params.conversationId and params.threadId
(or another immutable event field) so repeated retries yield the same localId;
reference the function name createStableBridgeLocalId and the fields
params.providerId, params.conversationId, params.threadId, params.messageId when
implementing the change.
---
Nitpick comments:
In `@apps/cli/src/channels/startChannelBridgeWorker.ts`:
- Around line 1-35: Add a clear file-level JSDoc header describing this module's
responsibilities (webhook fallback behavior, channel binding store selection,
transcript fetching/decryption behavior, and shutdown/cleanup guarantees) and
add a JSDoc comment on the public entry startChannelBridgeFromEnv() that
documents its parameters/expected environment variables, its return type
ChannelBridgeRuntimeHandle, the contract around webhook fallback vs webhook
relay, how it chooses between createInMemoryChannelBindingStore and
createServerBackedChannelBindingStore, what transcript behavior callers should
expect (use of
fetchEncryptedTranscriptPageAfterSeq/fetchEncryptedTranscriptPageLatest and
decryptTranscriptRows), and the shutdown/cleanup semantics (what the caller must
call/await to stop background tasks such as startTelegramWebhookRelay). Include
references to the unique symbols startChannelBridgeFromEnv,
ChannelBridgeRuntimeHandle, createInMemoryChannelBindingStore,
createServerBackedChannelBindingStore,
fetchEncryptedTranscriptPageAfterSeq/fetchEncryptedTranscriptPageLatest,
decryptTranscriptRows, and startTelegramWebhookRelay to make the contract
discoverable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 083fba6c-9cb9-4e49-854b-e8c021285f6b
📒 Files selected for processing (6)
apps/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.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cli/src/channels/startChannelBridgeWorker.test.ts
|
Addressed the latest greptile/coderabbit findings in commit @coderabbitai review |
|
@greptile review |
|
Good call pairing each of these with focused tests. Triggering an incremental review of commit 🧠 Learnings used✅ Actions performedReview triggered.
|
|
Addressed remaining Greptile feedback in |
|
@greptile review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
apps/cli/src/channels/startChannelBridgeWorker.test.ts (1)
917-962: Cover the inverse partial-scope case too.This exercises
serverIdwithoutaccountId, but notaccountIdwithoutserverId. If the guard becomes one-sided, this suite will miss it. Please parameterize the test over both invalid combinations. As per coding guidelinesapps/cli/**/*.{test,spec}.{js,jsx,ts,tsx}: Write unit tests for all public functions and components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/channels/startChannelBridgeWorker.test.ts` around lines 917 - 962, The test currently only asserts the guard when serverId is provided without accountId; add the inverse case (accountId without serverId) so both invalid combinations are covered: update the test in startChannelBridgeWorker.test.ts to parameterize or loop over two inputs ({serverId: 'server-only', accountId: undefined} and {serverId: undefined, accountId: 'account-only'}) and for each call startChannelBridgeFromEnv with credentials and the corresponding env, assert it rejects with 'require both serverId and accountId', and assert the mocked startChannelBridgeWorker (startWorker) was not called; keep using the existing mocks for createInMemoryChannelBindingStore and createTelegramChannelAdapter and the same expectations for startWorker.
🤖 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 195-214: The current withTimeout implementation only rejects but
does not cancel underlying operations, causing concurrent background work in
functions like ChannelBridgeAdapter.pullInboundMessages,
ChannelBridgeAdapter.sendMessage, ChannelBridgeAdapter.updateLastForwardedSeq
and ChannelBindingStore.* to continue and mutate shared state; add AbortSignal
support to the ChannelBridgeAdapter, ChannelBridgeDeps and ChannelBindingStore
method signatures (e.g., pullInboundMessages(signal?: AbortSignal),
sendMessage(signal?: AbortSignal), updateLastForwardedSeq(signal?: AbortSignal),
etc.), update their implementations to respect signal.aborted and throw on
abort, change callers in channelBridgeWorker to create an AbortController per
timed call, pass controller.signal into withTimeout and into the underlying
adapter/deps/store calls, and modify withTimeout to accept an AbortSignal and
call controller.abort() (or reject) on timeout so the in-flight operations are
actually cancelled rather than only rejected.
In `@apps/cli/src/channels/startChannelBridgeWorker.test.ts`:
- Around line 776-836: The test currently only checks the warning but not that
webhook mode wasn't started; update the test that calls
startChannelBridgeFromEnv to also mock the startTelegramWebhookRelay function
(the module that would start webhook relay) and assert that
startTelegramWebhookRelay was not called, and assert that
createTelegramChannelAdapter (the polling adapter factory) was used/created
instead; specifically mock/import startTelegramWebhookRelay alongside
createTelegramChannelAdapter in the vi.doMock for './telegram/telegramAdapter'
or its relay module and add an expectation that startTelegramWebhookRelay was
not invoked after startChannelBridgeFromEnv returns while warnSpy recorded the
warning.
- Around line 54-55: Add teardown to remove vi.doMock registrations: after each
test in this suite (including the test named 'fetches only the first sessions
page in default bridge deps') call vi.unmock() or vi.doUnmock() for modules
mocked via vi.doMock(), or add a global afterEach that resets the mock registry
to avoid order-dependent leaks; ensure vi.resetModules() remains for module
cache clearing. For the webhook test (the one that logs the missing-secret
warning), extend assertions to verify fallback behavior by asserting that
relay.start was not called or that the bridge entered polling mode (check the
mock relay.start spy or adapter fallback method). For scoped-config validation
add the inverse test: assert that providing accountId without serverId triggers
the same validation error as providing serverId without accountId (create a test
mirroring the existing 'serverId without accountId' case and assert the same
error message/exception).
In `@apps/cli/src/channels/startChannelBridgeWorker.ts`:
- Around line 52-71: The fallback that builds the hash in
createStableBridgeLocalId uses
`fallback:${params.sessionId}:${params.text.trim()}` when `params.messageId` is
blank, which causes distinct messages with identical trimmed text to collide;
change createStableBridgeLocalId to no longer use sessionId+text as the
uniqueness source—either (A) require a stable per-event identifier and throw an
error if `params.messageId` is missing/blank (replace the current fallback
branch that checks `normalizedMessageId.length > 0`) so callers must supply a
stable id, or (B) add a new required parameter like `eventId` (or
`adapterEventId`) and use that in the hash when `messageId` is empty, and update
all callers of createStableBridgeLocalId (and any other locations that use the
same fallback pattern) to provide the new id; reference the
`createStableBridgeLocalId` function and the `params.messageId` /
`fallback:${params.sessionId}:${params.text.trim()}` code path when making the
change.
In `@apps/cli/src/cli/commands/bridge.ts`:
- Around line 224-234: The parsing currently treats missing and explicit-empty
values the same because argvValue(...).trim() returns "" for both; add explicit
presence checks like hasAllowedChatIdsFlag = args.some(arg => arg ===
'--allowed-chat-ids' || arg.startsWith('--allowed-chat-ids=')) (same pattern
used by hasBotTokenFlag) for each value-taking flag (allowedChatIds,
requireTopics, tickMs, webhookEnabled, webhookSecret, webhookHost, webhookPort)
and then validate: if the presence flag is true and the corresponding raw value
is an empty string, throw or exit with a clear error; alternatively, adjust
argvValue usage to return undefined for missing flags and keep the same presence
check to reject '' when the flag was provided. Ensure you update the conditional
update blocks that currently check truthiness (e.g., if (allowedChatIdsRaw) {
... }) to instead use the presence flags and non-empty value checks so explicit
empty options are rejected.
---
Nitpick comments:
In `@apps/cli/src/channels/startChannelBridgeWorker.test.ts`:
- Around line 917-962: The test currently only asserts the guard when serverId
is provided without accountId; add the inverse case (accountId without serverId)
so both invalid combinations are covered: update the test in
startChannelBridgeWorker.test.ts to parameterize or loop over two inputs
({serverId: 'server-only', accountId: undefined} and {serverId: undefined,
accountId: 'account-only'}) and for each call startChannelBridgeFromEnv with
credentials and the corresponding env, assert it rejects with 'require both
serverId and accountId', and assert the mocked startChannelBridgeWorker
(startWorker) was not called; keep using the existing mocks for
createInMemoryChannelBindingStore and createTelegramChannelAdapter and the same
expectations for startWorker.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f600912-d9c5-4672-8a44-f2b6dc5ea4f5
📒 Files selected for processing (7)
apps/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/cli/commands/bridge.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/cli/src/channels/channelBridgeServerKv.ts
- apps/cli/src/channels/channelBridgeServerKv.test.ts
| const rawBotToken = argvValue(args, '--bot-token'); | ||
| const hasBotTokenFlag = args.some((arg) => arg === '--bot-token' || arg.startsWith('--bot-token=')); | ||
| const botToken = rawBotToken.trim(); | ||
| const allowedChatIdsRaw = argvValue(args, '--allowed-chat-ids').trim(); | ||
| const allowAll = args.includes('--allow-all'); | ||
| const requireTopicsRaw = argvValue(args, '--require-topics').trim(); | ||
| const tickMsRaw = argvValue(args, '--tick-ms').trim(); | ||
| const webhookEnabledRaw = argvValue(args, '--webhook-enabled').trim(); | ||
| const webhookSecret = argvValue(args, '--webhook-secret').trim(); | ||
| const webhookHost = argvValue(args, '--webhook-host').trim(); | ||
| const webhookPortRaw = argvValue(args, '--webhook-port').trim(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect argvValue semantics and the current empty-value handling in bridge.ts.
fd 'commandUtilities\.ts$' apps/cli/src --exec sed -n '1,220p' {}
sed -n '224,287p' apps/cli/src/cli/commands/bridge.ts
rg -n "hasBotTokenFlag|argvValue\(args, '--" apps/cli/src/cli/commands/bridge.tsRepository: happier-dev/happier
Length of output: 5646
Explicit empty option values silently skip updates instead of being rejected.
Only --bot-token checks for flag presence with hasBotTokenFlag. Other value-taking flags rely on argvValue(...).trim(), which returns empty string for both missing and present-but-empty (e.g., --allowed-chat-ids=, --webhook-secret=). Since the update blocks use truthiness checks (if (allowedChatIdsRaw) { ... }), providing --allowed-chat-ids=, --tick-ms=, --webhook-enabled=, --webhook-host=, --webhook-port=, or --webhook-secret= with no value silently succeeds without updating config and without raising an error.
Add explicit presence tracking for all value-taking flags (consistent with --bot-token), or reject empty values at parse time.
🤖 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 224 - 234, The parsing
currently treats missing and explicit-empty values the same because
argvValue(...).trim() returns "" for both; add explicit presence checks like
hasAllowedChatIdsFlag = args.some(arg => arg === '--allowed-chat-ids' ||
arg.startsWith('--allowed-chat-ids=')) (same pattern used by hasBotTokenFlag)
for each value-taking flag (allowedChatIds, requireTopics, tickMs,
webhookEnabled, webhookSecret, webhookHost, webhookPort) and then validate: if
the presence flag is true and the corresponding raw value is an empty string,
throw or exit with a clear error; alternatively, adjust argvValue usage to
return undefined for missing flags and keep the same presence check to reject ''
when the flag was provided. Ensure you update the conditional update blocks that
currently check truthiness (e.g., if (allowedChatIdsRaw) { ... }) to instead use
the presence flags and non-empty value checks so explicit empty options are
rejected.
| const runtime = await resolveSessionRuntime(sessionId); | ||
| if (!runtime) { | ||
| return { | ||
| messages: [], | ||
| highestSeenSeq: null, | ||
| }; |
There was a problem hiding this comment.
Silent outbound stall when session becomes inaccessible
When resolveSessionRuntime returns null (e.g. the session has been deleted, revoked, or is temporarily unreachable), the function returns { messages: [], highestSeenSeq: null } without emitting any warning. The bridge worker receives zero messages and no highestSeenSeq to advance with, so the binding's lastForwardedSeq cursor is permanently frozen. Every subsequent tick repeats the same silent no-op: fetchSessionById returns null, fetchEncryptedTranscriptPageAfterSeq is never called, and the binding stalls indefinitely.
Compare this with the analogous store.getBinding failure path in channelBridgeWorker.ts, which both notifies the user in-channel and marks the event seen so polling batches are not replayed. Here the operator has no log-level signal at all to indicate why their Telegram conversation stopped receiving agent replies.
A minimal fix is to emit a warning via onWarning so operators can observe the condition in logs:
const runtime = await resolveSessionRuntime(sessionId);
if (!runtime) {
onWarning(
`Session not found or inaccessible; outbound forwarding stalled for session=${sessionId}`,
);
return {
messages: [],
highestSeenSeq: null,
};
}| if (typeof record.tickMs === 'number' && Number.isFinite(record.tickMs)) { | ||
| out.tickMs = Math.trunc(record.tickMs); | ||
| } |
There was a problem hiding this comment.
parseTelegramConfigRecord accepts out-of-range tickMs from KV
The write path (applyTelegramConfigUpdate) now rejects tickMs outside [250, 60_000] with a ChannelBridgeBadPayloadError. The read path here accepts any finite number, so a KV record written by an external tool with tickMs: 50 would silently pass through parseTelegramConfigRecord and into the settings overlay. It is only discarded later in resolveChannelBridgeRuntimeConfig via parseInteger(value, 250, 60_000), which silently falls back to the 2500ms default with no diagnostic message.
While end-to-end correctness is preserved, the inconsistency means parseTelegramConfigRecord can return a value whose tickMs field is already known to be invalid. Aligning the read path with the write-path range check (or at minimum emitting a log warning when the KV value is clamped away downstream) would make the contract easier to reason about.
| if (typeof record.tickMs === 'number' && Number.isFinite(record.tickMs)) { | |
| out.tickMs = Math.trunc(record.tickMs); | |
| } | |
| if (typeof record.tickMs === 'number' && Number.isFinite(record.tickMs)) { | |
| const truncatedTickMs = Math.trunc(record.tickMs); | |
| if (truncatedTickMs < 250 || truncatedTickMs > 60_000) { | |
| throw new ChannelBridgeBadPayloadError('Invalid tickMs payload: must be in [250, 60000]'); | |
| } | |
| out.tickMs = truncatedTickMs; | |
| } |
| const settingsWebhookSecret = | ||
| firstParsed([ | ||
| secretsAccount?.webhookSecret, | ||
| webhookAccount?.secret, | ||
| secretsServer?.webhookSecret, | ||
| webhookServer?.secret, | ||
| secretsGlobal?.webhookSecret, | ||
| webhookGlobal?.secret, | ||
| ], readTrimmedString) | ||
| ?? ''; | ||
| const envWebhookSecretRaw = | ||
| typeof env.HAPPIER_TELEGRAM_WEBHOOK_SECRET === 'string' | ||
| ? env.HAPPIER_TELEGRAM_WEBHOOK_SECRET.trim() | ||
| : null; | ||
| const envWebhookSecret = readWebhookSecretToken(envWebhookSecretRaw); | ||
| const webhookSecret = envWebhookSecret ?? settingsWebhookSecret; |
There was a problem hiding this comment.
Settings webhook token not validated for allowed character set
settingsWebhookSecret is resolved through readTrimmedString, which only checks for a non-empty string. By contrast, the env-var path on line 230 runs the value through readWebhookSecretToken, which enforces the same [A-Za-z0-9_-] pattern required by startTelegramWebhookRelay.
If a user directly edits settings.json and sets a webhook.secret containing characters outside that set (e.g. spaces or punctuation), the value passes this resolution step and is forwarded to startTelegramWebhookRelay, which throws "Webhook secret token must match [A-Za-z0-9_-]". The catch block in startChannelBridgeFromEnv then silently falls back to polling mode with only a logger.warn.
The exact same gap was previously fixed for settingsWebhookHost — the settings-level host is now validated via isLoopbackHost and falls back to 127.0.0.1 when invalid (line 239). Applying the same pattern here would give consistent early-catch behaviour: run the resolved raw value through readWebhookSecretToken before assigning to settingsWebhookSecret, and fall back to '' (disabled) when the pattern check fails. This mirrors the env-var path and eliminates the silent polling fallback that only a buried logger.warn would signal.
|
Addressed latest review findings in |
|
@greptile review |
|
Triggering an incremental review of commit 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
apps/cli/src/channels/core/channelBridgeWorker.ts (1)
63-68:⚠️ Potential issue | 🟠 MajorTimeouts still don't cancel bridge I/O.
withTimeout()only rejects locally; none of these contracts carry cancellation, so the timed-out adapter/store/deps call keeps running in the background. The next tick can overlap it and duplicate sends or race cursor updates. Add a cancellation signal to the bridge contracts and have the timeout path abort in-flight work instead of only racing it.#!/bin/bash sed -n '63,68p' apps/cli/src/channels/core/channelBridgeWorker.ts sed -n '108,147p' apps/cli/src/channels/core/channelBridgeWorker.ts sed -n '195,214p' apps/cli/src/channels/core/channelBridgeWorker.ts rg -n "AbortSignal|signal" apps/cli/src/channels/core/channelBridgeWorker.ts apps/cli/src/channels/startChannelBridgeWorker.tsAs per coding guidelines "Use graceful error handling with proper error messages, try-catch blocks with specific error logging, and abort controllers for cancellable operations".
Also applies to: 108-147, 195-214
🤖 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 63 - 68, The bridge I/O functions currently race with withTimeout() but continue running because the ChannelBridgeAdapter contracts (pullInboundMessages, ackInboundMessages, sendMessage, stop) carry no cancellation; update the adapter type to accept an optional AbortSignal parameter on these methods and propagate that signal through usages in channelBridgeWorker and startChannelBridgeWorker, and where withTimeout() times out create an AbortController and call controller.abort() to cancel in-flight work; ensure callers (e.g., pullInboundMessages, sendMessage, ackInboundMessages) check signal.aborted and throw/return promptly and wrap calls in try/catch to log specific abort vs other errors.apps/cli/src/channels/startChannelBridgeWorker.ts (1)
52-71:⚠️ Potential issue | 🟠 MajorBlank
messageIdstill collides for repeated messages.When
messageIdis empty, the fallback hash usessessionId + text.trim(). Two distinct"ok"messages in the same conversation/thread/session generate the samelocalId, so the upstream idempotency key can collapse one of them. This needs a stable per-event identifier from the adapter, or a hard failure on blank IDs.🤖 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 52 - 71, createStableBridgeLocalId currently falls back to hashing sessionId+text when messageId is blank, causing distinct messages with identical text in the same session to collide; update createStableBridgeLocalId to reject blank/empty messageId instead of falling back: validate params.messageId (trimmed) at the top of the function and throw a clear error (including providerId/conversationId/threadId/sessionId) when it's missing so adapters must supply a stable per-event identifier; keep the rest of the hash logic unchanged for valid messageId values.
🧹 Nitpick comments (2)
apps/cli/src/cli/commands/bridge.ts (2)
445-467: Add JSDoc for public API export.As per coding guidelines, public APIs should have JSDoc comments. This is the main exported entry point for the bridge CLI command.
📝 Suggested JSDoc
+/** + * Handles the `happier bridge` CLI command and its subcommands. + * + * Subcommands: + * - `list` - Display bridge configuration from all sources + * - `telegram set` - Configure Telegram bridge settings + * - `telegram clear` - Remove Telegram bridge configuration + * + * `@param` context - Command context containing args and environment + */ export async function handleBridgeCliCommand(context: CommandContext): Promise<void> {🤖 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 445 - 467, Add a JSDoc block above the exported function handleBridgeCliCommand describing its purpose as the CLI entry point for the "bridge" command, documenting the parameter (context: CommandContext) and its properties or role, and specifying the return type Promise<void> and any thrown errors; include tags like `@param`, `@returns`, and a short example or remarks if helpful so the function is documented as a public API export.
1-26: Missing file header JSDoc comment.As per coding guidelines, CLI TypeScript files should include comprehensive JSDoc comments as file header comments explaining file responsibilities. This file implements the account-scoped bridge CLI command handling but lacks this documentation.
📝 Suggested file header
+/** + * Bridge CLI command handler for account-scoped channel bridge configuration. + * + * Provides commands to: + * - List bridge state (server KV, local settings, effective runtime) + * - Configure Telegram bridge settings (set/clear) + * + * Handles transactional persistence to server KV with local settings overlay + * and rollback compensation on partial failures. + * + * `@module` bridge + */ 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 - 26, Add a comprehensive top-of-file JSDoc header describing this module's responsibility as the CLI command handler for account-scoped bridge operations (handling Telegram bridge config reads/updates, KV overlay, and daemon checks), list the main exported command and its purpose, and mention key collaborators used in the file such as CommandContext, readScopedTelegramBridgeConfig, upsertScopedTelegramBridgeConfig, readChannelBridgeTelegramConfigFromKv, replaceChannelBridgeTelegramConfigRawInKv, checkIfDaemonRunningAndCleanupStaleState, and decodeJwtPayload so readers know the high-level flow; include usage notes for relevant CLI flags parsed via argvValue and any important side effects (KV writes, daemon interactions), authorship and any applicable license or lint tags to satisfy the repo JSDoc guideline.
🤖 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/channelBridgeConfig.ts`:
- Around line 168-171: The current botToken assignment treats a whitespace-only
HAPPIER_TELEGRAM_BOT_TOKEN as a valid (empty) override; change the logic so that
if env.HAPPIER_TELEGRAM_BOT_TOKEN is a string you trim it and only use it when
the trimmed value is non-empty, otherwise fall back to settingsBotToken; update
the code around the botToken variable (referencing HAPPIER_TELEGRAM_BOT_TOKEN
and settingsBotToken) to check trimmed.length > 0 before choosing the env value.
- Around line 216-248: The code selects the first non-empty scoped value and
then validates it, so invalid account-level values (e.g., bad secret or
non-loopback host) can wrongly mask valid lower-scope values; update selection
to validate each candidate before precedence. For webhookSecret: apply
readWebhookSecretToken (or a small validator wrapper) to each candidate in the
firstParsed call instead of using readTrimmedString, or pre-map/filter
[secretsAccount?.webhookSecret, webhookAccount?.secret, ...] through
readTrimmedString then readWebhookSecretToken and pass the validated list to
firstParsed so only valid tokens win; update env handling to keep using
readWebhookSecretToken for envWebhookSecretRaw. For webhookHost: replace
firstParsed([...], readTrimmedString) with a validator that trims then checks
isLoopbackHost (or pre-filter the candidates via trim+isLoopbackHost) so
settingsWebhookHostRaw is guaranteed valid before falling back; then compute
webhookHost using envWebhookHostRaw as now but ensure envWebhookHostRaw is
validated by isLoopbackHost first.
In `@apps/cli/src/channels/channelBridgeServerKv.ts`:
- Around line 271-297: The parser currently truncates fractional
lastForwardedSeq which can cause replay bugs; change the validation for
lastForwardedSeq to require an integer (e.g., use
Number.isInteger(item.lastForwardedSeq)) and throw ChannelBridgeBadPayloadError
if it is not an integer or is negative/invalid, and stop using Math.trunc for
lastForwardedSeq when constructing the binding; keep the existing
finite/negative checks for createdAtMs/updatedAtMs (or add integer checks if you
also want them strict) and ensure the error messages reference lastForwardedSeq
for clarity.
In `@apps/cli/src/channels/core/channelBridgeWorker.ts`:
- Around line 778-792: The current branches call replyToConversation() then
always call deduper.markSeen(...), push the event to ackableInbound, and set
processedSuccessfully = true even if the reply threw; change each fallback-reply
branch (the unknown-command branch and the other branches around
replyToConversation in channelBridgeWorker.ts — references: replyToConversation,
deduper.markSeen, ackableInbound, processedSuccessfully) so that
deduper.markSeen(...), ackableInbound.push(event) and processedSuccessfully =
true are executed only after the await replyToConversation(...) completes
successfully (i.e., move those three statements into the try block immediately
after the await), and leave the catch to call params.deps.onWarning(...) but not
ack or mark the event as processed; apply the same fix to the other reply
branches you noted (lines covering binding-read-failure, no-binding,
session-forward-failure).
---
Duplicate comments:
In `@apps/cli/src/channels/core/channelBridgeWorker.ts`:
- Around line 63-68: The bridge I/O functions currently race with withTimeout()
but continue running because the ChannelBridgeAdapter contracts
(pullInboundMessages, ackInboundMessages, sendMessage, stop) carry no
cancellation; update the adapter type to accept an optional AbortSignal
parameter on these methods and propagate that signal through usages in
channelBridgeWorker and startChannelBridgeWorker, and where withTimeout() times
out create an AbortController and call controller.abort() to cancel in-flight
work; ensure callers (e.g., pullInboundMessages, sendMessage,
ackInboundMessages) check signal.aborted and throw/return promptly and wrap
calls in try/catch to log specific abort vs other errors.
In `@apps/cli/src/channels/startChannelBridgeWorker.ts`:
- Around line 52-71: createStableBridgeLocalId currently falls back to hashing
sessionId+text when messageId is blank, causing distinct messages with identical
text in the same session to collide; update createStableBridgeLocalId to reject
blank/empty messageId instead of falling back: validate params.messageId
(trimmed) at the top of the function and throw a clear error (including
providerId/conversationId/threadId/sessionId) when it's missing so adapters must
supply a stable per-event identifier; keep the rest of the hash logic unchanged
for valid messageId values.
---
Nitpick comments:
In `@apps/cli/src/cli/commands/bridge.ts`:
- Around line 445-467: Add a JSDoc block above the exported function
handleBridgeCliCommand describing its purpose as the CLI entry point for the
"bridge" command, documenting the parameter (context: CommandContext) and its
properties or role, and specifying the return type Promise<void> and any thrown
errors; include tags like `@param`, `@returns`, and a short example or remarks if
helpful so the function is documented as a public API export.
- Around line 1-26: Add a comprehensive top-of-file JSDoc header describing this
module's responsibility as the CLI command handler for account-scoped bridge
operations (handling Telegram bridge config reads/updates, KV overlay, and
daemon checks), list the main exported command and its purpose, and mention key
collaborators used in the file such as CommandContext,
readScopedTelegramBridgeConfig, upsertScopedTelegramBridgeConfig,
readChannelBridgeTelegramConfigFromKv,
replaceChannelBridgeTelegramConfigRawInKv,
checkIfDaemonRunningAndCleanupStaleState, and decodeJwtPayload so readers know
the high-level flow; include usage notes for relevant CLI flags parsed via
argvValue and any important side effects (KV writes, daemon interactions),
authorship and any applicable license or lint tags to satisfy the repo JSDoc
guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d213e89-8aa0-4d4d-8b1b-a62574582def
📒 Files selected for processing (9)
apps/cli/src/channels/channelBridgeConfig.test.tsapps/cli/src/channels/channelBridgeConfig.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/cli/commands/bridge.ts
| const botToken = | ||
| typeof env.HAPPIER_TELEGRAM_BOT_TOKEN === 'string' | ||
| ? env.HAPPIER_TELEGRAM_BOT_TOKEN.trim() | ||
| : settingsBotToken; |
There was a problem hiding this comment.
Treat blank HAPPIER_TELEGRAM_BOT_TOKEN as unset.
Every other env override in this resolver ignores effectively empty values, but this branch resolves whitespace-only input to ''. That lets an accidentally defined empty env var mask a valid scoped token and disable bridge startup.
♻️ Suggested fix
+ const envBotToken = readTrimmedString(env.HAPPIER_TELEGRAM_BOT_TOKEN);
const botToken =
- typeof env.HAPPIER_TELEGRAM_BOT_TOKEN === 'string'
- ? env.HAPPIER_TELEGRAM_BOT_TOKEN.trim()
- : settingsBotToken;
+ envBotToken ?? settingsBotToken;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/cli/src/channels/channelBridgeConfig.ts` around lines 168 - 171, The
current botToken assignment treats a whitespace-only HAPPIER_TELEGRAM_BOT_TOKEN
as a valid (empty) override; change the logic so that if
env.HAPPIER_TELEGRAM_BOT_TOKEN is a string you trim it and only use it when the
trimmed value is non-empty, otherwise fall back to settingsBotToken; update the
code around the botToken variable (referencing HAPPIER_TELEGRAM_BOT_TOKEN and
settingsBotToken) to check trimmed.length > 0 before choosing the env value.
| const settingsWebhookSecret = | ||
| firstParsed([ | ||
| secretsAccount?.webhookSecret, | ||
| webhookAccount?.secret, | ||
| secretsServer?.webhookSecret, | ||
| webhookServer?.secret, | ||
| secretsGlobal?.webhookSecret, | ||
| webhookGlobal?.secret, | ||
| ], readTrimmedString) | ||
| ?? ''; | ||
| const envWebhookSecretRaw = | ||
| typeof env.HAPPIER_TELEGRAM_WEBHOOK_SECRET === 'string' | ||
| ? env.HAPPIER_TELEGRAM_WEBHOOK_SECRET.trim() | ||
| : null; | ||
| const envWebhookSecret = readWebhookSecretToken(envWebhookSecretRaw); | ||
| const webhookSecret = envWebhookSecret ?? settingsWebhookSecret; | ||
|
|
||
| const settingsWebhookHostRaw = | ||
| firstParsed( | ||
| [webhookAccount?.host, webhookServer?.host, webhookGlobal?.host], | ||
| readTrimmedString, | ||
| ) | ||
| || '127.0.0.1'; | ||
| const settingsWebhookHost = isLoopbackHost(settingsWebhookHostRaw) ? settingsWebhookHostRaw : '127.0.0.1'; | ||
| const envWebhookHostRaw = | ||
| typeof env.HAPPIER_TELEGRAM_WEBHOOK_HOST === 'string' | ||
| ? env.HAPPIER_TELEGRAM_WEBHOOK_HOST.trim() | ||
| : null; | ||
| const envWebhookHost = | ||
| envWebhookHostRaw && isLoopbackHost(envWebhookHostRaw) | ||
| ? envWebhookHostRaw | ||
| : null; | ||
| const webhookHost = envWebhookHost ?? settingsWebhookHost; |
There was a problem hiding this comment.
Validate scoped webhook secret/host values before applying precedence.
Right now the first non-empty scoped string wins, and only then do we validate it. An account-level secret: 'bad token!' survives unchanged, and an account-level host: '0.0.0.0' masks a valid lower-scope host and then drops to the hardcoded default at Line 239 instead of the next valid scope. A scoped regression with an invalid account value and a valid server value would catch both paths.
🛡️ Suggested fix
const settingsWebhookSecret =
firstParsed([
secretsAccount?.webhookSecret,
webhookAccount?.secret,
secretsServer?.webhookSecret,
webhookServer?.secret,
secretsGlobal?.webhookSecret,
webhookGlobal?.secret,
- ], readTrimmedString)
+ ], readWebhookSecretToken)
?? '';
@@
- const settingsWebhookHostRaw =
- firstParsed(
- [webhookAccount?.host, webhookServer?.host, webhookGlobal?.host],
- readTrimmedString,
- )
- || '127.0.0.1';
- const settingsWebhookHost = isLoopbackHost(settingsWebhookHostRaw) ? settingsWebhookHostRaw : '127.0.0.1';
+ const settingsWebhookHost =
+ firstParsed(
+ [webhookAccount?.host, webhookServer?.host, webhookGlobal?.host],
+ (value) => {
+ const host = readTrimmedString(value);
+ return host && isLoopbackHost(host) ? host : null;
+ },
+ ) ?? '127.0.0.1';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/cli/src/channels/channelBridgeConfig.ts` around lines 216 - 248, The
code selects the first non-empty scoped value and then validates it, so invalid
account-level values (e.g., bad secret or non-loopback host) can wrongly mask
valid lower-scope values; update selection to validate each candidate before
precedence. For webhookSecret: apply readWebhookSecretToken (or a small
validator wrapper) to each candidate in the firstParsed call instead of using
readTrimmedString, or pre-map/filter [secretsAccount?.webhookSecret,
webhookAccount?.secret, ...] through readTrimmedString then
readWebhookSecretToken and pass the validated list to firstParsed so only valid
tokens win; update env handling to keep using readWebhookSecretToken for
envWebhookSecretRaw. For webhookHost: replace firstParsed([...],
readTrimmedString) with a validator that trims then checks isLoopbackHost (or
pre-filter the candidates via trim+isLoopbackHost) so settingsWebhookHostRaw is
guaranteed valid before falling back; then compute webhookHost using
envWebhookHostRaw as now but ensure envWebhookHostRaw is validated by
isLoopbackHost first.
| if ( | ||
| typeof item.lastForwardedSeq !== 'number' | ||
| || !Number.isFinite(item.lastForwardedSeq) | ||
| || item.lastForwardedSeq < 0 | ||
| ) { | ||
| throw new ChannelBridgeBadPayloadError(`Invalid channel bridge binding lastForwardedSeq at index ${index}`); | ||
| } | ||
| if (typeof item.createdAtMs !== 'number' || !Number.isFinite(item.createdAtMs)) { | ||
| throw new ChannelBridgeBadPayloadError(`Invalid channel bridge binding createdAtMs at index ${index}`); | ||
| } | ||
| if (typeof item.updatedAtMs !== 'number' || !Number.isFinite(item.updatedAtMs)) { | ||
| throw new ChannelBridgeBadPayloadError(`Invalid channel bridge binding updatedAtMs at index ${index}`); | ||
| } | ||
|
|
||
| if ( | ||
| item.threadId !== undefined | ||
| && item.threadId !== null | ||
| && typeof item.threadId !== 'string' | ||
| ) { | ||
| throw new ChannelBridgeBadPayloadError(`Invalid channel bridge binding threadId at index ${index}`); | ||
| } | ||
|
|
||
| const threadIdRaw = typeof item.threadId === 'string' ? item.threadId.trim() : ''; | ||
| const threadId = threadIdRaw.length > 0 ? threadIdRaw : null; | ||
| const lastForwardedSeq = Math.trunc(item.lastForwardedSeq); | ||
| const createdAtMs = Math.trunc(item.createdAtMs); | ||
| const updatedAtMs = Math.trunc(item.updatedAtMs); |
There was a problem hiding this comment.
Reject fractional persisted cursors instead of truncating them.
lastForwardedSeq is replay coordination state. Accepting 1.9 and normalizing it to 1 can replay already-forwarded agent output on the next sync. This parser is already fail-closed for missing cursor fields; it should also require an integer cursor before constructing the binding record.
🤖 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 271 - 297, The
parser currently truncates fractional lastForwardedSeq which can cause replay
bugs; change the validation for lastForwardedSeq to require an integer (e.g.,
use Number.isInteger(item.lastForwardedSeq)) and throw
ChannelBridgeBadPayloadError if it is not an integer or is negative/invalid, and
stop using Math.trunc for lastForwardedSeq when constructing the binding; keep
the existing finite/negative checks for createdAtMs/updatedAtMs (or add integer
checks if you also want them strict) and ensure the error messages reference
lastForwardedSeq for clarity.
| if (event.text.trim().startsWith('/')) { | ||
| try { | ||
| await replyToConversation(adapter, { | ||
| conversationId: event.conversationId, | ||
| threadId: event.threadId, | ||
| }, 'Unknown command. Use /help for supported commands.'); | ||
| } catch (replyError) { | ||
| params.deps.onWarning?.( | ||
| `Failed to send unknown-command reply for provider=${adapter.providerId} conversation=${event.conversationId} thread=${event.threadId ?? 'null'}`, | ||
| replyError, | ||
| ); | ||
| } | ||
| deduper.markSeen(event); | ||
| ackableInbound.push(event); | ||
| processedSuccessfully = true; |
There was a problem hiding this comment.
Don't mark these inbound messages handled when the fallback reply failed.
Each of these branches marks the event seen/ackable even when replyToConversation() throws. If the unknown-command, binding-read-failure, no-binding, or session-forward-failure reply cannot be sent, the original inbound message is dropped with no session forward and no visible feedback. Only acknowledge after the fallback reply succeeds.
Also applies to: 803-829, 831-847, 864-890
🤖 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 778 - 792,
The current branches call replyToConversation() then always call
deduper.markSeen(...), push the event to ackableInbound, and set
processedSuccessfully = true even if the reply threw; change each fallback-reply
branch (the unknown-command branch and the other branches around
replyToConversation in channelBridgeWorker.ts — references: replyToConversation,
deduper.markSeen, ackableInbound, processedSuccessfully) so that
deduper.markSeen(...), ackableInbound.push(event) and processedSuccessfully =
true are executed only after the await replyToConversation(...) completes
successfully (i.e., move those three statements into the try block immediately
after the await), and leave the catch to call params.deps.onWarning(...) but not
ack or mark the event as processed; apply the same fix to the other reply
branches you noted (lines covering binding-read-failure, no-binding,
session-forward-failure).
|
All current review threads are resolved and checks are green; requesting a final pass update on latest head |
|
@greptile review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
| function createStableBridgeLocalId(params: Readonly<{ | ||
| providerId: string; | ||
| conversationId: string; | ||
| threadId: string | null; | ||
| sessionId: string; | ||
| text: string; | ||
| messageId?: string; | ||
| }>): string { | ||
| const normalizedMessageId = typeof params.messageId === 'string' ? params.messageId.trim() : ''; | ||
| if (normalizedMessageId.length === 0) { | ||
| throw new Error('Channel bridge inbound messageId is required for stable idempotency'); | ||
| } | ||
|
|
||
| const digest = createHash('sha256') | ||
| .update(params.providerId) | ||
| .update('\u0000') | ||
| .update(params.conversationId) | ||
| .update('\u0000') | ||
| .update(params.threadId ?? '') | ||
| .update('\u0000') |
There was a problem hiding this comment.
text and sessionId are declared but unused in the hash
createStableBridgeLocalId accepts sessionId: string and text: string in its parameter type but neither is fed into the SHA-256 digest — only providerId, conversationId, threadId, and messageId contribute. The two dead parameters increase cognitive overhead: a reader might reasonably assume they influence the localId or are reserved for a future version. Additionally, while messageId is typed as optional (?: string), the function throws immediately when it is absent, which is inconsistent with the ChannelBridgeDeps.sendUserMessageToSession interface (which also declares messageId?: string). Any caller that follows the interface contract and omits messageId will receive an unexpected runtime exception rather than a type error.
Consider removing the unused parameters and either making messageId required in both the interface and this function, or generating a UUID fallback when it is absent:
function createStableBridgeLocalId(params: Readonly<{
providerId: string;
conversationId: string;
threadId: string | null;
messageId: string; // required — callers must supply a stable message identity
}>): string {
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/cli/src/channels/channelBridgeServerKv.ts (1)
271-297:⚠️ Potential issue | 🟠 MajorRequire an integer
lastForwardedSeq.This field is replay cursor state. Truncating
1.9to1turns a corrupt persisted row into “forward from 1”, which can resend already-forwarded agent output after the next sync.💡 Suggested fix
- if ( - typeof item.lastForwardedSeq !== 'number' - || !Number.isFinite(item.lastForwardedSeq) - || item.lastForwardedSeq < 0 - ) { + const lastForwardedSeq = item.lastForwardedSeq; + if ( + typeof lastForwardedSeq !== 'number' + || !Number.isFinite(lastForwardedSeq) + || !Number.isInteger(lastForwardedSeq) + || lastForwardedSeq < 0 + ) { throw new ChannelBridgeBadPayloadError(`Invalid channel bridge binding lastForwardedSeq at index ${index}`); } @@ - const lastForwardedSeq = Math.trunc(item.lastForwardedSeq); const createdAtMs = Math.trunc(item.createdAtMs); const updatedAtMs = Math.trunc(item.updatedAtMs);🤖 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 271 - 297, The code currently truncates item.lastForwardedSeq (Math.trunc) which silently accepts non-integer values like 1.9 and can cause replay/duplicate forwards; change the validation in the channelBridgeServerKv binding parsing so lastForwardedSeq must be an integer (use Number.isInteger) and non-negative, throw ChannelBridgeBadPayloadError if not, and stop using Math.trunc when assigning lastForwardedSeq (use the validated value directly); update the validation logic that references item.lastForwardedSeq and the assignment to lastForwardedSeq to reflect this stricter integer requirement.apps/cli/src/cli/commands/bridge.ts (1)
224-234:⚠️ Potential issue | 🟠 MajorReject explicit empty values for every value-taking flag.
Only
--bot-tokentracks flag presence separately. The other options collapse “missing” and “present but empty” into the same''value, so inputs like--webhook-secret=or--tick-ms=are silently ignored if another flag in the same command is valid. That leaves stale config in place while the command still reports success.💡 Suggested direction
const rawBotToken = argvValue(args, '--bot-token'); const hasBotTokenFlag = args.some((arg) => arg === '--bot-token' || arg.startsWith('--bot-token=')); + const hasAllowedChatIdsFlag = args.some((arg) => arg === '--allowed-chat-ids' || arg.startsWith('--allowed-chat-ids=')); + const hasRequireTopicsFlag = args.some((arg) => arg === '--require-topics' || arg.startsWith('--require-topics=')); + const hasTickMsFlag = args.some((arg) => arg === '--tick-ms' || arg.startsWith('--tick-ms=')); + const hasWebhookEnabledFlag = args.some((arg) => arg === '--webhook-enabled' || arg.startsWith('--webhook-enabled=')); + const hasWebhookSecretFlag = args.some((arg) => arg === '--webhook-secret' || arg.startsWith('--webhook-secret=')); + const hasWebhookHostFlag = args.some((arg) => arg === '--webhook-host' || arg.startsWith('--webhook-host=')); + const hasWebhookPortFlag = args.some((arg) => arg === '--webhook-port' || arg.startsWith('--webhook-port=')); @@ - } else if (allowedChatIdsRaw) { + } else if (hasAllowedChatIdsFlag) { + if (!allowedChatIdsRaw) { + throw new Error('Invalid --allowed-chat-ids value: cannot be empty'); + } const parsedAllowedChatIds = parseCsvList(allowedChatIdsRaw); @@ - if (tickMsRaw) { + if (hasTickMsFlag) { + if (!tickMsRaw) { + throw new Error('Invalid --tick-ms value: cannot be empty'); + } update.tickMs = parseIntegerInput(tickMsRaw, '--tick-ms', 250, 60_000); }Apply the same presence-check pattern to
--require-topics,--webhook-enabled,--webhook-secret,--webhook-host, and--webhook-port.Also applies to: 251-287
🤖 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 224 - 234, The flags other than --bot-token are currently treated the same whether omitted or passed with an empty value; update the parsing around argvValue/args to mirror the --bot-token pattern: for each value-taking flag (requireTopicsRaw, webhookEnabledRaw, webhookSecret, webhookHost, webhookPort, tickMsRaw, allowedChatIdsRaw) add a hasXFlag boolean (e.g., hasRequireTopicsFlag = args.some(arg => arg === '--require-topics' || arg.startsWith('--require-topics='))) and then if the flag is present but argvValue returned an empty string, reject it (throw or processLogger.error + exit) instead of treating it as “missing”; apply this change to the parsing code that defines requireTopicsRaw, webhookEnabledRaw, webhookSecret, webhookHost, webhookPort, tickMsRaw and allowedChatIdsRaw so explicit empty values are detected and handled consistently.
🤖 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 190-192: The KV helper currently truncates fractional tickMs on
write (record.tickMs -> out.tickMs via Math.trunc) and the read/parser accepts
any finite number, letting malformed values like 1.9, 0, or -5 pass; change both
paths to enforce a validated integer: on the write side (where record.tickMs is
handled) only accept Number.isInteger(record.tickMs) and a sensible range (e.g.
> 0) instead of truncating, and on the read/parser side ensure it uses
Number.isInteger(value) and the same range check so invalid values are rejected
(allowUnsupportedSchema can then handle the failure); update the checks around
record.tickMs and the corresponding parser logic (and remove Math.trunc usage)
to implement this strict validation.
---
Duplicate comments:
In `@apps/cli/src/channels/channelBridgeServerKv.ts`:
- Around line 271-297: The code currently truncates item.lastForwardedSeq
(Math.trunc) which silently accepts non-integer values like 1.9 and can cause
replay/duplicate forwards; change the validation in the channelBridgeServerKv
binding parsing so lastForwardedSeq must be an integer (use Number.isInteger)
and non-negative, throw ChannelBridgeBadPayloadError if not, and stop using
Math.trunc when assigning lastForwardedSeq (use the validated value directly);
update the validation logic that references item.lastForwardedSeq and the
assignment to lastForwardedSeq to reflect this stricter integer requirement.
In `@apps/cli/src/cli/commands/bridge.ts`:
- Around line 224-234: The flags other than --bot-token are currently treated
the same whether omitted or passed with an empty value; update the parsing
around argvValue/args to mirror the --bot-token pattern: for each value-taking
flag (requireTopicsRaw, webhookEnabledRaw, webhookSecret, webhookHost,
webhookPort, tickMsRaw, allowedChatIdsRaw) add a hasXFlag boolean (e.g.,
hasRequireTopicsFlag = args.some(arg => arg === '--require-topics' ||
arg.startsWith('--require-topics='))) and then if the flag is present but
argvValue returned an empty string, reject it (throw or processLogger.error +
exit) instead of treating it as “missing”; apply this change to the parsing code
that defines requireTopicsRaw, webhookEnabledRaw, webhookSecret, webhookHost,
webhookPort, tickMsRaw and allowedChatIdsRaw so explicit empty values are
detected and handled consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e2a57fb1-8747-4dee-88a6-7b17997214b1
📒 Files selected for processing (9)
apps/cli/src/channels/channelBridgeConfig.test.tsapps/cli/src/channels/channelBridgeConfig.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/cli/commands/bridge.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/cli/src/channels/channelBridgeConfig.ts
- apps/cli/src/channels/core/channelBridgeWorker.ts
- apps/cli/src/channels/channelBridgeConfig.test.ts
| if (typeof record.tickMs === 'number' && Number.isFinite(record.tickMs)) { | ||
| out.tickMs = Math.trunc(record.tickMs); | ||
| } |
There was a problem hiding this comment.
Treat tickMs as a validated integer on both read and write paths.
The KV helper still truncates fractional tickMs, and the read path accepts any finite number at all. That means malformed KV can still smuggle values like 1.9 -> 1, 0, or -5 into runtime config, and allowUnsupportedSchema never gets a chance to recover because the parser does not fail.
💡 Suggested fix
- if (typeof record.tickMs === 'number' && Number.isFinite(record.tickMs)) {
- out.tickMs = Math.trunc(record.tickMs);
+ if (record.tickMs !== undefined) {
+ if (
+ typeof record.tickMs !== 'number'
+ || !Number.isFinite(record.tickMs)
+ || !Number.isInteger(record.tickMs)
+ || record.tickMs < 250
+ || record.tickMs > 60_000
+ ) {
+ throw new ChannelBridgeBadPayloadError('Invalid tickMs payload');
+ }
+ out.tickMs = record.tickMs;
}
@@
- if (typeof update.tickMs === 'number' && Number.isFinite(update.tickMs)) {
- const normalizedTickMs = Math.trunc(update.tickMs);
- if (normalizedTickMs < 250 || normalizedTickMs > 60_000) {
+ if (typeof update.tickMs === 'number') {
+ if (
+ !Number.isFinite(update.tickMs)
+ || !Number.isInteger(update.tickMs)
+ || update.tickMs < 250
+ || update.tickMs > 60_000
+ ) {
throw new ChannelBridgeBadPayloadError('Invalid tickMs update payload: must be in [250, 60000]');
}
- next.tickMs = normalizedTickMs;
+ next.tickMs = update.tickMs;
}Also applies to: 563-568
🤖 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 190 - 192, The
KV helper currently truncates fractional tickMs on write (record.tickMs ->
out.tickMs via Math.trunc) and the read/parser accepts any finite number,
letting malformed values like 1.9, 0, or -5 pass; change both paths to enforce a
validated integer: on the write side (where record.tickMs is handled) only
accept Number.isInteger(record.tickMs) and a sensible range (e.g. > 0) instead
of truncating, and on the read/parser side ensure it uses
Number.isInteger(value) and the same range check so invalid values are rejected
(allowUnsupportedSchema can then handle the failure); update the checks around
record.tickMs and the corresponding parser logic (and remove Math.trunc usage)
to implement this strict validation.
|
@coderabbitai review |
|
@greptile review |
✅ Actions performedReview triggered.
|
| function createStableBridgeLocalId(params: Readonly<{ | ||
| providerId: string; | ||
| conversationId: string; | ||
| threadId: string | null; | ||
| sessionId: string; | ||
| text: string; | ||
| messageId?: string; | ||
| }>): string { | ||
| const normalizedMessageId = typeof params.messageId === 'string' ? params.messageId.trim() : ''; | ||
| if (normalizedMessageId.length === 0) { | ||
| throw new Error('Channel bridge inbound messageId is required for stable idempotency'); | ||
| } | ||
|
|
||
| const digest = createHash('sha256') | ||
| .update(params.providerId) | ||
| .update('\u0000') | ||
| .update(params.conversationId) | ||
| .update('\u0000') | ||
| .update(params.threadId ?? '') | ||
| .update('\u0000') | ||
| .update(normalizedMessageId) | ||
| .digest('hex'); | ||
| return `bridge-${digest}`; | ||
| } |
There was a problem hiding this comment.
createStableBridgeLocalId declares sessionId: string and text: string in its parameter object (lines 56-57), but neither is ever fed into the SHA-256 digest. These dead parameters inflate cognitive overhead for future readers who may assume they influence the resulting localId.
More importantly, messageId is typed ?: string (optional) at line 58, but the function throws immediately when it is absent (line 61-62: throw new Error('Channel bridge inbound messageId is required for stable idempotency')). The ChannelBridgeDeps.sendUserMessageToSession interface declares messageId?: string (optional). Any implementation that follows the interface contract and omits messageId will receive an unexpected runtime exception rather than a compile-time error — breaking the contract the optional type is meant to convey.
Consider:
- Removing the unused
sessionIdandtextparameters. - Either making
messageIdrequired in both this function andChannelBridgeDeps.sendUserMessageToSessionto align the type with runtime reality, or providing a safe UUID fallback when it is absent.
| fetchAgentMessagesAfterSeq: async ({ sessionId, afterSeq }) => { | ||
| const runtime = await resolveSessionRuntime(sessionId); | ||
| if (!runtime) { | ||
| return { | ||
| messages: [], | ||
| highestSeenSeq: null, | ||
| }; | ||
| } |
There was a problem hiding this comment.
When resolveSessionRuntime returns null (e.g. the session was deleted, revoked, or is transiently unreachable), the function returns { messages: [], highestSeenSeq: null } without emitting any diagnostic. The bridge worker receives zero messages and no highestSeenSeq to advance with, so the binding's lastForwardedSeq cursor is permanently frozen. Every subsequent tick repeats the same silent no-op: fetchSessionById returns null, the transcript is never fetched, and the binding stalls indefinitely.
Compare this with the analogous store.getBinding failure path in channelBridgeWorker.ts, which both notifies the user in-channel and marks the event seen so polling batches are not replayed. An operator with a bound session that becomes inaccessible has no log-level signal at all to indicate why their Telegram conversation stopped receiving agent replies.
Consider emitting a warning via onWarning so the issue is visible in operator logs and logs can surface why forwarding stalled.
| const settingsWebhookSecret = | ||
| firstParsed([ | ||
| secretsAccount?.webhookSecret, | ||
| webhookAccount?.secret, | ||
| secretsServer?.webhookSecret, | ||
| webhookServer?.secret, | ||
| secretsGlobal?.webhookSecret, | ||
| webhookGlobal?.secret, | ||
| ], readTrimmedString) | ||
| ?? ''; |
There was a problem hiding this comment.
settingsWebhookSecret is resolved via readTrimmedString (line 224), which only checks for a non-empty string. The env-var path below (line 230) correctly uses readWebhookSecretToken, enforcing the alphanumeric/dash/underscore pattern [A-Za-z0-9_-] that startTelegramWebhookRelay requires.
A user who edits settings.json directly (bypassing the CLI) and enters a token with characters outside that set (e.g., spaces or special characters) will see no validation error at config-resolution time. When startTelegramWebhookRelay is called at daemon startup, it will throw "Webhook secret token must match [A-Za-z0-9_-]", causing startChannelBridgeFromEnv to silently fall back to polling mode with only a logger.warn in daemon logs.
This is inconsistent with the webhook host validation (line 239), which validates settings-derived values with isLoopbackHost and falls back to a safe default when invalid. Consider applying the same pattern: validate the settings-derived token with readWebhookSecretToken and fall back to an empty string (no webhook) when the stored token is malformed.
1 participant
Summary
upstream/devSupersedes
Depends on
Notes
feat/telegram-channel-adapter-v2Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
UI/Diagnostics