feat(channel): add daemon bridge spike#4203
Conversation
📋 Review SummaryThis is a well-structured draft design document for channel adapters and web backends to consume 🔍 General Feedback
🎯 Specific Feedback🔵 Low
✅ Highlights
|
| const session = this.ensureSession(sessionId); | ||
| const chunks: string[] = []; | ||
| const onChunk = (sid: string, chunk: string) => { | ||
| if (sid === sessionId) { |
There was a problem hiding this comment.
[Critical] prompt() SSE/HTTP race — text chunks silently dropped
session.prompt() resolves on the HTTP POST response channel while textChunk events arrive on the independent SSE stream via pumpEvents. When the POST resolves before all SSE chunks have been iterated by the for await loop, the finally block removes the onChunk listener and remaining chunks are emitted to no subscriber. The returned text is incomplete with no error signal.
The mock in tests masks this by pushing events synchronously inside session.prompt(), guaranteeing ordering. In production the POST and SSE travel on separate TCP connections with no ordering guarantee.
Suggested fix: Have prompt() await a completion signal from the SSE stream (e.g., a terminal stopReason-bearing session_update) rather than relying on the HTTP response lifetime. Or collect text in handleSessionUpdate into a per-session buffer that prompt() drains after observing the turn-end marker.
— glm-5.1 via Qwen Code /review
|
|
||
| try { | ||
| await session.prompt({ prompt }); | ||
| } finally { |
There was a problem hiding this comment.
[Suggestion] prompt() discards stopReason from PromptResult
The method returns only chunks.join(''), losing the stopReason field that distinguishes end_turn from cancelled or tool_use. Channel/web backends that need to display a "response was interrupted" indicator or handle cancellation differently cannot tell the difference. This is a new API — now is the right time to enrich the return type.
Suggested fix: Return Promise<{ text: string; stopReason?: string }> instead of Promise<string>.
— glm-5.1 via Qwen Code /review
e61eac1 to
0c90189
Compare
|
处理了这轮 review：
有效但需要协议/roadmap 后续处理的点：
验证：
|
4f49825 to
1fe2b04
Compare
1fe2b04 to
da6a35c
Compare
0c90189 to
1f8ff59
Compare
|
Follow-up update after rebasing this PR to main:
Local validation passed:
I resolved the addressed review threads. Two items remain intentionally open:
|
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
| ): Promise<boolean>; | ||
| } | ||
|
|
||
| export interface DaemonChannelSessionFactoryRequest { |
There was a problem hiding this comment.
[Critical] Thread the daemon sessionScope override through this bridge. The SDK/daemon now support per-request sessionScope, and the production daemon default is still single, which coalesces same-workspace session creation. Since this bridge is intended for multi-user channel/web backends, newSession()/loadSession() currently have no way to request isolated daemon sessions even when the channel router maps different senders/threads to different logical sessions. That can silently attach unrelated channel users or web rooms to the same daemon session and leak conversation/tool/permission state across them.
Please add a sessionScope option to the factory request/bridge options and pass the appropriate isolated scope (for example thread) when creating channel/web sessions, with a regression test that two logical channel sessions pass the override instead of inheriting the daemon default.
— gpt-5.5 via Qwen Code /review
| return this.connected; | ||
| } | ||
|
|
||
| private attachSession(session: DaemonChannelSessionClient): void { |
There was a problem hiding this comment.
[Critical] Guard against stale pumps when replacing a session binding. attachSession() aborts the old event controller, but the old async iterator can still yield or finish during the abort race, and pumpEvents()/dropSession() operate only by sessionId. If the same sessionId has already been reattached, an old session_died, stream_error, completion, or error can delete the newly bound session, clear permissions/commands, and abort the new prompt state.
Before handling yielded events or dropping on completion/error, verify the pump is still current, e.g. this.sessions.get(session.sessionId) === session and the stored controller/signal still matches. When replacing a binding, also abort any prompt controllers for that session or reject replacement while a prompt is in flight.
— gpt-5.5 via Qwen Code /review
| controller.abort(); | ||
| } | ||
| }; | ||
| this.on('textChunk', onChunk); |
There was a problem hiding this comment.
[Suggestion] Avoid using bridge-wide EventEmitter listeners as the internal prompt collection mechanism. Each concurrent prompt on a different session adds its own textChunk and sessionDied listener to the same bridge; with more than 10 active sessions this will trigger Node's MaxListenersExceededWarning, and every streamed chunk fans out to every active prompt listener before being filtered.
Consider tracking active prompt state in a map keyed by sessionId and appending/aborting directly from handleSessionUpdate() / dropSession(). You can still emit textChunk for external observers, but the bridge's own collection should not depend on per-prompt global listeners.
— gpt-5.5 via Qwen Code /review
|
|
||
| Channel backend: | ||
|
|
||
| ```bash |
There was a problem hiding this comment.
[Suggestion] Mark these entry points as future/illustrative instead of runnable commands. The document later says the daemon bridge is not wired into qwen channel start yet, and the PR does not add QWEN_CHANNEL_DAEMON_URL, QWEN_WEB_DAEMON_URL, or qwen web-chat-backend wiring. As written, readers can reasonably copy these commands and expect them to work.
Please rename this section to something like Future Entry Points or add a short note before the snippets that these commands are proposed examples, not implemented by this PR.
— gpt-5.5 via Qwen Code /review
| this.sessions.clear(); | ||
| this.requestToSession.clear(); | ||
| for (const controllers of this.activePromptControllers.values()) { | ||
| for (const controller of controllers) { |
There was a problem hiding this comment.
[Suggestion] stop() never emits sessionDied for active sessions
stop() aborts all event controllers and clears all maps, but never emits sessionDied for any active session. Consumers listening for sessionDied to trigger cleanup (reconnecting, notifying users, releasing resources) will never learn that sessions were terminated. The next inbound user message fails with "No daemon session bound" with no prior event or log.
Note: controller.abort() causes pumpEvents to skip the dropSession call (because signal.aborted is true), so neither path emits sessionDied during shutdown.
| for (const controller of controllers) { | |
| stop(): void { | |
| // Emit sessionDied for active sessions before clearing | |
| for (const [sessionId] of this.sessions) { | |
| this.emit('sessionDied', { sessionId, reason: 'bridge_stopped' }); | |
| } | |
| for (const controller of this.eventControllers.values()) { | |
| controller.abort(); | |
| } |
— glm-5.1 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
Code Review: DaemonChannelBridge
Reviewed by mimo-v2.5-pro via qwen review.
Scope: packages/channels/base/src/DaemonChannelBridge.ts (480 lines, new file)
Test coverage: 8/8 tests pass. Build clean (tsc + eslint 0 findings).
Summary
Well-structured bridge design with defensive input validation (isRecord/getString guards), proper resource cleanup (dropSession, stop), and good test coverage for the core happy paths. No Critical issues found.
4 Suggestion-level findings below, all in DaemonChannelBridge.ts. Prior review items (SSE/HTTP race, stopReason discard, EventEmitter pattern, stop() no sessionDied, sessionScope threading, stale pump race, test gaps) are already tracked as open inline comments and not repeated here.
| } | ||
| const requestId = data['requestId']; | ||
| this.requestToSession.set(requestId, sessionId); | ||
| this.emit('permissionRequest', { |
There was a problem hiding this comment.
[Suggestion] handlePermissionRequest validates top-level shape (requestId is string, toolCall is record, options is array) but then casts the entire data as RequestPermissionRequest without validating inner structure.
A malformed permission_request event with an empty {} toolCall or entries missing optionId will produce a typed object with undefined inner fields. Consumers trusting the typed request.toolCall.toolCallId will hit runtime errors.
Suggested fix: Validate required inner fields before casting:
const tc = data['toolCall'] as Record<string, unknown>;
if (typeof tc['toolCallId'] !== 'string' || typeof tc['kind'] !== 'string') {
return;
}Alternatively, use a satisfies check on the emit (as you do for the event shape) to catch structural mismatches at compile time.
| case 'available_commands_update': { | ||
| if (Array.isArray(update['availableCommands'])) { | ||
| const commands = update['availableCommands'] as AvailableCommand[]; | ||
| this.availableCommandsBySession.set(sessionId, commands); |
There was a problem hiding this comment.
[Suggestion] available_commands_update casts update['availableCommands'] as AvailableCommand[] without validating individual elements. If a daemon event sends [null, 42, {name: '__proto__', description: "..."}], these are stored verbatim in availableCommandsBySession and exposed via the typed public API (availableCommands / getAvailableCommands()).
Suggested fix: Filter to valid entries:
const commands = (update['availableCommands'] as unknown[]).filter(
(c): c is AvailableCommand => isRecord(c) && typeof c['name'] === 'string'
);| private readonly options: DaemonChannelBridgeOptions; | ||
| private readonly sessions = new Map<string, DaemonChannelSessionClient>(); | ||
| private readonly eventControllers = new Map<string, AbortController>(); | ||
| private readonly requestToSession = new Map<string, string>(); |
There was a problem hiding this comment.
[Suggestion] this.on('error', () => {}) prevents uncaught-exception crashes (which is good) but silently discards all error context. When pumpEvents catches a non-abort error (line 275), it emits 'error' then calls dropSession — but the error itself is swallowed. At 3AM you'd see "session died" with no idea why.
Suggested fix: Forward errors to a logger or emit a structured event:
this.on('error', (err) => {
this.emit('bridgeError', err);
});Or accept a logger in DaemonChannelBridgeOptions and log here. This gives consumers a way to observe daemon failures without crashing the process.
| this.activePromptControllers.delete(sessionId); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] prompt() returns chunks.join('') which is empty string for both: (1) a successful completion with no text chunks (tool-only response), and (2) session death mid-prompt before any chunks arrived. The stopReason from the daemon's prompt result is also discarded — only raw text is returned.
The caller cannot distinguish "the assistant said nothing" from "the session died". Channel/web backends will silently send blank replies to users when sessions die.
Suggested fix: Return a richer result or throw on session-death abort:
// After the try/finally block:
if (controller.signal.aborted) {
throw new Error(`Session ${sessionId} died during prompt`);
}
return chunks.join('');Or return { text: chunks.join(''), stopReason } instead of a bare string.
wenshao
left a comment
There was a problem hiding this comment.
Code Review: DaemonChannelBridge — Follow-up Verification
Reviewed by mimo-v2.5-pro via qwen review.
Scope: DaemonChannelBridge.ts (480 lines), DaemonChannelBridge.test.ts (623 lines), channel-web.md (120 lines)
Test coverage: 8/8 tests pass. Build clean (tsc + eslint 0 findings).
Verified Fixes (9 of 25 prior findings)
The author addressed 5 critical lifecycle issues and 4 test gaps:
- ✅ session_died cleanup —
dropSessionnow iteratesrequestToSessionand deletes entries for the dead session (line 461). - ✅ pumpEvents normal completion — calls
dropSession(session.sessionId, 'stream_ended')when SSE stream ends (line 283). - ✅ pumpEvents error cleanup — catch block emits error AND calls
dropSession(lines 280-288). - ✅ stream_error/client_evicted — both terminal events handled in switch, calling
dropSession(lines 319-328). - ✅ stale pump guard —
attachSessioncallsexisting?.abort()before registering new session (line 254). - ✅ stop() aborts prompt controllers — iterates
activePromptControllersand aborts each (lines 449-453). - ✅ available_commands_update tested — test case 2 exercises the commands update path.
- ✅ ensureSession throw tested — test case 4 verifies the throw for unknown sessions.
- ✅ pumpEvents error path tested — test case 7 covers generator failure.
Still Open (6 issues from prior review)
These issues remain open and should be addressed before this spike graduates from draft:
_availableCommandsshared state (line 41) — single instance variable overwritten by any session'savailable_commands_update.prompt()discardsstopReason(line 199) — callers cannot distinguishend_turnfromcancelledortool_use.- Error swallowing (line 92) —
this.on('error', () => {})silently discards all error context. stop()never emitssessionDied(line 230) — consumers listening for cleanup are not notified during shutdown.- Image prompt untested — the
imageBase64+imageMimeTypecode path remains uncovered. prompt()empty string ambiguity (line 199) — returns''for both tool-only responses and session death.
Cross-file note
DaemonSessionClient.ts:145 has a lastSeenEventId timing issue (not in this PR's diff) — the property is advanced after yield, so for await...of loops see stale IDs on resume. Separate PR needed.
Verdict: COMMENT
Good progress on critical lifecycle fixes. Remaining issues are suggestions and test gaps that don't block the spike but should be addressed before graduation.
| this.availableCommandsBySession.delete(sessionId); | ||
| this._availableCommands = | ||
| Array.from(this.availableCommandsBySession.values()).at(-1) ?? []; | ||
| for (const [requestId, mappedSessionId] of this.requestToSession) { |
There was a problem hiding this comment.
[Verified Fixed] dropSession now iterates requestToSession and deletes all entries for the dead session. This resolves the prior critical finding about stale permission mappings after session death.
| this.handleEvent(session, event); | ||
| } | ||
| if (!signal.aborted) { | ||
| this.dropSession(session.sessionId, 'stream_ended'); |
There was a problem hiding this comment.
[Verified Fixed] pumpEvents normal completion path now calls dropSession(session.sessionId, 'stream_ended'). This resolves the prior critical finding about zombie sessions after stream end.
| lastEventId: session.lastEventId, | ||
| resume: true, | ||
| })) { | ||
| this.handleEvent(session, event); |
There was a problem hiding this comment.
[Verified Fixed] pumpEvents catch block now emits error AND calls dropSession to clean up session state. This resolves the prior critical finding about leaked sessions after stream errors.
| case 'session_died': | ||
| this.handleSessionDied(session.sessionId, event.data); | ||
| break; | ||
| case 'client_evicted': |
There was a problem hiding this comment.
[Verified Fixed] Both stream_error and client_evicted are now handled in the switch statement, calling dropSession with the appropriate reason. This resolves the prior critical finding about unhandled terminal events.
|
|
||
| private attachSession(session: DaemonChannelSessionClient): void { | ||
| const existing = this.eventControllers.get(session.sessionId); | ||
| existing?.abort(); |
There was a problem hiding this comment.
[Verified Fixed] attachSession now calls existing?.abort() before registering the new session, preventing stale pumps from interfering with the new binding.
| } | ||
| this.eventControllers.get(sessionId)?.abort(); | ||
| this.eventControllers.delete(sessionId); | ||
| this.sessions.delete(sessionId); |
There was a problem hiding this comment.
[Verified Fixed] stop() now iterates activePromptControllers and aborts each controller, resolving the prior critical finding about orphaned prompt controllers during shutdown.
|
Updated #4203 with Handled the latest channel/web bridge review items without changing the existing
Validation passed locally: cd packages/channels/base && npx vitest run src/DaemonChannelBridge.test.ts
cd packages/channels/base && npm run build
cd packages/channels/base && npx eslint src/DaemonChannelBridge.ts src/DaemonChannelBridge.test.ts --max-warnings 0 --no-warn-ignored
cd packages/channels/base && npx prettier --check src/DaemonChannelBridge.ts src/DaemonChannelBridge.test.tsStill intentionally not changing Generated by GPT-5 model |
wenshao
left a comment
There was a problem hiding this comment.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| mimeType: options.imageMimeType, | ||
| }); | ||
| } | ||
| prompt.push({ type: 'text', text }); |
There was a problem hiding this comment.
[Critical] SSE/HTTP race — text chunks silently dropped
prompt() collects text via textChunk events on the SSE stream (through pumpEvents), but waits for completion via a separate HTTP POST (session.prompt()). When the HTTP POST resolves, there is no guarantee that the final textChunk SSE event has already been processed by pumpEvents. This is a race condition between two independent HTTP connections with no ordering guarantee.
Impact: Intermittently truncated or empty responses returned to users. Classic Heisenbug — depends on network timing, daemon load, and event loop scheduling.
| prompt.push({ type: 'text', text }); | |
| // Option A: Wait for turn-complete signal on SSE stream before resolving | |
| // Option B: Add a short yield/drain after session.prompt() resolves: | |
| const result = await session.prompt({ prompt }, controller.signal); | |
| await new Promise(resolve => setImmediate(resolve)); // drain microtask queue | |
| const textResult = chunks.join(''); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return this.connected; | ||
| } | ||
|
|
||
| private attachSession(session: DaemonChannelSessionClient): void { |
There was a problem hiding this comment.
[Critical] attachSession with duplicate sessionId leaves zombie state
When a sessionId already bound gets a new binding, existing?.abort() is called but the old pumpEvents loop skips dropSession() due to the if (!signal.aborted) guard in both the normal completion and error paths. Stale entries in activePrompts, activePromptControllers, requestToSession, and availableCommandsBySession are never cleaned up.
Impact: Orphaned activePrompts entries permanently block new prompts on that sessionId. Orphaned requestToSession entries may route permissions to the wrong session.
| private attachSession(session: DaemonChannelSessionClient): void { | |
| private attachSession(session: DaemonChannelSessionClient): void { | |
| + this.dropSession(session.sessionId, 'session_replaced'); | |
| const existing = this.eventControllers.get(session.sessionId); | |
| existing?.abort(); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } satisfies DaemonPermissionRequestEvent); | ||
| } | ||
|
|
||
| private handlePermissionResolved(data: unknown): void { |
There was a problem hiding this comment.
[Critical] handlePermissionResolved does not verify session ownership
handlePermissionResolved(data) receives only data, not the session parameter. It deletes from requestToSession and emits permissionResolved without verifying that the request belongs to the session whose event stream delivered the event. Cross-session permission interference is possible if the daemon routes events incorrectly.
Impact: A permission resolution from one session can silently clear a pending permission request from another session. respondToPermission() returns false with no indication that another session triggered the resolution.
| private handlePermissionResolved(data: unknown): void { | |
| private handlePermissionResolved(sessionId: string, data: unknown): void { | |
| if (!isRecord(data) || typeof data['requestId'] !== 'string') { | |
| return; | |
| } | |
| const requestId = data['requestId']; | |
| + if (this.requestToSession.get(requestId) !== sessionId) { | |
| + return; | |
| + } | |
| this.requestToSession.delete(requestId); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| async loadSession(sessionId: string, cwd: string): Promise<string> { | ||
| const session = await this.options.sessionFactory({ | ||
| workspaceCwd: cwd || this.options.cwd, |
There was a problem hiding this comment.
[Critical] loadSession returns factory sessionId — may differ from requested
loadSession(sessionId, cwd) returns session.sessionId from the factory, which may differ from the requested sessionId. The caller uses the original sessionId for subsequent operations and gets "No daemon session bound" errors. The test passes only because the fake session hardcodes the same ID.
Impact: Nearly impossible to debug without the original author present. No type safety or runtime check catches this divergence.
| workspaceCwd: cwd || this.options.cwd, | |
| this.attachSession(session); | |
| + if (session.sessionId !== sessionId) { | |
| + throw new Error( | |
| + `Session factory returned ${session.sessionId}, expected ${sessionId}` | |
| + ); | |
| + } | |
| return session.sessionId; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| break; | ||
| } | ||
| case 'available_commands_update': { | ||
| if (Array.isArray(update['availableCommands'])) { |
There was a problem hiding this comment.
[Critical] Silent malformed event drops — hanging states with no error signal
handlePermissionRequest (line 399-405), handleModelSwitched (line 443), and other handlers silently return when validation fails without emitting any error event. If the daemon sends malformed data (missing requestId, non-string modelId), the bridge drops it without notifying anyone.
Impact: Permission flows can hang indefinitely. The channel/web backend never sees the permission request and cannot signal a timeout. Zero observability.
| if (Array.isArray(update['availableCommands'])) { | |
| if (/* validation fails */) { | |
| + this.emit('error', new Error( | |
| + `Malformed permission_request: missing required fields` | |
| + )); | |
| return; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| this.lastError = error; | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] availableCommands getter returns non-deterministic cross-session data
The getter returns commands from latestAvailableCommandsSessionId — the most recently updated session — not the caller's session. In multi-session deployments, this silently returns the wrong session's commands. Use getAvailableCommands(sessionId) for scoped access.
| get availableCommands(): AvailableCommand[] { | |
| + // NOTE: Only safe for single-session deployments. | |
| + // Use getAvailableCommands(sessionId) for multi-session. | |
| if (this.latestAvailableCommandsSessionId) { |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| async setSessionModel( | ||
| sessionId: string, | ||
| modelId: string, | ||
| ): Promise<Record<string, unknown>> { |
There was a problem hiding this comment.
[Suggestion] cancelSession does not abort in-flight prompt's AbortController
cancelSession() calls session.cancel() but does not abort the AbortController that prompt() created for the same session. If a prompt is in flight, the daemon may cancel the turn but the bridge's local AbortController is not signalled, so the prompt() Promise continues waiting.
| ): Promise<Record<string, unknown>> { | |
| async cancelSession(sessionId: string): Promise<void> { | |
| + const controllers = this.activePromptControllers.get(sessionId); | |
| + if (controllers) { | |
| + for (const ctrl of controllers) ctrl.abort(); | |
| + } | |
| await this.ensureSession(sessionId).cancel(); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| DaemonChannelEvent, | ||
| DaemonChannelSessionClient, | ||
| DaemonChannelSessionFactory, | ||
| DaemonChannelSessionFactoryRequest, |
There was a problem hiding this comment.
[Suggestion] DaemonPromptCompleteEvent is not exported
The interface is defined in DaemonChannelBridge.ts and emitted via this.emit('promptComplete', ...) but not exported from index.ts. Consumers listening for promptComplete events cannot type the event payload without importing from the internal source file.
| DaemonChannelSessionFactoryRequest, | |
| + DaemonPromptCompleteEvent, |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
3efff65 to
83777ab
Compare
|
Generated by GPT-5 model Updated #4203 after triaging the latest review comments. Valid fixes included in
Notes from triage:
Local verification:
|
|
Generated by GPT-5 model Marked this PR ready for review after confirming the latest head is mergeable and CI is green across Classify PR, Lint, CodeQL, and macOS/Ubuntu/Windows tests. Requested review from Merge-safety summary: this remains a default-off channel/web daemon bridge spike. It adds the adapter/export and tests, but does not switch existing channel/web runtime paths to daemon. The remaining HTTP/SSE prompt-completion waterline concern is explicitly a protocol-roadmap item, not a hidden default-path behavior change in this PR. |
| ); | ||
| return; | ||
| } | ||
| if (mappedSessionId !== sessionId) { |
There was a problem hiding this comment.
[Critical] handlePermissionResolved leaks requestToSession entries in the cross-session mismatch branch
When mappedSessionId !== sessionId, the method emits a protocol error and returns at line 472 — but never calls this.requestToSession.delete(requestId). The owning session stays alive, so the stale entry is never cleaned up. Repeated mismatched resolution events cause unbounded Map growth (memory leak).
| if (mappedSessionId !== sessionId) { | |
| if (mappedSessionId !== sessionId) { | |
| this.requestToSession.delete(requestId); | |
| this.emitProtocolError( | |
| `Ignoring daemon permission_resolved for request ${requestId} from non-owning session ${sessionId}`, | |
| data, | |
| ); | |
| return; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| this.off('sessionDied', onSessionDied); | ||
| this.activePrompts.delete(sessionId); | ||
| controllers.delete(controller); | ||
| if (controllers.size === 0) { |
There was a problem hiding this comment.
[Critical] prompt() finally block race condition: stale Set reference can delete new prompt's abort controllers
The finally block uses controllers.size === 0 to decide whether to delete from this.activePromptControllers. When a session is replaced mid-prompt: (1) dropSession → abortActivePrompts deletes the old Set from the Map, (2) new prompt() inserts a new Set, (3) the old finally block's captured controllers (the old Set) is empty, so controllers.size === 0 passes — deleting the NEW prompt's controllers and orphaning its abort signal. The new prompt becomes uncancellable.
| if (controllers.size === 0) { | |
| if (this.activePromptControllers.get(sessionId) === controllers) { | |
| this.activePromptControllers.delete(sessionId); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| async cancelSession(sessionId: string): Promise<void> { | ||
| const session = this.ensureSession(sessionId); | ||
| this.abortActivePrompts(sessionId); | ||
| await session.cancel(); |
There was a problem hiding this comment.
[Suggestion] cancelSession does not clear activePrompts Set — mismatch with dropSession
cancelSession calls this.abortActivePrompts(sessionId) but never calls this.activePrompts.delete(sessionId). Compare with dropSession which does both. If the daemon ignores the abort signal and the prompt promise never settles, activePrompts permanently retains the session ID, blocking all future prompts for that session with "Prompt already in flight."
| await session.cancel(); | |
| async cancelSession(sessionId: string): Promise<void> { | |
| const session = this.ensureSession(sessionId); | |
| this.abortActivePrompts(sessionId); | |
| this.activePrompts.delete(sessionId); | |
| await session.cancel(); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| this.activePromptControllers.delete(sessionId); | ||
| } | ||
|
|
||
| private emitProtocolError(message: string, details: unknown): void { |
There was a problem hiding this comment.
[Suggestion] emitProtocolError leaks raw daemon payload to the error channel
The method attaches the original daemon event data as error.details. Daemon payloads may contain file contents, agent responses, or API error details from permission_request, session_update, or model_switch_failed events. If any downstream consumer logs these errors (via console.error or structured logging), sensitive data persists in logs. Consider stripping details or only including known-safe fields (event type, requestId) in the error message itself.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
Generated by GPT-5 model Handled the latest valid channel/web bridge review comments in
Validation run locally:
Notes / triage:
|
| response: RequestPermissionResponse, | ||
| ): Promise<boolean> { | ||
| const sessionId = this.requestToSession.get(requestId); | ||
| if (!sessionId) { |
There was a problem hiding this comment.
[Critical] cancelSession calls abortActivePrompts before session.cancel() — wrong order
Aborting the prompt controller first causes session.prompt() to reject with AbortError immediately, before the daemon receives the cancel signal. If session.cancel() subsequently throws, the real cancellation error is masked — the caller already received AbortError and has no way to know the cancel failed, meaning the daemon session could remain running while the bridge believes it was cancelled.
| if (!sessionId) { | |
| async cancelSession(sessionId: string): Promise<void> { | |
| const session = this.ensureSession(sessionId); | |
| await session.cancel(); | |
| this.abortActivePrompts(sessionId); | |
| } |
— mimo-v2.5-pro via Qwen Code /review
| } | ||
| return session; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Critical] stop() never calls session.cancel() on underlying sessions
dropSession aborts the local event controller and cleans up internal maps, but never tells the daemon to stop the session. Any in-flight daemon-side agent work (prompt processing, tool execution) continues running orphaned on the server. Compare with cancelSession() which explicitly calls session.cancel().
| stop(): void { | |
| for (const sessionId of Array.from(this.sessions.keys())) { | |
| const session = this.sessions.get(sessionId); | |
| if (session) { | |
| void session.cancel().catch(() => {}); | |
| } | |
| this.dropSession(sessionId, 'bridge_stopped'); | |
| } | |
| this.eventControllers.clear(); | |
| this.sessions.clear(); | |
| this.requestToSession.clear(); |
Using void ... .catch() because stop() is synchronous — the cancel is best-effort.
— mimo-v2.5-pro via Qwen Code /review
| response: RequestPermissionResponse, | ||
| ): Promise<boolean> { | ||
| const sessionId = this.requestToSession.get(requestId); | ||
| if (!sessionId) { |
There was a problem hiding this comment.
[Critical] cancelSession calls abortActivePrompts before session.cancel() — wrong order
Aborting the prompt controller first causes session.prompt() to reject with AbortError immediately, before the daemon receives the cancel signal. If session.cancel() subsequently throws, the real cancellation error is masked — the caller already received AbortError and has no way to know the cancel failed, meaning the daemon session could remain running while the bridge believes it was cancelled.
| if (!sessionId) { | |
| async cancelSession(sessionId: string): Promise<void> { | |
| const session = this.ensureSession(sessionId); | |
| await session.cancel(); | |
| this.abortActivePrompts(sessionId); | |
| } |
— mimo-v2.5-pro via Qwen Code /review
| } | ||
| return session; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Critical] stop() never calls session.cancel() on underlying sessions
dropSession aborts the local event controller and cleans up internal maps, but never tells the daemon to stop the session. Any in-flight daemon-side agent work (prompt processing, tool execution) continues running orphaned on the server. Compare with cancelSession() which explicitly calls session.cancel().
| stop(): void { | |
| for (const sessionId of Array.from(this.sessions.keys())) { | |
| const session = this.sessions.get(sessionId); | |
| if (session) { | |
| void session.cancel().catch(() => {}); | |
| } | |
| this.dropSession(sessionId, "bridge_stopped"); | |
| } | |
| this.eventControllers.clear(); | |
| this.sessions.clear(); | |
| this.requestToSession.clear(); |
Using void ... .catch() because stop() is synchronous — the cancel is best-effort.
— mimo-v2.5-pro via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
2 high-confidence Critical findings on session lifecycle management:
(1) cancelSession wrong ordering — abortActivePrompts runs before session.cancel(), masking cancellation failures with AbortError.
(2) stop() leaks daemon sessions — dropSession cleans local state but never calls session.cancel(), leaving daemon-side work orphaned.
Both fixes are shown as inline suggestion blocks.
— mimo-v2.5-pro via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
All previous review findings (25 comments) have been addressed. 15/15 tests pass, build clean, tsc + eslint 0 findings. No new issues found. LGTM! ✅ — mimo-v2.5-pro via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
本地完整对账后发现：6 个 [Verified Fixed] 真修了；R6 的 5 条 critical (08:26 UTC) 由 83777ab22 修完；但 R7-R8 还有 4 条 critical 在 HEAD 72555381e 没修。逐条复用之前的 inline suggestion，请先合到一个修复 commit 再 merge。
本地实测见：验证报告（前次评审已贴）。本次 dismiss 之前的 APPROVED 是因为对账漏了。
|
|
||
| async cancelSession(sessionId: string): Promise<void> { | ||
| const session = this.ensureSession(sessionId); | ||
| this.abortActivePrompts(sessionId); |
There was a problem hiding this comment.
[Critical reaffirmed] cancelSession 顺序仍是先 abort 再 cancel（HEAD 72555381e）。先 abort 会让 session.prompt() 立刻以 AbortError reject，遮蔽随后 session.cancel() 抛出的真实错误 —— 调用方收到的是 AbortError，无从得知 daemon 端取消是否真的成功。
| this.abortActivePrompts(sessionId); | |
| async cancelSession(sessionId: string): Promise<void> { | |
| const session = this.ensureSession(sessionId); | |
| await session.cancel(); | |
| this.abortActivePrompts(sessionId); | |
| } |
|
|
||
| stop(): void { | ||
| for (const sessionId of Array.from(this.sessions.keys())) { | ||
| this.dropSession(sessionId, 'bridge_stopped'); |
There was a problem hiding this comment.
[Critical reaffirmed] stop() → dropSession 通路从不发 daemon 端 session.cancel()。bridge 关闭后 daemon 侧 prompt/tool 仍在跑，得等 daemon 自己 health-check 兜底。cancelSession() 有调 session.cancel()，对照不一致。
| this.dropSession(sessionId, 'bridge_stopped'); | |
| stop(): void { | |
| for (const sessionId of Array.from(this.sessions.keys())) { | |
| const session = this.sessions.get(sessionId); | |
| if (session) { | |
| void session.cancel().catch(() => {}); | |
| } | |
| this.dropSession(sessionId, 'bridge_stopped'); | |
| } |
stop() 是同步签名所以走 void ... .catch()，best-effort。
| this.off('sessionDied', onSessionDied); | ||
| this.activePrompts.delete(sessionId); | ||
| controllers.delete(controller); | ||
| if (controllers.size === 0) { |
There was a problem hiding this comment.
[Critical reaffirmed] finally 块用 controllers.size === 0 判断后按 key 删 activePromptControllers，但 controllers 是闭包里捕获的旧 Set。die-and-reattach 后新 prompt 写入了新 Set，旧 finally 触发时会按 sessionId key 误删新 Set，新 prompt 失去 cancel 通路。
| if (controllers.size === 0) { | |
| if (this.activePromptControllers.get(sessionId) === controllers) { | |
| this.activePromptControllers.delete(sessionId); | |
| } |
用同一性比较守门，避免按 key 盲删。
| ); | ||
| return; | ||
| } | ||
| if (mappedSessionId !== sessionId) { |
There was a problem hiding this comment.
[Critical reaffirmed] mappedSessionId !== sessionId 分支只 emitProtocolError + return，从不 requestToSession.delete(requestId)。daemon 重复发错误归属的 permission_resolved 会让 Map 无界增长（虽然窄但可被反复触发）。把 delete 挪到 mismatch 分支里：
| if (mappedSessionId !== sessionId) { | |
| if (mappedSessionId !== sessionId) { | |
| this.requestToSession.delete(requestId); | |
| this.emitProtocolError( | |
| `Ignoring daemon permission_resolved for request ${requestId} from non-owning session ${sessionId}`, | |
| data, | |
| ); | |
| return; | |
| } |
| this.requestToSession.delete(requestId); | ||
| return false; | ||
| } | ||
| return await session.respondToPermission(requestId, response); |
There was a problem hiding this comment.
[Suggestion] respondToPermission 在 session.respondToPermission() 抛出异常时不清除 requestToSession 映射
当 daemon 的 respondToPermission 调用失败（网络错误、超时），requestToSession 中的条目会被保留。调用方的后续重试可能在陈旧映射仍然存在的情况下重新尝试操作，导致权限响应在 daemon 侧被重复处理。
| return await session.respondToPermission(requestId, response); | |
| try { | |
| return await session.respondToPermission(requestId, response); | |
| } catch { | |
| this.requestToSession.delete(requestId); | |
| throw; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return await session.respondToPermission(requestId, response); | ||
| } | ||
|
|
||
| stop(): void { |
There was a problem hiding this comment.
[Suggestion] stop() 在 dropSession 已执行每会话清理后仍存在冗余的 .clear() 调用
stop() 遍历所有会话并调用 dropSession()，后者已从 sessions、eventControllers、activePrompts、availableCommandsBySession 和 requestToSession 中删除条目。随后对 .clear() 的调用属于空操作；对 activePromptControllers 的 abort 循环更是死代码（控制器已在 abortActivePrompts 中被中止并移除）。这种冗余掩盖了 dropSession 中的潜在 bug：若 dropSession 在某条路径上遗漏了清理，则 stop() 路径会通过，而单独调用 dropSession 的路径（stream_ended、session_died）仍会泄漏状态。
建议： 从 stop() 中移除冗余的 .clear() 调用（第 298–310 行），仅保留必要的批量控制器 abort。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| this.emit('sessionUpdate', data); | ||
| } | ||
|
|
||
| private handlePermissionRequest(sessionId: string, data: unknown): void { |
There was a problem hiding this comment.
[Suggestion] requestToSession Map 无驱逐机制 —— 未解析的权限请求会无限增长
handlePermissionRequest 每次权限请求都会向 requestToSession 写入条目，仅在 permission_resolved、dropSession 或 respondToPermission（仅限会话不存在时）时移除。没有超时、TTL 或容量限制。若 daemon 产生大量永不解析的权限请求，该 Map 将持续增长，导致缓慢的内存泄漏。
建议： 引入基于超时的驱逐机制（例如 5 分钟内未解析的请求自动清除并发出协议错误），或设置 Map 的最大容量限制。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| controller.abort(); | ||
| } | ||
| }; | ||
| this.on('textChunk', onChunk); |
There was a problem hiding this comment.
[Suggestion] prompt() 监听器注册窗口 —— loadSession 恢复时重放的数据块可能丢失
attachSession() 以 resume: true 启动 pumpEvents，导致 daemon 重放历史事件。但 textChunk 监听器（第 230 行）仅在稍后的 prompt() 中注册。在 loadSession 返回与 prompt() 被调用之间，重放的 agent_message_chunk 事件通过 this.emit('textChunk', ...) 发出，但因尚无监听器注册而静默丢失。对于全新会话（newSession）影响较小，但对会话恢复（loadSession）存在实际风险。
建议： 在 attachSession 时即为每个 session 预注册文本块缓冲区，在 prompt() 时排出已缓冲的块；或者延迟启动 pump 直到 prompt() 被调用时。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| await new Promise<void>((resolve) => setTimeout(resolve, 0)); | ||
| } | ||
|
|
||
| export class DaemonChannelBridge extends EventEmitter { |
There was a problem hiding this comment.
[Suggestion] EventEmitter 缺少类型化事件映射
DaemonChannelBridge extends EventEmitter 未使用 Node.js 支持的泛型事件映射参数。导致所有 this.on('textChunk', cb) 中的 cb 推断为 (...args: any[]) => void 而非正确的 (sessionId: string, text: string) => void。事件名称拼写错误（如 'textChunkk'）不会产生编译错误。
建议： 定义事件映射接口并使用 EventEmitter<DaemonChannelBridgeEvents>：
interface DaemonChannelBridgeEvents {
textChunk: (sessionId: string, text: string) => void;
thoughtChunk: (sessionId: string, text: string) => void;
sessionDied: (info: { sessionId: string; reason: string }) => void;
// ... 其他事件
}— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| break; | ||
| } | ||
| case 'tool_call': |
There was a problem hiding this comment.
[Suggestion] handleSessionUpdate 中 tool_call/tool_call_update 错误格式事件缺少验证
此前几轮修复已为畸形事件添加了 emitProtocolError 调用，覆盖了 session_update 外部包装器、permission_request、permission_resolved、model_switched、model_switch_failed 和 available_commands_update。但 tool_call/tool_call_update 分支（第 437–450 行）在未进行任何验证的情况下静默构造 ToolCallEvent 对象——缺失的字段以空字符串回退。一个缺失 toolCallId 的 daemon 事件会生成 toolCallId: '' 并静默 emit，而非发出协议错误。这与其余处理程序现已一致的显式错误信号模式不符。
建议： 在 toolCallId 或 kind 缺失时调用 emitProtocolError，与 permission_request 和 model_switched 的模式保持一致。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
7255538 to
1381074
Compare
|
Generated by GPT-5 model Handled the latest valid channel/web daemon bridge review comments in Fixes included:
Triage/deferred items:
Local validation run after the change:
CI status at this check: the new head has entered CI; Classify PR is queued, with downstream jobs pending. |
|
|
||
| try { | ||
| const result = await session.prompt({ prompt }, controller.signal); | ||
| await drainDaemonEventLoop(); |
There was a problem hiding this comment.
🟠 Medium — drainDaemonEventLoop 只是单 tick 启发式
你在评论里已经点出真正解决方案要等 daemon 那边补 turn-complete SSE frame / prompt response 的 lastEventId waterline，但代码里没有 TODO 也没有链接，后人很容易误以为这是稳态实现。
建议加一行注释，挂上 #4201(或 daemon roadmap issue)说明这是临时方案、何时可移除。
// TODO(#4201): replace single-tick drain once daemon emits a terminal
// turn-complete SSE frame or prompt response carries a lastEventId.
await drainDaemonEventLoop();| return false; | ||
| } | ||
| try { | ||
| return await session.respondToPermission(requestId, response); |
There was a problem hiding this comment.
🟠 Medium — 成功路径未立即清理 requestToSession
现在 success 只依赖后续 permission_resolved 事件来清 map:
try {
return await session.respondToPermission(requestId, response);
} catch (error) {
this.requestToSession.delete(requestId);
throw error;
}如果 daemon 漏发 permission_resolved(网络抖动 / daemon bug),同一个 requestId 第二次调用会再次走到 session.respondToPermission,可能拿到一个不该有的结果(mock 端返回 true,真 daemon 端可能 reject 也可能 idempotent 通过)。
两个备选:
- 成功也清掉 map(最简单);
- 引入
已响应未 resolved中间态,二次调用直接返回 false。
倾向选 1,逻辑更对称。
| this.requestToSession.delete(requestId); | ||
| this.emit('permissionResolved', { | ||
| requestId, | ||
| outcome: data['outcome'], |
There was a problem hiding this comment.
🟠 Medium — outcome 字段没有形状校验
这里仅校验了 requestId 是 string,outcome 直接透传 data['outcome'],可能是 undefined 或任意结构,会把 daemon 的 garbage 转手给消费方。
建议要么在这里做最小形状收敛(比如要求 outcome 是 record 且有 outcome 子字段),要么在 doc/type 上明确消费方需要自己处理 undefined / 任意 shape。
| private readonly sessions = new Map<string, DaemonChannelSessionClient>(); | ||
| private readonly eventControllers = new Map<string, AbortController>(); | ||
| private readonly requestToSession = new Map<string, string>(); | ||
| private readonly activePrompts = new Set<string>(); |
There was a problem hiding this comment.
🟡 Low — activePrompts Set 与 activePromptControllers Map 冗余
既然实现已经拒绝同一 session 并发 prompt(line 226),activePromptControllers.get(sid).size 最多就是 1,外层再套一个 Set<AbortController> 是过度设计。
建议改成 Map<sessionId, AbortController>:
- 并发判断直接
this.activePromptControllers.has(sessionId),可以删掉activePromptsSet; - finally 清理变成
if (this.activePromptControllers.get(sessionId) === controller) this.activePromptControllers.delete(sessionId)。
当前代码没有 bug,纯可读性改进。
| } | ||
| } | ||
|
|
||
| private handleSessionUpdate(sessionId: string, data: unknown): void { |
There was a problem hiding this comment.
🟡 Low — handleSessionUpdate 没校验 data.sessionId 与 pump 一致
这里的 sessionId 取自 pump 上下文(外层调用方传入),完全没看 event payload 里的 data.sessionId。daemon 受信,短期没问题;但加一行 defensive assert 能让 daemon 协议错乱时大声 fail,而不是静默把别的 session 的 update 当成本 session 的:
if (isRecord(data) && typeof data['sessionId'] === 'string' && data['sessionId'] !== sessionId) {
this.emitProtocolError('session_update sessionId mismatch', data);
return;
}| return summary; | ||
| } | ||
|
|
||
| async function drainDaemonEventLoop(): Promise<void> { |
There was a problem hiding this comment.
🔵 Nit — 单调用方的顶层函数
drainDaemonEventLoop 只有 line 267 一个调用点,可以内联或挪进类里。当前位置看起来像是个公共 utility,反而误导。
|
|
||
| export interface DaemonChannelEvent { | ||
| id?: number; | ||
| v: 1; |
There was a problem hiding this comment.
🔵 Nit — v: 1 字面量缺乏版本演进策略说明
写死 1 暗示有 v2、v3 演进,但没文档/注释说明协议版本的兼容策略。建议要么放一条注释指向 daemon 协议版本规范,要么改成 v: 1 | 2(等 v2 出现时)。
此外这个 interface 可以考虑放进 @qwen-code/daemon-protocol 之类的共享包,避免 channel-base 和 daemon client 各自 redeclare。
| | ---------------------------------------- | -------------------------------------- | | ||
| | `session_update` / `agent_message_chunk` | Append assistant text | | ||
| | `session_update` / `agent_thought_chunk` | Optional hidden/debug stream | | ||
| | `session_update` / `tool_call` | Emit tool status card/message | |
There was a problem hiding this comment.
🟡 Low — 事件映射表与实现不一致
表格只列了 tool_call,但实现同时处理 tool_call 和 tool_call_update(DaemonChannelBridge.ts:484-503)。建议补一行或合并表述:
| `session_update` / `tool_call` / `tool_call_update` | Emit tool status card/message |
| Channel backend: | ||
|
|
||
| ```bash | ||
| QWEN_CHANNEL_DAEMON_URL=http://127.0.0.1:4170 qwen channel start telegram |
There was a problem hiding this comment.
🟡 Low — 端口写死 4170
例子里 http://127.0.0.1:4170 是字面量,没提端口可配置。在多 daemon 部署(比如 per-channel-thread / per-workspace)下容易冲突。
建议加一句:端口可通过 daemon 启动参数 / 配置文件调整,这里只用 4170 作为示例默认值。
|
|
||
| ## Blockers Before Default Migration | ||
|
|
||
| - Per-request `sessionScope`. |
wenshao
left a comment
There was a problem hiding this comment.
No new high-confidence findings. All previously-raised critical issues (cancelSession ordering, stop() calling session.cancel(), dropSession cleanup, pumpEvents error handling, stale pump guards, sessionScope threading, stream_error/client_evicted handling) have been addressed in the latest commit.
Build clean, 15/15 tests pass, tsc + eslint 0 findings.
Remaining open items are suggestions and test gaps (not blockers for a draft spike PR).
— mimo-v2.5-pro via Qwen Code /review
|
Generated by GPT-5 Codex Processed the latest valid #4203 channel/web daemon bridge review nits with a narrow bridge-only change:
Triage notes:
Local validation:
|
wenshao
left a comment
There was a problem hiding this comment.
前次 CHANGES_REQUESTED 的 4 条 critical 在 HEAD b24c2e2ce 全部对账通过：
- L316-321
cancelSession—— 顺序按建议改成await session.cancel(); abortActivePrompts(...);，且多加了activePrompts.delete(sessionId)兜底 - L360-372
stop()—— 每个 session 先void session.cancel().catch(e => this.lastError = e)，再dropSession；stop()同步签名下走 best-effort，错误存lastError而非丢弃，比我建议的还细 - L307-311
prompt()finally —— 用this.activePromptControllers.get(sessionId) === controllers同一性比较守门，逐字采纳建议 - L592-594
handlePermissionResolvedmismatch —— mismatch 分支里requestToSession.delete+respondedRequestToSession.delete都做；外加引入二级 maprespondedRequestToSession让 ownership 判断在 delete 后仍可续，是建议范围外的额外健壮性
本地实测：
npx vitest run DaemonChannelBridge.test.ts→ 17/17 passed（新加 "clears permission ownership when daemon permission responses fail" + "rejects malformed permission resolution outcomes" 两条回归）npx eslintclean /npm run buildexit 0- Dist-level smoke 重跑：SSE/HTTP race / 并发 prompt 拒绝 / unknown id 拒绝 /
stop()流程 / 错误通道 全部 PASS
LGTM。
Summary
DaemonChannelBridgein@qwen-code/channel-basefor server-side channel/web backends to bind daemon sessions, consume daemon SSE events, collect assistant text, and route permission/cancel/model operations.qwen channel startstill uses the existing ACP bridge by default.Validation
cd packages/channels/base && npx vitest run src/DaemonChannelBridge.test.ts.src/DaemonChannelBridge.test.ts (4 tests)andTest Files 1 passed.Scope / Risk
qwen servesmoke, noqwen channel startflag/env wiring, no browser websocket/SSE BFF route, no per-requestsessionScope, and no default channel migration.Engineering principles checklist (Stage 1.5 wave PRs)
qwen serveStage 1 routes / SDK behavior preservedLinked Issues / Bugs
Related to #4175 Wave 5 adapter track and #4201.