feat(ide): add experimental daemon webview path#4267
Conversation
📋 Review SummaryThis PR introduces an experimental daemon-backed webview path for the VS Code IDE companion, allowing the IDE to route through a running 🔍 General Feedback
🎯 Specific Feedback🔴 CriticalNo critical issues identified that would block merging. 🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
| "order": 7, | ||
| "type": "boolean", | ||
| "default": false, | ||
| "description": "Experimental: route the IDE webview through a running qwen serve daemon instead of spawning a local ACP child process." |
There was a problem hiding this comment.
[Critical] These daemon settings are contributed without an application/user scope, so a workspace can set qwen-code.experimentalDaemonIde, qwen-code.daemonUrl, and qwen-code.daemonToken in .vscode/settings.json. Opening an untrusted repo can therefore opt the extension into the daemon-backed IDE path and point it at an attacker-controlled loopback daemon, which will receive prompts/workspace context and permission responses. Please make these daemon connection settings application-scoped, or otherwise ignore workspace values when constructing the daemon-backed connection.
— gpt-5.5 via Qwen Code /review
| this.wireDaemonCallbacks(); | ||
| const options = this.optionsProvider(); | ||
| const baseUrl = options.baseUrl || 'http://127.0.0.1:4170'; | ||
| await this.daemon.connect({ |
There was a problem hiding this comment.
[Critical] The daemon IDE adapter connects without requesting sessionScope: 'thread', so it inherits the daemon default (single) and POST /session can attach to an existing same-workspace session instead of creating an isolated webview session. This can make separate IDE webviews or another local daemon client share prompts, permission requests, and session state. Please pass a thread-scoped session request through this path (with the appropriate capability handling for older daemons), or explicitly block the daemon IDE path when isolated sessions are unavailable.
— gpt-5.5 via Qwen Code /review
| if (!sessionId) { | ||
| throw new Error('Daemon IDE session was not created'); | ||
| } | ||
| return { sessionId } as NewSessionResponse; |
There was a problem hiding this comment.
[Critical] newSession() returns the already-connected daemon session instead of creating a fresh one. QwenAgentManager.createNewSession(..., { forceNew: true }) relies on the connection's newSession() to honor explicit new-session requests, so in daemon IDE mode the UI can clear/open a new conversation while the backend still sends the next prompt into the old daemon session context. Please make this method create/attach a fresh daemon session for explicit session/new calls instead of reusing currentSessionId.
— gpt-5.5 via Qwen Code /review
| override async setMode( | ||
| _modeId: ApprovalModeValue, | ||
| ): Promise<SetSessionModeResponse> { | ||
| return {} as SetSessionModeResponse; |
There was a problem hiding this comment.
[Critical] setMode() is a silent no-op that resolves successfully. The manager treats a resolved setMode() as confirmed and emits modeChanged, so the webview can show that approval mode changed while the daemon session is still running with its previous policy. If the daemon session is in a permissive mode, switching back to a safer mode would appear to work but not actually change enforcement. Please either wire this to a real daemon mode-change endpoint or throw an explicit unsupported error so the UI does not report success.
— gpt-5.5 via Qwen Code /review
| return (await this.daemon.setModel(modelId)) as SetSessionModelResponse; | ||
| } | ||
|
|
||
| override async setMode( |
There was a problem hiding this comment.
[Critical] setMode() silently discards the approval mode change — the _modeId parameter is never forwarded to the daemon. It returns {} as SetSessionModeResponse, which is an unsafe empty-object cast.
The UI will show the mode as changed, but the daemon never receives the update. This creates a silent UI-vs-execution mismatch that undermines the approval-mode security boundary.
| override async setMode( | |
| override async setMode( | |
| _modeId: ApprovalModeValue, | |
| ): Promise<SetSessionModeResponse> { | |
| throw new Error('Daemon IDE setMode is not wired in this draft'); | |
| } |
At minimum, throw a clear error (like rewindSession does) rather than silently succeeding. If the daemon protocol supports mode changes, forward via an extension method.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| }); | ||
| } | ||
|
|
||
| override async authenticate( |
There was a problem hiding this comment.
[Critical] authenticate() returns {} as AuthenticateResponse — an empty object cast to a type that requires fields like authMethods. If downstream code destructures or checks response fields (e.g., in newSessionWithRetry), it will encounter undefined values.
Since authenticate() always succeeds while the daemon may reject the token at the HTTP layer, this also removes a defense-in-depth check: the caller gets a false sense that authentication passed.
| override async authenticate( | |
| override async authenticate( | |
| _methodId?: string, | |
| ): Promise<AuthenticateResponse> { | |
| return { authType: 'none', authMethods: [] } as unknown as AuthenticateResponse; | |
| } |
Return at least a minimal valid shape, or throw a meaningful error if auth is truly unsupported in this draft.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| * session-management APIs remain explicit no-ops/failures until the daemon | ||
| * protocol grows matching endpoints. | ||
| */ | ||
| export class DaemonAcpConnection extends AcpConnection { |
There was a problem hiding this comment.
[Critical] DaemonAcpConnection inherits get hasActiveSession() from AcpConnection, which checks this.sessionId !== null — a private field the daemon adapter never sets. As a result, hasActiveSession always returns false regardless of daemon session state.
No production caller currently uses hasActiveSession, but it's a latent correctness trap for future code that trusts this getter.
| export class DaemonAcpConnection extends AcpConnection { | |
| override get hasActiveSession(): boolean { | |
| return this.daemon.isConnected && this.daemon.currentSessionId !== null; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return { sessionId } as NewSessionResponse; | ||
| } | ||
|
|
||
| override async sendPrompt( |
There was a problem hiding this comment.
[Critical] Pattern: multiple bridge methods use unchecked as type casts with zero runtime validation. sendPrompt() casts daemon result → PromptResponse, setModel() casts → SetSessionModelResponse, setMode() casts → SetSessionModeResponse, authenticate() casts → AuthenticateResponse. If the daemon response shape diverges from the ACP SDK types, failures manifest as cryptic downstream crashes (cannot read property X of undefined) far from the actual mismatch.
| override async sendPrompt( | |
| // Example: add at least a lightweight validation guard | |
| override async sendPrompt( | |
| prompt: string | ContentBlock[], | |
| ): Promise<PromptResponse> { | |
| const result = await this.daemon.sendPrompt(prompt); | |
| if (!result || typeof result !== 'object') { | |
| throw new Error('Daemon returned invalid PromptResponse'); | |
| } | |
| return result as unknown as PromptResponse; | |
| } |
Apply similar guards to setModel, or document this as a known draft limitation with a tracking issue reference.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| super(); | ||
| } | ||
|
|
||
| override async connect( |
There was a problem hiding this comment.
[Suggestion] connect() ignores the _extraArgs parameter, which in the standard ACP path carries --proxy <url> from VS Code's HTTP configuration. In daemon mode, users behind a mandatory corporate proxy will experience silent connection failures with no indication that proxy settings were dropped.
Since the default daemon URL is http://127.0.0.1:4170 (localhost), this is benign today — but will become a prod incident if the daemon URL is ever configured to a non-localhost address.
Consider at minimum logging a warning: console.warn('[DaemonAcpConnection] Proxy settings from VS Code are not applied in daemon mode.'); when _extraArgs is non-empty.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return this.daemon.currentSessionId; | ||
| } | ||
|
|
||
| private wireDaemonCallbacks(): void { |
There was a problem hiding this comment.
[Suggestion] onInitialized callback never fires in the daemon path. The standard AcpConnection.setupChildProcessHandlers() fires onInitialized with the ACP initialize response (containing availableModes, currentModeId, model info). In the daemon path, connect() completes successfully but onInitialized is never invoked, so mode/model metadata is not discovered from initialization.
QwenAgentManager.connect() relies on this to populate initial capabilities. Ensure the daemon session_update events carry equivalent data, or fire onInitialized after daemon connect with whatever init metadata the daemon provides.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } from '../../services/settingsWriter.js'; | ||
| import { parseInsightMessage } from '@qwen-code/qwen-code-core'; | ||
|
|
||
| function createAgentManagerFromConfiguration(): QwenAgentManager { |
There was a problem hiding this comment.
[Suggestion] The createAgentManagerFromConfiguration() function — the configuration gate for the entire experimental daemon IDE feature — has no test coverage. WebViewProvider.test.ts mocks the entire QwenAgentManager module, so neither the experimentalDaemonIde flag branch nor the DaemonAcpConnection construction path is exercised.
Consider adding tests that un-mock QwenAgentManager and verify the two branches (flag enabled/disabled) produce the correct connection type with the right config values. The existing mockConfigGet infrastructure can drive the VS Code configuration values.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
b8a2cf3 to
e065be7
Compare
| throw new Error('Daemon IDE history restore is not wired in this draft'); | ||
| } | ||
|
|
||
| override disconnect(): void { |
There was a problem hiding this comment.
[Critical] disconnect() drops the async daemon cleanup by calling void this.daemon.disconnect(), but DaemonIdeConnection.disconnect() does not clear session or fire onDisconnected until after the SSE event pump finishes. Existing reconnect/dispose paths call agentManager.disconnect() and immediately continue, so daemon mode can still report isConnected === true and keep old callbacks alive while the provider starts a new connection. That can leave overlapping event streams or late onDisconnected/session updates from the previous daemon session racing into the new webview state.
Please make the daemon adapter teardown deterministic for callers: either clear/abort the active daemon session synchronously before returning, or expose and await an async disconnect path through the manager/provider before reconnecting.
— gpt-5.5 via Qwen Code /review
| "command": "qwen-code.showLogs", | ||
| "title": "Qwen Code: Show Logs" | ||
| }, | ||
| { |
There was a problem hiding this comment.
[Suggestion] The qwen-code.daemonSmoke command is contributed and activated unconditionally, so it is visible and runnable even when qwen-code.experimentalDaemonIde is disabled. That exposes the draft daemon flow in the normal command surface despite the feature being described as opt-in, and a user can accidentally send a prompt/workspace context to the configured daemon without first enabling the experimental IDE path.
Please gate the command behind the same experimental setting before it connects (and ideally hide it from the command palette unless the flag is enabled), showing a clear message when the draft daemon path is disabled.
— gpt-5.5 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
Summary
This review surfaces 8 new findings not covered by the existing 11 inline comments. The PR adds an experimental daemon-backed webview path for the VS Code IDE companion — the adapter is a reasonable draft but several gaps need attention before merging.
Findings that affect lines outside the diff (no inline anchor):
-
[Critical] New files
daemonAcpConnection.ts(195 lines) anddaemonSmoke.ts(138 lines) have zero test coverage — no.test.tsfiles exist. Every override method and command flow is untested. -
[Critical]
isLoopbackHostname()indaemonIdeConnection.ts(pre-existing, not in diff) accepts the full127.0.0.0/8range, while the server-sideLOOPBACK_BINDSonly recognizes127.0.0.1exactly. Combined with workspace-injectabledaemonUrl, this allows token exfiltration to any local loopback port. Align the client-side set withLOOPBACK_BINDSor validate port = 4170 when using a bearer token.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| override async getAccountInfo(): Promise<{ | ||
| authType: string | null; |
There was a problem hiding this comment.
[Critical] getAccountInfo() hardcodes model: null, so the webview model selector shows empty/nothing in daemon mode. The daemon session has an active model (set via modelServiceId during createOrAttach), but this info never reaches the UI.
| authType: string | null; | |
| override async getAccountInfo(): Promise<{ | |
| authType: string | null; | |
| model: string | null; | |
| baseUrl: string | null; | |
| apiKeyEnvKey: string | null; | |
| }> { | |
| const sessionModel = this.daemon.currentSessionModel ?? null; | |
| return { | |
| authType: 'daemon', | |
| model: sessionModel, | |
| baseUrl: this.cachedBaseUrl || 'http://127.0.0.1:4170', | |
| apiKeyEnvKey: null, | |
| }; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| override get currentSessionId(): string | null { | ||
| return this.daemon.currentSessionId; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Critical] wireDaemonCallbacks() bridges only 5 of the 8 AcpConnection callbacks. Missing: onAuthenticateUpdate (users never see auth re-prompt dialogs) and onSlashCommandNotification (/insight progress is silently lost). DaemonIdeConnection also lacks these callback slots entirely, so bridging alone isn't enough — both the DaemonIdeConnection class and DaemonAcpConnection.wireDaemonCallbacks() need corresponding additions.
| private wireDaemonCallbacks(): void { | |
| this.daemon.onSessionUpdate = (data) => this.onSessionUpdate(data); | |
| this.daemon.onPermissionRequest = (data) => this.onPermissionRequest(data); | |
| this.daemon.onAskUserQuestion = (data) => this.onAskUserQuestion(data); | |
| this.daemon.onEndTurn = (reason) => this.onEndTurn(reason); | |
| this.daemon.onDisconnected = (code, signal) => | |
| this.onDisconnected(code, signal); | |
| if (this.daemon.onAuthenticateUpdate) { | |
| this.daemon.onAuthenticateUpdate = (data) => | |
| this.onAuthenticateUpdate(data); | |
| } | |
| if (this.daemon.onSlashCommandNotification) { | |
| this.daemon.onSlashCommandNotification = (data) => | |
| this.onSlashCommandNotification(data); | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| return getTextContent(update['content']); | ||
| } | ||
|
|
There was a problem hiding this comment.
[Critical] getSessionUpdateText() only handles agent_message_chunk and agent_thought_chunk. All other session events (errors, state transitions, session_died) are silently discarded. The user sees "Qwen daemon smoke prompt completed." but the output channel may be empty — impossible to tell if the daemon errored or returned empty content.
| const text = getSessionUpdateText(data); | |
| if (text) { | |
| outputChannel?.append(text); | |
| } else if (data.update?.sessionUpdate) { | |
| outputChannel?.appendLine( | |
| `[daemon] event: ${data.update.sessionUpdate}`, | |
| ); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| outputChannel?.appendLine(`[daemon] connecting to ${baseUrl}`); | ||
| connection.onSessionUpdate = (data) => { | ||
| const text = getSessionUpdateText(data); | ||
| if (text) { |
There was a problem hiding this comment.
[Critical] The smoke command wires onSessionUpdate, onPermissionRequest, onEndTurn, and onDisconnected, but does not wire onAskUserQuestion. When the daemon sends an ask_user_question permission request, DaemonIdeConnection falls back to the default handler which returns { optionId: 'cancel' } — the multi-choice prompt is silently cancelled without the user ever seeing it.
| if (text) { | |
| connection.onAskUserQuestion = async (request) => { | |
| const picked = await vscode.window.showQuickPick( | |
| (request.options ?? []).map((o) => ({ | |
| label: o.label ?? o.optionId, | |
| optionId: o.optionId, | |
| })), | |
| { title: request.question ?? 'Qwen daemon question' }, | |
| ); | |
| return { optionId: picked?.optionId ?? 'cancel' }; | |
| }; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| private extensionUri: vscode.Uri, | ||
| ) { | ||
| this.agentManager = new QwenAgentManager(); | ||
| this.agentManager = createAgentManagerFromConfiguration(); |
There was a problem hiding this comment.
[Critical] createAgentManagerFromConfiguration() is called once in the constructor and onDidChangeConfiguration does NOT watch experimentalDaemonIde, daemonUrl, or daemonToken. Toggling the feature flag at runtime has no effect until VS Code is reloaded — a frustrating developer experience during draft evaluation.
| this.agentManager = createAgentManagerFromConfiguration(); | |
| // Add daemon settings to the watched list | |
| const DAEMON_RELATED_SETTINGS = [ | |
| 'experimentalDaemonIde', | |
| 'daemonUrl', | |
| 'daemonToken', | |
| ]; | |
| context.subscriptions.push( | |
| vscode.workspace.onDidChangeConfiguration((e) => { | |
| if ( | |
| DAEMON_RELATED_SETTINGS.some((s) => e.affectsConfiguration(`qwen-code.${s}`)) | |
| ) { | |
| // Re-create agent manager with updated daemon config | |
| this.agentManager = createAgentManagerFromConfiguration(); | |
| } | |
| }), | |
| ); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
Pushed a follow-up commit to align this draft with the latest daemon architecture boundary:
Validation after the follow-up: Generated by GPT-5 Codex |
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. |
wenshao
left a comment
There was a problem hiding this comment.
| } catch (error) { | ||
| const message = toSafeErrorMessage(error); | ||
| throw new Error( | ||
| `Failed to attach IDE to daemon workspace "${opts.workspaceCwd ?? ''}": ${message}`, |
There was a problem hiding this comment.
[Suggestion] Error message exposes the full workspace path to the user (e.g., Failed to attach IDE to daemon workspace "/Users/alice/secret-project": ...). This propagates to vscode.window.showErrorMessage in daemonSmoke.ts and error logs in WebViewProvider.
Impact: Information leak — workspace paths with sensitive directory names (client names, project codenames) become visible in error dialogs, screenshots, and log exports.
| `Failed to attach IDE to daemon workspace "${opts.workspaceCwd ?? ''}": ${message}`, | |
| } catch (error) { | |
| const message = toSafeErrorMessage(error); | |
| throw new Error( | |
| `Failed to attach IDE to daemon workspace: ${message}`, | |
| ); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| const token = | ||
| config.get<string>('qwen-code.daemonToken') || |
There was a problem hiding this comment.
[Suggestion] The smoke command reads QWEN_SERVER_TOKEN from the process environment as a fallback for the daemon token. This is inconsistent with the WebViewProvider daemon path (createAgentManagerFromConfiguration) which reads only from qwen-code.daemonToken VS Code config.
Impact: (a) Any VS Code extension running in the same process can discover the token via process.env; (b) users who set QWEN_SERVER_TOKEN only for CLI use may unintentionally leak it to the VS Code smoke test.
| config.get<string>('qwen-code.daemonToken') || | |
| const token = config.get<string>('qwen-code.daemonToken'); |
If interactive smoke testing genuinely needs a fallback, prompt the user with vscode.window.showInputBox({ password: true }) before connecting instead of reading the environment.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| export const daemonSmokeCommand = 'qwen-code.daemonSmoke'; | ||
|
|
||
| function isRecord(value: unknown): value is Record<string, unknown> { |
There was a problem hiding this comment.
[Suggestion] The isRecord type guard is duplicated verbatim between daemonSmoke.ts and daemonIdeConnection.ts. If the implementation diverges or a bug is found, both copies must be updated separately.
Impact: Maintenance burden — two independent copies create a risk of behavioral divergence that is hard to debug.
| function isRecord(value: unknown): value is Record<string, unknown> { | |
| // Extract to a shared utility, e.g. packages/vscode-ide-companion/src/utils/typeGuards.ts | |
| export function isRecord(value: unknown): value is Record<string, unknown> { | |
| return typeof value === 'object' && value !== null; | |
| } |
Then import from the shared module in both daemonSmoke.ts and daemonIdeConnection.ts.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| value: 'Say hello from the daemon IDE wire-up.', | ||
| ignoreFocusOut: true, | ||
| }); | ||
| if (!prompt) { |
There was a problem hiding this comment.
[Suggestion] The default daemon URL 'http://127.0.0.1:4170' is hardcoded in 4 separate locations: daemonSmoke.ts:89, daemonAcpConnection.ts:68, daemonAcpConnection.ts:133, and WebViewProvider.ts:64. If the default port or address changes, missing any one of these will produce inconsistent connection behavior with no compile error.
Impact: Brittle defaults — a port change requires hunting down multiple magic strings across the codebase.
| if (!prompt) { | |
| // Define once in a shared constant, e.g. in daemonIdeConnection.ts or a new constants/daemon.ts | |
| export const DEFAULT_DAEMON_BASE_URL = 'http://127.0.0.1:4170'; |
Refactor all 4 locations to reference this constant.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // Daemon mode does not spawn the CLI entrypoint; the running qwen serve | ||
| // instance owns runtime startup, auth, tools, MCP, skills, and files. | ||
| this.daemonWorkingDir = workingDir; | ||
| this.wireDaemonCallbacks(); |
There was a problem hiding this comment.
[Suggestion] DaemonAcpConnection has zero diagnostic logging — no console.log/console.warn/console.error calls across 212 lines. The base class AcpConnection logs every operation ([ACP] Spawning command:, [ACP] Session created with ID:, [ACP] Sending session/set_mode:, etc.), providing a clear audit trail for debugging.
Impact: At 3 AM, when the daemon path misbehaves, an oncall engineer has no breadcrumbs to trace the adapter's execution. The silent adapter layer makes root-causing daemon-mode failures far harder than the child-process path.
| this.wireDaemonCallbacks(); | |
| // Add console.debug before key operations, matching base class convention: | |
| console.debug('[DaemonACP] Connecting to daemon at', baseUrl); | |
| console.debug('[DaemonACP] New session requested, sessionId:', sessionId); | |
| // etc. |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| override get currentSessionId(): string | null { | ||
| return this.daemon.currentSessionId; |
There was a problem hiding this comment.
[Suggestion] disconnect() uses void this.daemon.disconnect() — a fire-and-forget that silently swallows any async errors from DaemonIdeConnection.disconnect(). If the SSE event pump or resource cleanup fails, callers (WebViewProvider, QwenAgentManager) will never know.
Impact: Failed disconnects leave orphaned resources with no error signal and no diagnostic log. Residual state can accumulate across reconnections.
| return this.daemon.currentSessionId; | |
| override async disconnect(): Promise<void> { | |
| try { | |
| await this.daemon.disconnect(); | |
| } catch (error) { | |
| console.warn('[DaemonAcpConnection] Disconnect failed:', error); | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| workspaceCwd: opts.workspaceCwd, | ||
| modelServiceId: opts.modelServiceId, | ||
| }); | ||
| let session: DaemonIdeSessionClient; |
There was a problem hiding this comment.
[Suggestion] In createSdkDaemonSessionFactory, a new DaemonClient(...) is created before calling SdkDaemonSessionClient.createOrAttach(...). If createOrAttach throws, the DaemonClient instance is never cleaned up — there is no .close() or .disconnect() call in the catch block.
Impact: If DaemonClient holds HTTP connections, AbortControllers, or event emitters, each failed createOrAttach leaks a client instance. The actual severity depends on the SDK internals which are not visible in this diff.
| let session: DaemonIdeSessionClient; | |
| try { | |
| session = await SdkDaemonSessionClient.createOrAttach(daemon, {...}); | |
| } catch (error) { | |
| daemon.close?.(); // or whatever cleanup method the SDK exposes | |
| const message = toSafeErrorMessage(error); | |
| throw new Error(`Failed to attach IDE to daemon workspace: ${message}`); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| ).rejects.toThrow( | ||
| 'Daemon baseUrl must target a loopback address, got "example.com"', | ||
| ); | ||
| ).rejects.toThrow('Daemon baseUrl must target a loopback address'); |
There was a problem hiding this comment.
[Suggestion] The test assertion was weakened from an exact match ('Daemon baseUrl must target a loopback address, got "example.com"') to a substring match ('Daemon baseUrl must target a loopback address'). The updated error message is a superset of the old one, so the exact match would still pass. The change unnecessarily reduces test protection.
Impact: If someone accidentally removes the hostname from the error message in the future, this weakened assertion will not catch it.
| ).rejects.toThrow('Daemon baseUrl must target a loopback address'); | |
| ).rejects.toThrow( | |
| 'Daemon baseUrl must target a loopback address for this local IDE draft, got "example.com"', | |
| ); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| override disconnect(): void { | ||
| void this.daemon.disconnect(); |
There was a problem hiding this comment.
[Critical] disconnect() is fire-and-forget on an async operation. DaemonIdeConnection.disconnect() is async — it aborts the event controller, awaits the event pump drain, and clears session state. Using void discards the Promise, so:
- Any rejection from daemon disconnect becomes an unhandled Promise rejection
QwenAgentManager.reconnect()callsdisconnect()then immediatelyconnect(), creating a race where the new connect begins before the old session is fully torn down
| void this.daemon.disconnect(); | |
| override async disconnect(): Promise<void> { | |
| try { | |
| await this.daemon.disconnect(); | |
| } catch (err) { | |
| console.warn('[DaemonAcpConnection] disconnect error:', err); | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| override async setModel(modelId: string): Promise<SetSessionModelResponse> { | ||
| return (await this.daemon.setModel(modelId)) as SetSessionModelResponse; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] setMode() is a silent no-op that returns {}. The daemon is never informed of the mode change, but the UI (via onModeChanged) believes it succeeded. This creates a silent state divergence where the user thinks they're in one mode but the daemon operates in another.
The comment documents this as a draft gap but there is no runtime log. Consider at minimum logging via outputChannel:
| override async setMode(_modeId: ApprovalModeValue): Promise<SetSessionModeResponse> { | |
| console.warn('[DaemonAcpConnection] setMode is not wired in this draft — mode change ignored'); | |
| return {} as SetSessionModeResponse; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| override async newSession( | ||
| cwd: string = process.cwd(), | ||
| ): Promise<NewSessionResponse> { |
There was a problem hiding this comment.
[Suggestion] newSession() returns only { sessionId } — the response is missing models and modes fields that NewSessionResponse carries in the ACP contract. In the daemon path, extractModelInfoFromNewSessionResult, extractSessionModelState, and extractSessionModeState all resolve to null, so onModelInfo, onAvailableModels, and onModeChanged never fire. The model selector and mode indicator in the webview remain blank until a follow-up mechanism populates them.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| * the extension still owns the editor UI, while qwen serve owns runtime, | ||
| * tools, MCP, skills, and files for the same workspace. Setting changes apply | ||
| * to newly created managers; existing sessions are not hot-swapped. | ||
| */ |
There was a problem hiding this comment.
[Suggestion] createAgentManagerFromConfiguration() reads experimentalDaemonIde and switches to the daemon path with no user confirmation. A malicious repository can include a .vscode/settings.json that sets "qwen-code.experimentalDaemonIde": true, silently routing the IDE webview through a loopback daemon on open. While the loopback check prevents SSRF to external servers, this is still a confused-deputy scenario — the extension silently changes its execution model based on untrusted workspace config.
Consider showing a confirmation dialog on the first activation, or scoping experimentalDaemonIde to application (user-level) settings only:
| */ | |
| const useDaemon = config.inspect<boolean>('experimentalDaemonIde')?.globalValue === true; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| "type": "string", | ||
| "default": "", | ||
| "description": "Experimental qwen serve URL used by daemon-backed IDE drafts and the Daemon Smoke Test command." | ||
| }, |
There was a problem hiding this comment.
[Suggestion] qwen-code.daemonToken is stored as a plain "type": "string" configuration value. This means the bearer token is written in cleartext to settings.json, synced across machines via Settings Sync, and could be committed into dotfiles repos. Consider using VS Code's SecretStorage API (context.secrets) instead, or at minimum mark it "secret": true in the configuration contribution.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| @@ -910,8 +910,6 @@ describe('DaemonIdeConnection', () => { | |||
| baseUrl: 'http://example.com:4170', | |||
| sessionFactory: vi.fn(), | |||
| }), | |||
There was a problem hiding this comment.
[Suggestion] The test assertion was weakened from the exact error message to a substring match:
Before: 'Daemon baseUrl must target a loopback address, got "example.com"'
After: 'Daemon baseUrl must target a loopback address'
The test no longer verifies that the offending hostname appears in the error message. A refactor that drops the got "..." suffix would still pass. Restore the full message match, or use toMatch(/Daemon baseUrl.*got "example\.com"/) to validate the hostname is included.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
Architecture update after the 2026-05-19 decision: IDE should continue to use the existing Given the current CHANGES_REQUESTED review surface and the revised roadmap, I recommend closing/defering this draft rather than fixing it to merge now. Keep the code as reference for a later IDE migration once daemon control-plane parity and web contract are stable. Generated by GPT-5 Codex |
wenshao
left a comment
There was a problem hiding this comment.
[Critical] DaemonIdeConnection.handleEvent() currently treats daemon control/status frames other than session_died as unknown and acknowledges them via the default branch. That includes terminal/status events the daemon already emits, such as client_evicted, stream_error, session_closed, model_switched, and model_switch_failed. Because the adapter advances lastSeenEventId after the default branch, the IDE can miss the terminal cause or model-switch failure and keep showing an apparently healthy connected session. Please handle these daemon-native terminal/status events explicitly before acknowledging them; at minimum, clear/disconnect on client_evicted, stream_error, and session_closed, and surface model_switch_failed instead of silently dropping it.
— gpt-5.5 via Qwen Code /review
| const connection = new DaemonIdeConnection(); | ||
|
|
||
| outputChannel?.show(true); | ||
| outputChannel?.appendLine(`[daemon] connecting to ${baseUrl}`); |
There was a problem hiding this comment.
[Critical] This logs the user-entered daemon URL before any validation or redaction. If the user pastes something like http://user:pass@127.0.0.1:4170/?token=..., validateDaemonBaseUrl() will reject the embedded credentials later, but the secret has already been persisted in the VS Code output channel. Please validate/parse the URL first and log only a sanitized origin with username, password, query, and hash removed.
— gpt-5.5 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
Second-opinion review with qwen-latest-series-invite-beta-v28: no new issues found beyond the 33 existing inline comments. Build passed (391 tests, 0 failures), ESLint clean. LGTM! ✅ — qwen-latest-series-invite-beta-v28 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
No new issues found beyond the existing inline comments. LGTM! ✅ — gpt-5.5 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
No new review findings. The high-confidence issues found in this pass are already covered by existing inline comments, so I am not posting duplicates. — gpt-5.5 via Qwen Code /review
|
Closing this draft per the latest daemon roadmap decision: IDE should continue using the existing ACP subprocess path for now. Daemon-backed IDE integration is deferred until the remote web chat / web terminal contract, control-plane parity, identity, and auth/status UX are stable. Generated by GPT-5 Codex |
Summary
qwen-code.experimentalDaemonIdeis enabled.qwen serveowns runtime execution for the same workspace throughDaemonSessionClient; IDE code does not import daemon EventBus internals or own a parallel runtime/event protocol.Validation
qwen-code.experimentalDaemonIdeis enabled.editorGroupUtils.tscurly-brace warning is still present and unrelated to this branch.npm run build --workspace=packages/vscode-ide-companion.qwen serveon a local loopback port for the same workspace.qwen-code.experimentalDaemonIdeand pointqwen-code.daemonUrlat the local daemon.Scope / Risk
qwen-code.experimentalDaemonIdeis set totrue.Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Related to #3803 and #4175.