feat(channel): add daemon bridge wire-up#4261
Conversation
📋 Review SummaryThis PR introduces an opt-in daemon bridge path for 🔍 General Feedback
🎯 Specific Feedback🔴 Critical
🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
50d5416 to
933a9d8
Compare
wenshao
left a comment
There was a problem hiding this comment.
Critical: Daemon bridge path has zero test coverage. createBridge daemon branch (dynamic import of @qwen-code/sdk, DaemonClient construction, DaemonChannelBridge creation with sessionFactory) is completely untested. resolveDaemonUrl, resolveDaemonToken, toDaemonSessionScope are pure functions with no unit tests. The @qwen-code/channel-base vi.mock does not export DaemonChannelBridge. All mock objects in SessionRouter.test.ts, ChannelBase.test.ts, and start.test.ts do not satisfy the ChannelBridge interface (missing cancelSession, stop, isConnected).
| writeStdoutLine(`[Channel] "${name}" is running. Press Ctrl+C to stop.`); | ||
|
|
||
| const attachDisconnectHandler = (b: AcpBridge): void => { | ||
| const attachDisconnectHandler = (b: ChannelBridge): void => { |
There was a problem hiding this comment.
[Critical] Crash recovery never triggers for daemon bridge — DaemonChannelBridge never emits disconnected
attachDisconnectHandler listens for bridge.on('disconnected', ...). AcpBridge emits 'disconnected' when the child process exits. DaemonChannelBridge has no child process and never emits 'disconnected' — across all 12 emit() calls in the class, none are 'disconnected'. When the daemon connection is lost (SSE stream error, network blip, daemon restart), the crash recovery loop never fires. The channel process stays alive with isConnected === true but is permanently non-functional.
| const attachDisconnectHandler = (b: ChannelBridge): void => { | |
| // DaemonChannelBridge must emit 'disconnected' when connectivity is lost. | |
| // Trigger point: when the last session is dropped due to a transport-level | |
| // error (not a graceful end). In dropSession(), after all sessions are gone: | |
| if (this.sessions.size === 0 && this.connected) { | |
| this.connected = false; | |
| this.emit('disconnected'); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| bridge = new AcpBridge(bridgeOpts); | ||
| await bridge.start(); | ||
| bridge = await createBridge(bridgeOpts, options, config.sessionScope); | ||
| router.setBridge(bridge); |
There was a problem hiding this comment.
[Critical] Single restart failure = channel permanently zombie
The catch block in the disconnected handler logs the error and returns — it does not retry, does not re-attach the disconnected listener, and does not exit the process. The old bridge reference b is dead. The channel's await new Promise(() => {}) still runs — process stays alive but the channel is non-operational. Combined with the daemon disconnected gap, a single transient failure leaves a permanently dead channel with no recovery path.
| router.setBridge(bridge); | |
| // Add retry with backoff before giving up: | |
| let retries = 0; | |
| while (retries < 3) { | |
| try { | |
| bridge = await createBridge(bridgeOpts, options, config.sessionScope); | |
| router.setBridge(bridge); | |
| channel.setBridge(bridge); | |
| registerToolCallDispatch(bridge, router, channels); | |
| attachDisconnectHandler(bridge); | |
| break; | |
| } catch (err) { | |
| retries++; | |
| if (retries >= 3) throw err; | |
| await new Promise((r) => setTimeout(r, RESTART_DELAY_MS * retries)); | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return bridge; | ||
| } | ||
|
|
||
| const { DaemonClient, DaemonSessionClient } = await import('@qwen-code/sdk'); |
There was a problem hiding this comment.
[Critical] Dynamic import of @qwen-code/sdk has no error handling
When daemonUrl is set, createBridge does await import('@qwen-code/sdk') without try/catch. If the package is missing or incompatible, the raw Cannot find module error propagates to the caller with no context about why the import was attempted or how to fix it. An operator who sets QWEN_DAEMON_URL inadvertently gets a cryptic error with no mention of daemon or the URL.
| const { DaemonClient, DaemonSessionClient } = await import('@qwen-code/sdk'); | |
| try { | |
| const { DaemonClient, DaemonSessionClient } = await import('@qwen-code/sdk'); | |
| } catch (err) { | |
| throw new Error( | |
| `Daemon bridge requires @qwen-code/sdk (daemon URL: ${options.daemonUrl}). ` + | |
| `Ensure the SDK package is installed. Original error: ${err instanceof Error ? err.message : String(err)}` | |
| ); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return cliDaemonToken || process.env['QWEN_SERVER_TOKEN']; | ||
| } | ||
|
|
||
| function toDaemonSessionScope(scope: SessionScope | undefined): 'thread' { |
There was a problem hiding this comment.
[Critical] toDaemonSessionScope silently discards user configuration
This function always returns 'thread' regardless of input. void scope; exists solely to suppress the unused-variable warning. If a channel is configured with sessionScope: 'user' or 'single' and uses the daemon bridge, that configuration is silently dropped — no warning, no error. Meanwhile SessionRouter still routes sessions using the configured scope, creating a mismatch that is nearly impossible to diagnose.
| function toDaemonSessionScope(scope: SessionScope | undefined): 'thread' { | |
| // Rename to make intent explicit: | |
| function forceThreadScopeForDaemon(channelScope?: SessionScope): 'thread' { | |
| if (channelScope && channelScope !== 'thread') { | |
| writeStderrLine( | |
| `[Channel] Daemon bridge forces 'thread' session scope ` + | |
| `(channel configured '${channelScope}').` | |
| ); | |
| } | |
| return 'thread'; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| export class AcpBridge extends EventEmitter { | ||
| export interface ChannelBridge extends EventEmitter { |
There was a problem hiding this comment.
[Critical] DaemonChannelBridge does not declare implements ChannelBridge
AcpBridge declares implements ChannelBridge but DaemonChannelBridge relies on structural typing only. If ChannelBridge gains a new method or changes a signature, TypeScript will not flag DaemonChannelBridge — the mismatch will only surface as a runtime error at the createBridge(): Promise<ChannelBridge> return site.
| export interface ChannelBridge extends EventEmitter { | |
| export class DaemonChannelBridge extends EventEmitter implements ChannelBridge { |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| export class AcpBridge extends EventEmitter { | ||
| export interface ChannelBridge extends EventEmitter { | ||
| readonly availableCommands: AvailableCommand[]; |
There was a problem hiding this comment.
[Critical] availableCommands semantics diverge between bridge implementations
AcpBridge.availableCommands returns globally stored commands shared across all sessions. DaemonChannelBridge.availableCommands returns commands from the most recent session that emitted available_commands_update. In a multi-session daemon deployment (multiple users/chats active), /help may display commands from a different user's session — a race-condition-level bug that is intermittent and hard to reproduce.
| readonly availableCommands: AvailableCommand[]; | |
| // Option A: Make availableCommands session-aware in the interface: | |
| readonly availableCommands: AvailableCommand[]; // Keep for backward compat | |
| // Option B: DaemonChannelBridge should merge commands across sessions | |
| // instead of tracking only the latest session's commands. | |
| // Option C: ChannelBase should call bridge.getAvailableCommands(sessionId) | |
| // before /help logic instead of using the session-less getter. |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| @@ -254,8 +336,7 @@ async function startSingle(name: string, proxy?: string): Promise<void> { | |||
| await new Promise((r) => setTimeout(r, RESTART_DELAY_MS)); | |||
There was a problem hiding this comment.
[Suggestion] Old bridge event listeners leak on crash recovery
Crash recovery creates a new bridge and overwrites the bridge variable without calling removeAllListeners() on the old instance. The old bridge's disconnected and toolCall listeners — which close over router, channels, and shuttingDown — prevent GC of the old bridge and all its internal state.
| await new Promise((r) => setTimeout(r, RESTART_DELAY_MS)); | |
| const oldBridge = bridge; | |
| bridge = await createBridge(bridgeOpts, options, config.sessionScope); | |
| oldBridge.removeAllListeners(); | |
| router.setBridge(bridge); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| }, | ||
| }); | ||
| await bridge.start(); | ||
| writeStdoutLine(`[Channel] Using daemon bridge at ${options.daemonUrl}.`); |
There was a problem hiding this comment.
[Suggestion] Daemon URL logged to stdout leaks embedded credentials
writeStdoutLine(\[Channel] Using daemon bridge at ${options.daemonUrl}.`) outputs the full URL. If users embed credentials in the URL (http://user:pass@host`), they are written to stdout, potentially captured in logs, CI output, or terminal scrollback.
| writeStdoutLine(`[Channel] Using daemon bridge at ${options.daemonUrl}.`); | |
| try { | |
| const origin = new URL(options.daemonUrl).origin; | |
| writeStdoutLine(`[Channel] Using daemon bridge at ${origin}.`); | |
| } catch { | |
| writeStdoutLine(`[Channel] Using daemon bridge.`); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| function resolveDaemonToken(cliDaemonToken?: string): string | undefined { | ||
| return cliDaemonToken || process.env['QWEN_SERVER_TOKEN']; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] QWEN_SERVER_TOKEN co-opted as daemon token creates confused deputy
resolveDaemonToken falls back to QWEN_SERVER_TOKEN. If this env var is set for another purpose (e.g., API authentication), it is silently sent to the daemon URL as a Bearer token. Combined with an attacker-controlled QWEN_DAEMON_URL, this exfiltrates the server token.
| // Use a daemon-specific env var instead: | |
| function resolveDaemonToken(cliDaemonToken?: string): string | undefined { | |
| return cliDaemonToken || process.env['QWEN_DAEMON_TOKEN']; | |
| } | |
| // If backward compat with QWEN_SERVER_TOKEN is needed, add a deprecation warning. |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return await DaemonSessionClient.createOrAttach(client, { | ||
| workspaceCwd: req.workspaceCwd, | ||
| modelServiceId: req.modelServiceId, | ||
| sessionScope: toDaemonSessionScope(req.sessionScope), |
There was a problem hiding this comment.
[Critical] This only works against daemons that honor the per-request sessionScope override, but the client never checks that capability. The SDK documents that older daemons silently ignore sessionScope; those daemons default to single, so multiple distinct channel conversations can be coalesced into one daemon session while SessionRouter believes it created separate sessions. That breaks the core routing invariant this code is trying to enforce. Please call client.capabilities() before enabling daemon mode and fail fast unless features includes session_scope_override (or use a compatibility path that cannot coalesce sessions).
— gpt-5.5 via Qwen Code /review
| baseUrl: options.daemonUrl, | ||
| token: options.daemonToken, | ||
| }); | ||
| const bridge = new DaemonChannelBridge({ |
There was a problem hiding this comment.
[Critical] The daemon bridge emits permissionRequest and exposes respondToPermission(), but this wire-up never registers a handler for it. The ACP bridge currently auto-approves requestPermission, so any permission-gated tool works in the default path; in daemon mode, the same prompt will leave the permission request unanswered until the daemon times out or cancels it. Please preserve the existing channel behavior by wiring a daemon permissionRequest listener that selects the same proceed_once/first-option policy and calls bridge.respondToPermission(...), or move that policy into the shared bridge abstraction so both bridges implement it consistently.
— gpt-5.5 via Qwen Code /review
|
Architecture update after the 2026-05-19 decision: this draft no longer matches the current mainline. Channel should continue to use the existing Given that #4203 already provides a default-off server-side bridge spike, I do not think this wire-up should move toward merge as-is. Recommendation: close/defer this draft, and only mine reusable pieces later if/when channel daemon migration is reopened. Generated by GPT-5 Codex |
| "@modelcontextprotocol/sdk": "^1.25.1", | ||
| "@qwen-code/channel-base": "file:../channels/base", | ||
| "@qwen-code/channel-dingtalk": "file:../channels/dingtalk", | ||
| "@qwen-code/sdk": "file:../sdk-typescript", |
There was a problem hiding this comment.
[Suggestion] @qwen-code/sdk should be an optional dependency, not a hard dependency
The SDK is only needed for the opt-in daemon bridge path. Every user pays the install cost (and transitive deps like @modelcontextprotocol/sdk, zod) even when using the default ACP bridge. The code already uses await import('@qwen-code/sdk') (dynamic import) — signaling the intent is optional. If the SDK fails to build or introduces a breaking change, it blocks the default ACP path for all channel users.
| "@qwen-code/sdk": "file:../sdk-typescript", |
Move "@qwen-code/sdk": "file:../sdk-typescript" from dependencies to optionalDependencies.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| return ( | ||
| cliDaemonUrl || | ||
| process.env['QWEN_CHANNEL_DAEMON_URL'] || | ||
| process.env['QWEN_DAEMON_URL'] |
There was a problem hiding this comment.
[Suggestion] QWEN_DAEMON_URL env var is too generic — risks silent activation of daemon bridge
resolveDaemonUrl() checks three sources: --daemon-url → QWEN_CHANNEL_DAEMON_URL → QWEN_DAEMON_URL. The last one is a very generic env var name. If a user has QWEN_DAEMON_URL set for an unrelated purpose (another tool, a CI pipeline variable), qwen channel start will silently activate the daemon bridge instead of the default ACP bridge, resulting in confusing connection failures.
| process.env['QWEN_DAEMON_URL'] | |
| function resolveDaemonUrl(cliDaemonUrl?: string): string | undefined { | |
| return ( | |
| cliDaemonUrl || | |
| process.env['QWEN_CHANNEL_DAEMON_URL'] | |
| ); | |
| } |
Remove the QWEN_DAEMON_URL fallback, keeping only the unambiguous QWEN_CHANNEL_DAEMON_URL and the explicit --daemon-url CLI flag.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| }; | ||
| let bridge = new AcpBridge(bridgeOpts); | ||
| await bridge.start(); | ||
| let bridge = await createBridge(bridgeOpts, options, 'thread'); |
There was a problem hiding this comment.
[Suggestion] startAll hardcodes 'thread' scope — inconsistent with startSingle
startSingle (line 282) passes config.sessionScope to createBridge, while startAll hardcodes 'thread'. Today this is masked because toDaemonSessionScope() discards the argument entirely. But when that function is fixed to respect its input, the multi-channel daemon path in startAll will still use 'thread' regardless of per-channel configuration.
Consider deriving the bridge-level scope from parsed configs, or let createBridge not take a sessionScope parameter and let the daemon bridge resolve scope per-session from DaemonChannelSessionFactoryRequest (which already carries sessionScope).
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
|
Closing this draft per the latest daemon roadmap decision: channel should keep the existing ACP path for now, while daemon client validation moves to remote web chat + web terminal first. This branch remains useful only as a future/reference spike if channel daemon migration is reopened. Generated by GPT-5 Codex |
Summary
qwen channel startvia--daemon-url,QWEN_CHANNEL_DAEMON_URL, orQWEN_DAEMON_URL. The existing ACP bridge remains the default. Channel internals now depend on a structuralChannelBridgeinterface so either the existingAcpBridgeor the newDaemonChannelBridgecan drive sessions.AcpBridgepath is unchanged, daemon mode is explicit, and routing still stays owned bySessionRouterrather than daemon-level single-session coalescing.Validation
channel start --helpqwen channel startadvertises the daemon bridge flags.--daemon-urland--daemon-token.main, package-local CLI typecheck is blocked by unrelatedgoalCommanderrors from files not touched by this PR:GoalTerminalKindno longer accepts"failed".Scope / Risk
Testing Matrix
Testing matrix notes:
npm run dev -- channel start --help.Linked Issues / Bugs