feat(ide): add daemon smoke wire-up - #4263
Closed
chiga0 wants to merge 1 commit into
Closed
Conversation
Contributor
📋 Review SummaryThis PR adds a 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
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
requested changes
May 18, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] No test coverage for daemonSmoke.ts — the entire 138-line module has zero tests. Add a test file with mocked VS Code APIs covering: URL/prompt cancellation, happy path, connect failure, sendPrompt failure, and disconnect in finally.
— mimo-v2.5-pro via Qwen Code /review
| `[daemon] session ${connection.currentSessionId ?? 'unknown'}`, | ||
| ); | ||
| await connection.sendPrompt(prompt); | ||
| vscode.window.showInformationMessage( |
Collaborator
There was a problem hiding this comment.
[Critical] sendPrompt resolves when the HTTP response arrives, but the SSE event pump may still be delivering agent_message_chunk events. The "completed" notification fires before all events are drained, and the finally block's disconnect() aborts the event controller — silently dropping in-flight output.
Consider awaiting the onEndTurn callback before showing the success message:
Suggested change
| vscode.window.showInformationMessage( | |
| connection.onEndTurn = (reason) => { | |
| outputChannel?.appendLine(''); | |
| outputChannel?.appendLine(`[daemon] turn ended: ${reason ?? 'ok'}`); | |
| turnEndedResolve?.(); | |
| }; |
Then add a Promise that resolves when onEndTurn fires, and await it between sendPrompt and the success notification. Alternatively, refactor DaemonIdeConnection to expose an awaitTurnComplete() API.
— mimo-v2.5-pro via Qwen Code /review
| }, | ||
| "qwen-code.daemonUrl": { | ||
| "order": 5, | ||
| "type": "string", |
Collaborator
There was a problem hiding this comment.
[Suggestion] The qwen-code.daemonToken setting stores a bearer token as a plain "type": "string" in VS Code settings.json. It's persisted unencrypted, synced via Settings Sync in cleartext, and readable by any installed extension via vscode.workspace.getConfiguration().
Use VS Code's SecretStorage API instead: prompt via showInputBox({ password: true }), store with context.secrets.store(), retrieve with context.secrets.get(). At minimum, add "scope": "application" to prevent workspace-level storage.
— mimo-v2.5-pro via Qwen Code /review
| connection.onPermissionRequest = pickPermissionOption; | ||
| connection.onEndTurn = (reason) => { | ||
| outputChannel?.appendLine(''); | ||
| outputChannel?.appendLine(`[daemon] turn ended: ${reason ?? 'ok'}`); |
Collaborator
There was a problem hiding this comment.
[Suggestion] Neither connection.connect() nor connection.sendPrompt() has a timeout or abort signal. If the daemon hangs (accepts TCP but stalls on HTTP), the command blocks indefinitely with no user-cancel mechanism.
Suggested change
| outputChannel?.appendLine(`[daemon] turn ended: ${reason ?? 'ok'}`); | |
| const ac = new AbortController(); | |
| const timeout = setTimeout(() => ac.abort(), 30_000); | |
| try { | |
| await connection.connect({ | |
| baseUrl, | |
| token, | |
| workspaceCwd, | |
| }); |
Or wrap the entire try block in Promise.race with AbortSignal.timeout(30_000).
— mimo-v2.5-pro via Qwen Code /review
| vscode.window.showInformationMessage( | ||
| 'Qwen daemon smoke prompt completed.', | ||
| ); | ||
| } catch (error) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] Use the codebase-wide getErrorMessage() utility (56 other call sites) instead of the inline pattern:
Suggested change
| } catch (error) { | |
| import { getErrorMessage } from '../utils/errorMessage.js'; |
Then: const message = getErrorMessage(error);
— mimo-v2.5-pro via Qwen Code /review
wenshao
reviewed
May 18, 2026
| outputChannel?.appendLine(`[daemon] turn ended: ${reason ?? 'ok'}`); | ||
| }; | ||
| connection.onDisconnected = (_code, signal) => { | ||
| outputChannel?.appendLine(`[daemon] disconnected: ${signal ?? 'ok'}`); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The onAskUserQuestion callback is never set on the DaemonIdeConnection. The default implementation returns { optionId: 'cancel' }, so any ask_user_question permission request from the daemon is silently cancelled with zero visibility to the smoke tester. For a diagnostic tool named "Smoke Test", this is a blind spot — if a user is troubleshooting why a daemon session seems unresponsive, and the root cause is an unexpected ask_user_question, the smoke test will not surface it.
Suggested change
| outputChannel?.appendLine(`[daemon] disconnected: ${signal ?? 'ok'}`); | |
| connection.onAskUserQuestion = async () => { | |
| outputChannel?.appendLine('[daemon] ask_user_question received (auto-cancelled in smoke test)'); | |
| return { optionId: 'cancel' }; | |
| }; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
reviewed
May 18, 2026
| const connection = new DaemonIdeConnection(); | ||
|
|
||
| outputChannel?.show(true); | ||
| outputChannel?.appendLine(`[daemon] connecting to ${baseUrl}`); |
Collaborator
There was a problem hiding this comment.
[Suggestion] This logs the raw daemon URL before DaemonIdeConnection.connect() validates it. If someone accidentally pastes a URL with embedded credentials (for example http://token@127.0.0.1:4170), validation will reject it later, but the secret has already been written to the VS Code output channel. Please validate or redact the URL before logging, or log only a normalized URL after connect() succeeds.
— gpt-5.5 via Qwen Code /review
wenshao
requested changes
May 18, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Also noted: index.ts:44 — The JSDoc for outputChannel only mentions the showLogs command, but registerDaemonSmokeCommand now also depends on it. If outputChannel is undefined, the daemon smoke test silently produces no output. Consider updating: @param outputChannel - Optional output channel for the showLogs and daemon smoke commands.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const connection = new DaemonIdeConnection(); | ||
|
|
||
| outputChannel?.show(true); | ||
| outputChannel?.appendLine(`[daemon] connecting to ${baseUrl}`); |
Collaborator
There was a problem hiding this comment.
[Critical] outputChannel?.appendLine(\[daemon] connecting to ${baseUrl}`)logs the raw user-provided URL beforeconnection.connect()invokesvalidateDaemonBaseUrl. While validateDaemonBaseUrl rejects userinfo credentials (user:pass@), it does NOT inspect or strip query parameters. A URL like http://127.0.0.1:4170?token=secret` writes the credential in plaintext to the persistent VS Code OutputChannel log file (CWE-532).
Suggested change
| outputChannel?.appendLine(`[daemon] connecting to ${baseUrl}`); | |
| // Sanitize URL before logging: strip query params and hash | |
| const safeUrl = new URL(baseUrl).origin + new URL(baseUrl).pathname; | |
| outputChannel?.appendLine(`[daemon] connecting to ${safeUrl}`); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| ); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| log(`[DaemonSmoke] ${message}`); |
Collaborator
There was a problem hiding this comment.
[Critical] log() in the catch block delegates to createLogger (utils/logger.ts:9-14), which checks context.extensionMode === vscode.ExtensionMode.Development and is a silent no-op otherwise. In production, error details are written only to a transient showErrorMessage notification — not to the output channel or console. The output channel shows only [daemon] connecting to ... with no follow-up error or disconnect line, making production failures nearly impossible to diagnose.
Suggested change
| log(`[DaemonSmoke] ${message}`); | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error); | |
| log(`[DaemonSmoke] ${message}`); | |
| outputChannel?.appendLine(`[daemon] error: ${message}`); | |
| vscode.window.showErrorMessage(`Qwen daemon smoke failed. Check the Qwen Code Companion output channel for details.`); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| export const daemonSmokeCommand = 'qwen-code.daemonSmoke'; | ||
|
|
||
| function isRecord(value: unknown): value is Record<string, unknown> { |
Collaborator
There was a problem hiding this comment.
[Suggestion] isRecord is duplicated identically in this file and services/daemonIdeConnection.ts:82. Neither is exported. Any change to this type guard must be synchronized manually across both locations.
Suggested change
| function isRecord(value: unknown): value is Record<string, unknown> { | |
| import { isRecord } from '../services/daemonIdeConnection.js'; |
If isRecord is not exported from daemonIdeConnection.ts, consider extracting it to src/utils/typeGuards.ts for shared use.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| async function pickPermissionOption( | ||
| request: RequestPermissionRequest, | ||
| ): Promise<{ optionId?: string }> { | ||
| const options = Array.isArray(request.options) ? request.options : []; |
Collaborator
There was a problem hiding this comment.
[Suggestion] getSessionUpdateText silently returns undefined for all non-chunk sessionUpdate types (tool_call, tool_result, user_message_chunk, future ACP extensions). When the daemon protocol evolves or a bug causes unexpected update types, the output channel goes silent with no indication of why — indistinguishable from "the daemon produced no output."
Consider adding a debug fallback for unmatched types:
Suggested change
| const options = Array.isArray(request.options) ? request.options : []; | |
| // After the existing return undefined for non-chunk types, add: | |
| // Uncomment for debugging protocol mismatches: | |
| // outputChannel?.appendLine(`[daemon] unhandled sessionUpdate: ${sessionUpdate}`); | |
| return undefined; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| process.env['QWEN_SERVER_TOKEN']; | ||
| const workspaceCwd = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath; | ||
| const connection = new DaemonIdeConnection(); | ||
|
|
Collaborator
There was a problem hiding this comment.
[Suggestion] Token resolution uses || which makes an explicit empty string (config cleared) indistinguishable from "not set" — the expression always falls back to process.env['QWEN_SERVER_TOKEN']. A user who clears the VS Code daemonToken setting intending to use NO token cannot do so when the env var is present.
Suggested change
| const token = | |
| config.get<string>('qwen-code.daemonToken') | |
| ?? process.env['QWEN_SERVER_TOKEN']; |
Use ?? to treat empty string as an intentional "no token" choice.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| title: `Qwen daemon permission: ${request.toolCall?.kind ?? 'tool'}`, | ||
| placeHolder: 'Choose a daemon permission response', | ||
| }, | ||
| ); |
Collaborator
There was a problem hiding this comment.
[Suggestion] pickPermissionOption calls vscode.window.showQuickPick with no timeout and no output-channel trace. If the daemon has a server-side permission-response timeout, the quick pick may hang after the daemon has already moved on. When the user eventually selects an option, respondToPermission fails silently (console.warn in DaemonIdeConnection), with no user-facing indication.
Consider adding output-channel breadcrumbs so permission interactions are visible:
Suggested change
| ); | |
| outputChannel?.appendLine(`[daemon] permission requested: ${request.toolCall?.kind ?? 'tool'}`); | |
| const picked = await vscode.window.showQuickPick( | |
| // ... existing options ... | |
| ); | |
| outputChannel?.appendLine(`[daemon] permission response: ${picked?.optionId ?? 'cancel'}`); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| vscode.window.showErrorMessage(`Qwen daemon smoke failed: ${message}`); | ||
| } finally { | ||
| await connection.disconnect(); | ||
| } |
Collaborator
There was a problem hiding this comment.
[Suggestion] Raw error.message is passed directly to vscode.window.showErrorMessage. If the daemon or SDK includes response bodies, headers, or internal state in thrown error messages, that data appears in the VS Code notification bar. DaemonIdeConnection has an internal toSafeErrorMessage() helper for this purpose.
Suggested change
| } | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error); | |
| log(`[DaemonSmoke] ${message}`); | |
| outputChannel?.appendLine(`[daemon] error: ${message}`); | |
| vscode.window.showErrorMessage( | |
| 'Qwen daemon smoke failed. Check the Qwen Code Companion output channel for details.', | |
| ); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
|
Superseded by #4267, which now includes the needed daemon IDE smoke wire-up pieces and targets main directly.\n\nGenerated by GPT-5 Codex |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Qwen Code: Daemon Smoke TestVS Code command that connects to a runningqwen servedaemon throughDaemonIdeConnection, streams daemon session text into the Qwen Code Companion output channel, and handles daemon permission requests with a quick-pick.Validation
npm run lintcompleted with one pre-existing warning insrc/utils/editorGroupUtils.ts, outside this PR's files.Scope / Risk
Testing Matrix
Testing matrix notes:
Linked Issues / Bugs