feat(ide): add daemon connection spike - #4199
Conversation
📋 Review SummaryThis PR introduces a well-structured 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Additional findings (not mappable to specific diff lines):
[Critical] Critical error paths untested: pumpEvents non-abort error path, handlePermissionRequest malformed data guard, and ensureSession() error throw have zero test coverage — these are the most likely production failure modes.
[Suggestion] Permission logic duplicated from AcpConnection: resolvePermissionOptionId, isCancelledOption, and parts of resolvePermissionResponse are copied verbatim from AcpConnection. Bug fixes or policy changes in one path won't propagate. Extract shared utilities or add a TODO explaining intentional duplication.
[Suggestion] Test gaps: createSdkDaemonSessionFactory (dynamic import failure), AskUserQuestion routing (~15 lines), resolvePermissionOptionId fallback chain (4 tiers), and handleSessionDied default reason are untested.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Second-opinion review with glm-5.1 (prior review used DeepSeek/deepseek-v4-pro). Found 3 new high-confidence issues not in the prior review:
-
[Critical] AskUserQuestion
answerssilently stripped by ACP schema — theanswersfield is attached toRequestPermissionResponsebut the daemon-side Zod schema only defines_metaandoutcome. Answers are stripped, causing silent data loss. -
[Critical] Test mock returns bare string instead of
{ optionId }object — masks that the callback return value is never actually propagated. The test passes only because the fallback chain coincidentally resolves to the same value. -
[Critical] AskUserQuestion and isCancelledOption branches have zero test coverage — two security-critical code paths are completely untested.
49d5ee4 to
3a76d74
Compare
|
处理了这轮 review：
误报/澄清：
验证：
|
4f49825 to
1fe2b04
Compare
1fe2b04 to
da6a35c
Compare
3a76d74 to
d0217f1
Compare
|
Follow-up update after rebasing this PR to main:
False-positive / clarified points already covered in code/commentary:
Local validation passed:
I resolved the addressed review threads; waiting for CI before considering this ready to undraft. |
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. |
| } | ||
| const optionId = | ||
| this.resolvePermissionOptionId(request, askResponse.optionId) ?? | ||
| askResponse.optionId; |
There was a problem hiding this comment.
[Critical] The AskUserQuestion path still falls back to the raw callback option when it is not one of the daemon-advertised options. The regular permission path now cancels stale option ids, but here { optionId: 'stale-option' } becomes a selected response with an invalid option. If the daemon rejects that response, the user's answer is dropped and the pending request can remain unresolved.
| askResponse.optionId; | |
| const optionId = this.resolvePermissionOptionId( | |
| request, | |
| askResponse.optionId, | |
| ); | |
| if (!optionId) { | |
| return { outcome: { outcome: 'cancelled' } }; | |
| } |
— gpt-5.5 via Qwen Code /review
| }, | ||
| "dependencies": { | ||
| "@agentclientprotocol/sdk": "^0.14.1", | ||
| "@qwen-code/sdk": "*", |
There was a problem hiding this comment.
[Critical] Declaring @qwen-code/sdk here does not make the hidden dynamic import available in the packaged VSIX. The extension script still uses vsce package --no-dependencies, .vscodeignore only includes dist/ plus a few assets, and daemonIdeConnection.ts intentionally hides import('@qwen-code/sdk') from esbuild. That means local builds/tests can pass, but an installed extension will not contain node_modules/@qwen-code/sdk, so enabling the daemon adapter fails at runtime before it can connect. Please either bundle the SDK with a normal import/esbuild path or explicitly include the runtime dependency in the VSIX packaging.
— gpt-5.5 via Qwen Code /review
| } catch (error) { | ||
| if (!signal.aborted) { | ||
| console.warn( | ||
| '[DaemonIdeConnection] Event stream failed:', |
There was a problem hiding this comment.
[Critical] Permission response TOCTOU race: handlePermissionRequest calls await this.resolvePermissionResponse(request) (may block on user interaction for seconds), then calls this.ensureSession().respondToPermission(...). If a session_died event arrives during the await, this.session is set to null by clearCurrentSession, and ensureSession() throws 'Not connected to daemon session'. The user's permission decision (allow/deny) is silently lost — the daemon hangs waiting for a response that never arrives.
| '[DaemonIdeConnection] Event stream failed:', | |
| private async handlePermissionRequest(data: unknown): Promise<void> { | |
| if (!isPermissionRequestData(data)) { | |
| return; | |
| } | |
| const requestId = data['requestId']; | |
| const request = data; | |
| const session = this.session; | |
| const response = await this.resolvePermissionResponse(request); | |
| if (!session) { | |
| return; | |
| } | |
| const accepted = await session.respondToPermission(requestId, response); | |
| if (!accepted) { | |
| console.warn( | |
| '[DaemonIdeConnection] Permission response rejected by daemon for request:', | |
| requestId, | |
| ); | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const response = await session.prompt({ prompt: promptBlocks }); | ||
| this.onEndTurn(response.stopReason); | ||
| return response; | ||
| } |
There was a problem hiding this comment.
[Critical] disconnect() calls this.eventController?.abort() then await this.eventPump, but the AbortController only aborts the SSE stream — it cannot interrupt an in-flight onPermissionRequest or onAskUserQuestion callback. If the event pump is blocked inside a user-facing permission callback, disconnect() hangs forever, requiring a VS Code restart.
| } | |
| if (this.eventPump) { | |
| try { | |
| await Promise.race([ | |
| this.eventPump, | |
| new Promise((_, reject) => | |
| setTimeout(() => reject(new Error('Event pump shutdown timed out')), 5000), | |
| ), | |
| ]); | |
| } catch { | |
| /* pump errors are converted into callbacks */ | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| case 'session_died': | ||
| this.handleSessionDied(event.data); | ||
| break; | ||
| default: |
There was a problem hiding this comment.
[Critical] In the AskUserQuestion branch of resolvePermissionResponse, when resolvePermissionOptionId returns undefined (callback's optionId doesn't match any in request.options), the code falls back to ?? askResponse.optionId — sending an unvalidated optionId to the daemon. The non-AUQ path correctly returns { outcome: 'cancelled' } in this case. Inconsistent behavior between the two paths.
| default: | |
| const optionId = this.resolvePermissionOptionId(request, askResponse.optionId); | |
| if (!optionId) { | |
| return { outcome: { outcome: 'cancelled' } }; | |
| } | |
| return { | |
| outcome: { | |
| outcome: 'selected', | |
| optionId, | |
| }, | |
| answers: askResponse.answers, | |
| } as RequestPermissionResponse; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| if (!signal.aborted) { | ||
| this.clearCurrentSession(session, 'stream_ended'); | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
[Suggestion] handleEvent only handles session_update, permission_request, and session_died. The design doc (ide.md Event Mapping Contract) lists permission_resolved ("Close/update approval UI") and model_switched ("Existing model-state callback") as expected events, but they fall into default: break; and are silently dropped. At minimum, log them so they're discoverable during dogfood testing.
| } catch (error) { | |
| case 'permission_resolved': | |
| // TODO: Close/update approval UI when wired into AgentManager | |
| console.debug('[DaemonIdeConnection] Unhandled event:', event.type); | |
| break; | |
| case 'model_switched': | |
| // TODO: Fire model-state callback when wired into AgentManager | |
| console.debug('[DaemonIdeConnection] Unhandled event:', event.type); | |
| break; | |
| default: | |
| break; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| await session.cancel(); | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] disconnect() sets this.session = null but does not clear this.lastSeenEventId. After disconnect, the lastEventId getter returns a stale event ID from the dead session, which could confuse code that reads it between disconnect and reconnect.
| this.eventController = null; | |
| this.eventPump = null; | |
| this.session = null; | |
| this.lastSeenEventId = undefined; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| onSessionUpdate: (data: SessionNotification) => void = () => {}; | ||
| onPermissionRequest: (data: RequestPermissionRequest) => Promise<{ | ||
| optionId: string; | ||
| }> = () => Promise.resolve({ optionId: 'cancel' }); |
There was a problem hiding this comment.
[Suggestion] The default onPermissionRequest handler silently returns { optionId: 'cancel' }. If a consumer forgets to wire this callback, all tool calls are silently denied with zero diagnostic feedback — making integration bugs extremely hard to diagnose.
| }> = () => Promise.resolve({ optionId: 'cancel' }); | |
| onPermissionRequest: (data: RequestPermissionRequest) => Promise<{ | |
| optionId: string; | |
| }> = () => { | |
| console.warn( | |
| '[DaemonIdeConnection] onPermissionRequest not wired — permission will be cancelled', | |
| ); | |
| return Promise.resolve({ optionId: 'cancel' }); | |
| }; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| }); | ||
| if ( | ||
| !askResponse.optionId || | ||
| this.isCancelledOption(askResponse.optionId) |
There was a problem hiding this comment.
[Suggestion] resolvePermissionOptionId in DaemonIdeConnection is a near-verbatim copy of the same method in AcpConnection (same fallback chain: allow_once kind → proceed_once optionId → namespaced proceed_once → first option). The two implementations will inevitably diverge. Extract to a shared utility (e.g., packages/vscode-ide-companion/src/utils/permissionOptions.ts).
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| private lastSeenEventId: number | undefined; | ||
|
|
||
| onSessionUpdate: (data: SessionNotification) => void = () => {}; | ||
| onPermissionRequest: (data: RequestPermissionRequest) => Promise<{ |
There was a problem hiding this comment.
[Suggestion] AcpConnection exposes onInitialized, onAuthenticateUpdate, and onSlashCommandNotification callbacks that QwenAgentManager wires. DaemonIdeConnection omits all three. When this adapter is wired into QwenAgentManager, assigning to these missing properties will silently create new JS properties instead of surfacing an error. Add at minimum onInitialized as a no-op placeholder to surface the gap.
| onPermissionRequest: (data: RequestPermissionRequest) => Promise<{ | |
| onInitialized: (init: unknown) => void = () => {}; | |
| // Not yet mapped from daemon events — see design doc §Event Mapping Contract. | |
| onAuthenticateUpdate: (data: unknown) => void = () => {}; | |
| onSlashCommandNotification: (data: unknown) => void = () => {}; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| if (url.username || url.password) { | ||
| throw new Error('Daemon baseUrl must not contain credentials'); | ||
| } | ||
| return baseUrl; |
There was a problem hiding this comment.
[Suggestion] validateDaemonBaseUrl parses via new URL(baseUrl) but returns the original un-normalized baseUrl instead of url.href. A URL like http://127.0.0.1:4170/api/../../admin passes validation yet the path traversal remains in the stored value. Since DaemonClient uses template-literal URL construction (${this.baseUrl}/session/...), downstream code that adds allowlisting or prefix checks operates on the non-canonical form.
| return baseUrl; | |
| return url.href; |
— glm-5.1 via Qwen Code /review
| this.lastSeenEventId = options.lastEventId ?? this.session.lastEventId; | ||
|
|
||
| this.eventController = new AbortController(); | ||
| this.eventPump = this.pumpEvents(this.session, this.eventController.signal); |
There was a problem hiding this comment.
[Suggestion] connect() assigns this.session and starts pumpEvents() as fire-and-forget — it resolves before confirming the SSE stream is healthy. If session.events() throws on the first generator call (auth failure, daemon down), the pump catches it asynchronously and calls clearCurrentSession. Between connect() resolving and the pump's async error, isConnected === true and consumers may call sendPrompt(). Consider awaiting the first SSE event or a health-check round-trip before returning, or documenting this transient window.
— glm-5.1 via Qwen Code /review
| this.eventPump = this.pumpEvents(this.session, this.eventController.signal); | ||
| } | ||
|
|
||
| async sendPrompt( |
There was a problem hiding this comment.
[Critical] sendPrompt() has no try/finally around session.prompt(). If the prompt HTTP request rejects (network error, session expired), onEndTurn() is never called. The webview UI enters a permanent "thinking" state with no timeout or recovery. Add a try/finally that calls this.onEndTurn('error') in the catch/rejection path.
| async sendPrompt( | |
| async sendPrompt( | |
| prompt: string | ContentBlock[], | |
| ): Promise<DaemonIdePromptResult> { | |
| const session = this.ensureSession(); | |
| const promptBlocks = normalizePrompt(prompt); | |
| try { | |
| const response = await session.prompt({ prompt: promptBlocks }); | |
| this.onEndTurn(response.stopReason); | |
| return response; | |
| } catch (error) { | |
| this.onEndTurn('error'); | |
| throw error; | |
| } | |
| } |
— glm-5.1 via Qwen Code /review
| ): Promise<DaemonIdePromptResult> { | ||
| const session = this.ensureSession(); | ||
| const promptBlocks = normalizePrompt(prompt); | ||
| const response = await session.prompt({ prompt: promptBlocks }); |
There was a problem hiding this comment.
[Suggestion] session.prompt() accepts an optional AbortSignal but sendPrompt() never passes one. If the session dies or disconnect is called while a prompt is in-flight, the HTTP request hangs until the daemon responds or times out. After disconnect, the orphaned prompt continues consuming daemon resources.
| const response = await session.prompt({ prompt: promptBlocks }); | |
| const response = await session.prompt( | |
| { prompt: promptBlocks }, | |
| this.eventController?.signal, | |
| ); |
— glm-5.1 via Qwen Code /review
| const session = this.ensureSession(); | ||
| const promptBlocks = normalizePrompt(prompt); | ||
| const response = await session.prompt({ prompt: promptBlocks }); | ||
| this.onEndTurn(response.stopReason); |
There was a problem hiding this comment.
[Suggestion] onEndTurn is called from sendPrompt() (HTTP POST response) while streaming events arrive through the independent SSE pump. In AcpConnection, both travel through a single JSON-RPC stream with FIFO ordering — onEndTurn always fires after the last onSessionUpdate. Here, onEndTurn can fire before the pump has delivered buffered agent_message_chunk events, causing the UI to render "turn complete" then continue receiving content. Consider deferring onEndTurn into the pump (detect end-of-turn from a session_update event) or documenting this as a known semantic gap.
— glm-5.1 via Qwen Code /review
| return await this.ensureSession().setModel(modelId); | ||
| } | ||
|
|
||
| async disconnect(): Promise<void> { |
There was a problem hiding this comment.
[Critical] Two lifecycle issues with disconnect():
-
onDisconnectedis never called. The method nullssession/eventController/eventPumpbut never invokesthis.onDisconnected(...). The involuntary disconnect paths (clearCurrentSession) correctly call it, but user-initiated disconnect silently drops the session without notifying the webview. After disconnect, the UI remains in a "connected" state while the session is dead. -
asyncvsAcpConnectionsync mismatch.AcpConnection.disconnect()returnsvoid(synchronous).DaemonIdeConnection.disconnect()returnsPromise<void>. All existing callers inQwenAgentManagerandWebViewProvidercall.disconnect()withoutawait. When this adapter is wired in, every disconnect call returns immediately while the event pump is still running — the connection appears to never actually disconnect.
| async disconnect(): Promise<void> { | |
| disconnect(): void { | |
| const session = this.session; | |
| this.eventController?.abort(); | |
| this.eventController = null; | |
| this.eventPump = null; | |
| this.session = null; | |
| if (session) { | |
| this.onDisconnected(null, 'disconnected'); | |
| } | |
| } |
— glm-5.1 via Qwen Code /review
| return this.session !== null; | ||
| } | ||
|
|
||
| get hasActiveSession(): boolean { |
There was a problem hiding this comment.
[Suggestion] hasActiveSession and isConnected both return this.session !== null — they are semantically identical. In AcpConnection, isConnected checks child process liveness while hasActiveSession checks sessionId !== null (orthogonal states: connected-without-session vs session-loaded-on-dead-transport). When QwenAgentManager branches on these two properties to distinguish transport state from session state, DaemonIdeConnection gives wrong answers. Consider tracking sessionId separately so the two getters report independently.
— glm-5.1 via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Qwen Code Review — PR #4199 (feat: add daemon connection spike)
Model: mimo-v2.5-pro | Date: 2026-05-17
Deterministic Analysis
- TypeScript strict: 0 findings in changed files
- ESLint: clean
- Build: pass
- Tests: 11/11 pass (22ms)
Review Stats
- 9 parallel agents + 3 reverse audit rounds
- 14 raw findings → 14 confirmed (6 Critical, 8 Suggestion)
Critical (6)
| # | Finding | File | Line |
|---|---|---|---|
| C1 | AskUserQuestion sends unvalidated optionId | daemonIdeConnection.ts | 359 |
| C2 | Unknown event types silently discarded | daemonIdeConnection.ts | 315 |
| C3 | Malformed permission_request silently dropped | daemonIdeConnection.ts | 320 |
| C4 | sendPrompt has no timeout/AbortSignal | daemonIdeConnection.ts | 206 |
| C5 | respondToPermission error burns event | daemonIdeConnection.ts | 281 |
| C6 | disconnect() hangs on pending permission | daemonIdeConnection.ts | 228 |
Suggestion (8)
| # | Finding | File | Line |
|---|---|---|---|
| S1 | session_update cast without validation | daemonIdeConnection.ts | 308 |
| S2 | options[0] fallback depends on ordering | daemonIdeConnection.ts | 435 |
| S3 | isConnected/hasActiveSession identical | daemonIdeConnection.ts | 237 |
| S4 | Dead validateDaemonBaseUrl in connect() | daemonIdeConnection.ts | 196 |
| S5 | resolvePermissionOptionId diverges from AcpConnection | daemonIdeConnection.ts | 413 |
| S6 | Callbacks not cleared on disconnect | daemonIdeConnection.ts | 170 |
| S7 | validateDaemonBaseUrl throws raw TypeError | daemonIdeConnection.ts | 112 |
| S8 | onEndTurn exception masks success | daemonIdeConnection.ts | 207 |
| ) { | ||
| return { outcome: { outcome: 'cancelled' } }; | ||
| } | ||
| const optionId = |
There was a problem hiding this comment.
[Critical] C1: AskUserQuestion path sends unvalidated optionId to daemon (confidence: 8/10)
When resolvePermissionOptionId returns undefined (user's choice not in daemon's option list), the ?? fallback sends the raw unvalidated askResponse.optionId to the daemon via selected outcome. The regular permission path at line 408 correctly returns cancelled in this case. A mismatched optionId causes the daemon to silently ignore the response and hang indefinitely.
| const optionId = | |
| const resolvedOptionId = | |
| this.resolvePermissionOptionId(request, askResponse.optionId); | |
| if (!resolvedOptionId) { | |
| return { outcome: { outcome: 'cancelled' } }; | |
| } | |
| return { | |
| outcome: { | |
| outcome: 'selected', | |
| optionId: resolvedOptionId, | |
| }, | |
| answers: askResponse.answers, | |
| } as RequestPermissionResponse; |
| case 'session_died': | ||
| this.handleSessionDied(event.data); | ||
| break; | ||
| default: |
There was a problem hiding this comment.
[Critical] C2: Unknown event types silently discarded (confidence: 9/10)
The default: break branch drops unrecognized event types with zero logging. The design doc states unknown events should be "logged as debug metadata." Without trace logging, event mismatches between daemon and extension are invisible to developers.
| default: | |
| default: | |
| console.debug('[DaemonIdeConnection] Unknown event type:', event.type, event.id); | |
| break; |
| } | ||
| } | ||
|
|
||
| private async handlePermissionRequest(data: unknown): Promise<void> { |
There was a problem hiding this comment.
[Critical] C3: Malformed permission_request silently dropped (confidence: 9/10)
When isPermissionRequestData returns false, handlePermissionRequest returns without logging. The daemon is waiting for a response via respondToPermission that will never arrive — the daemon session hangs indefinitely.
| private async handlePermissionRequest(data: unknown): Promise<void> { | |
| if (!isPermissionRequestData(data)) { | |
| console.warn('[DaemonIdeConnection] Malformed permission_request event, ignoring:', data); | |
| return; | |
| } |
| ): Promise<DaemonIdePromptResult> { | ||
| const session = this.ensureSession(); | ||
| const promptBlocks = normalizePrompt(prompt); | ||
| const response = await session.prompt({ prompt: promptBlocks }); |
There was a problem hiding this comment.
[Critical] C4: sendPrompt has no timeout/AbortSignal (confidence: 8/10)
session.prompt() is called without any signal or timeout. If the daemon hangs, the promise never settles and the UI shows a running prompt forever with no recovery path.
Consider adding a timeout mechanism or forwarding the session.close() abort signal to the prompt call.
| }); | ||
| } finally { | ||
| if (event.id !== undefined) { | ||
| this.lastSeenEventId = event.id; |
There was a problem hiding this comment.
[Critical] C5: respondToPermission error burns event permanently (confidence: 8/10)
lastSeenEventId is unconditionally advanced in the finally block. If respondToPermission throws (HTTP timeout, network error), the permission event is skipped forever — on reconnect the pump resumes from the next event. The daemon session hangs waiting for a response that will never come.
Consider only advancing lastSeenEventId after successful response, or retrying failed permission responses on reconnect.
| lastEventId: options.lastEventId, | ||
| }); | ||
| this.lastSeenEventId = options.lastEventId ?? this.session.lastEventId; | ||
|
|
There was a problem hiding this comment.
[Suggestion] S4: Dead validateDaemonBaseUrl in connect() (confidence: 7/10)
connect() calls validateDaemonBaseUrl but discards the return value, passing the original options.baseUrl to the factory. The factory validates again via its own createClient path. This is either dead code or should use the validated result.
| ); | ||
| } | ||
|
|
||
| private resolvePermissionOptionId( |
There was a problem hiding this comment.
[Suggestion] S5: resolvePermissionOptionId diverges from AcpConnection (confidence: 7/10)
When preferredOptionId is not found in options, daemon path returns undefined (→ cancel), while AcpConnection falls through to optionallySelectPermissionOptionId. This means options with no allow_once/proceed_once are auto-selected in AcpConnection but cancelled in DaemonIdeConnection — behavioral divergence.
| private eventPump: Promise<void> | null = null; | ||
| private lastSeenEventId: number | undefined; | ||
|
|
||
| onSessionUpdate: (data: SessionNotification) => void = () => {}; |
There was a problem hiding this comment.
[Suggestion] S6: Callbacks not cleared on disconnect (confidence: 6/10)
The 5 on* callbacks retain consumer closures after disconnect. In long-lived extension hosts where close() is not called, this leaks memory.
Consider adding a clearCallbacks() method called from disconnect().
| ); | ||
| } | ||
|
|
||
| function validateDaemonBaseUrl(baseUrl: string): string { |
There was a problem hiding this comment.
[Suggestion] S7: validateDaemonBaseUrl throws raw TypeError (confidence: 6/10)
new URL(baseUrl) throws a raw TypeError: Invalid URL with no context. Consider wrapping in a try-catch that throws a descriptive DaemonConnectionError with the invalid URL value.
| const session = this.ensureSession(); | ||
| const promptBlocks = normalizePrompt(prompt); | ||
| const response = await session.prompt({ prompt: promptBlocks }); | ||
| this.onEndTurn(response.stopReason); |
There was a problem hiding this comment.
[Suggestion] S8: onEndTurn callback exception masks success (confidence: 6/10)
If onEndTurn throws, sendPrompt rejects even though the prompt completed successfully. The caller may retry, causing duplicate agent actions. Consider wrapping in try-catch and logging the error instead of propagating.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
PR Review: feat(ide): add daemon connection spike
| Metric | Value |
|---|---|
| Files reviewed | 3 (code + test + design doc) |
| Lines of code | 992 (451 production + 541 test) |
| Build status | clean |
| Test status | pass |
| Deterministic analysis | 0 findings (tsc + eslint clean) |
| Findings | 5 Critical, 3 Suggestion |
| Model | mimo-v2.5-pro |
| Iterations | 2 (reverse audit found 1 additional Critical) |
Critical (5)
-
pumpEvents finally block eventPump cleanup always fails -
this.session === sessionguard always evaluates tofalsebecauseclearCurrentSessionnullsthis.sessionon all three exit paths (session_died, stream_ended, daemon_error) before pumpEvents reaches its finally block.this.eventPumpremains a stale resolved promise after the pump is dead. -
disconnect() deadlocks when permission callback is in-flight -
abort()terminates the SSE generator but cannot interrupt the JS-levelawait this.onPermissionRequest(request).disconnect()awaits a pump that is blocked on a user callback that will never return. -
handleSessionDied doesn't validate sessionId - On reconnect with
resume: true, the daemon may replay the old session_died event. Nodata.sessionId !== this.session.sessionIdguard exists, so a stale event kills the freshly connected session. -
Concurrent connect() calls orphan the first session - No mutex or connectPromise guard. Two overlapping calls both create sessions. The second overwrites
this.sessionandthis.eventController. The first session's pump is orphaned; if it errors, it aborts the second session's controller (stale reference), killing the live stream withisConnected === trueand noonDisconnectedcallback. -
resolvePermissionOptionId fallback chain completely untested - The four-stage fallback (
allow_oncekind ->proceed_onceid -> namespacedproceed_once->options[0]) is never exercised by any test.
Suggestion (3)
-
event.data cast to SessionNotification without validation -
this.onSessionUpdate(event.data as SessionNotification)with no runtime type guard. Malformed SSE data flows unvalidated to webview consumers. -
questions/metadata casts without validation -
isPermissionRequestDatavalidates outer shape but notrawInputcontent.questionsis only checked asArray.isArray, not element structure. -
EventQueue.fail() does not resolve pending waiters -
fail()setsthis.failurebut doesn't drainthis.waiters. Tests callingfail()with a pending waiter hang until timeout.
Fix guidance for Critical findings:
- Remove the
if (this.session === session)guard -this.eventPump = null;unconditionally in the finally block. - Thread the abort signal through permission resolution and race the user callback against it.
- Guard with
if (data.sessionId !== this.session.sessionId) return;before tearing down. - Add a
connectPromiseguard to serialize concurrent connect() calls. - Add a test with
{ optionId: undefined }return andkind: 'allow_once'option.
| toSafeErrorMessage(error), | ||
| ); | ||
| this.eventController?.abort(); | ||
| this.clearCurrentSession(session, 'daemon_error'); |
There was a problem hiding this comment.
[Critical] The if (this.session === session) guard always evaluates to false - clearCurrentSession() sets this.session = null on all three exit paths (session_died, stream_ended, daemon_error) before this finally block runs. this.eventPump remains a stale resolved-promise reference after the pump is dead.
Fix: remove the guard - set this.eventPump = null; unconditionally.
| return await this.ensureSession().setModel(modelId); | ||
| } | ||
|
|
||
| async disconnect(): Promise<void> { |
There was a problem hiding this comment.
[Critical] Deadlock scenario: when the pump is inside handlePermissionRequest -> await this.onPermissionRequest(request), abort() terminates the SSE generator but not the JS-level await on the user callback. disconnect() then hangs on await this.eventPump forever.
Fix: thread the abort signal into permission resolution and race onPermissionRequest against it, or use AbortSignal.any() to combine the permission abort with the controller signal.
| this.eventController?.abort(); | ||
| if (this.session) { | ||
| this.clearCurrentSession(this.session, reason); | ||
| } else { |
There was a problem hiding this comment.
[Critical] On reconnect with resume: true, the daemon may replay the old session_died event from the prior session. This handler never checks data.sessionId against this.session.sessionId, so a stale event tears down the freshly connected session.
Fix: guard with if (data.sessionId !== this.session.sessionId) return; before calling clearCurrentSession().
| onEndTurn: (reason?: string) => void = () => {}; | ||
| onDisconnected: (code: number | null, signal: string | null) => void = | ||
| () => {}; | ||
|
|
There was a problem hiding this comment.
[Critical] No mutex - concurrent connect() calls both pass the this.session guard, both create sessions and SSE streams. The second call overwrites this.session and this.eventController. The first session's pump is orphaned; if it errors, it reads the stale this.eventController reference (now belonging to session 2) and aborts the live stream. isConnected stays true with no onDisconnected callback.
Fix: add a private connectPromise: Promise<void> | null = null; guard and return/reuse the in-flight promise.
| cancel: vi.fn().mockResolvedValue(undefined), | ||
| setModel: vi.fn().mockResolvedValue({}), | ||
| respondToPermission: vi.fn().mockResolvedValue(true), | ||
| }; |
There was a problem hiding this comment.
[Critical] No test exercises the resolvePermissionOptionId fallback chain when onPermissionRequest returns { optionId: undefined }. The four-stage fallback (allow_once kind -> proceed_once id -> namespaced proceed_once -> options[0]) is completely untested.
Fix: add a test that provides options with { kind: "allow_once", id: "test" } and returns { optionId: undefined } from onPermissionRequest, verifying allow_once-test is sent.
| private async handleEvent(event: DaemonIdeEvent): Promise<void> { | ||
| switch (event.type) { | ||
| case 'session_update': | ||
| this.onSessionUpdate(event.data as SessionNotification); |
There was a problem hiding this comment.
[Suggestion] event.data is cast to SessionNotification with as and no runtime validation. Malformed SSE data flows unvalidated to webview consumers. Consider adding a isSessionNotificationData() guard analogous to isPermissionRequestData().
|
|
||
| if (isAskUserQuestion) { | ||
| const askResponse = await this.onAskUserQuestion({ | ||
| sessionId: request.sessionId, |
There was a problem hiding this comment.
[Suggestion] isPermissionRequestData validates the outer shape but not the inner rawInput content. questions is only checked via Array.isArray, not element structure. Consider adding element-level validation before casting.
| } | ||
|
|
||
| fail(error: unknown): void { | ||
| this.failure = error; |
There was a problem hiding this comment.
[Suggestion] EventQueue.fail() sets this.failure but does not drain this.waiters. If a waiter is pending when fail() is called, the test hangs until timeout. Consider calling this.close() to drain waiters, or rejecting them with the error.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
PR Review: feat(ide): add daemon connection spike
| Metric | Value |
|---|---|
| Files reviewed | 3 (code + test + design doc) |
| Lines of code | 992 (451 production + 541 test) |
| Build status | clean |
| Test status | pass |
| Deterministic analysis | 0 findings (tsc + eslint clean) |
| Findings | 5 Critical, 3 Suggestion |
| Model | mimo-v2.5-pro |
| Iterations | 2 (reverse audit found 1 additional Critical) |
Critical (5)
-
pumpEvents finally block eventPump cleanup always fails -
this.session === sessionguard always evaluates tofalsebecauseclearCurrentSessionnullsthis.sessionon all three exit paths (session_died, stream_ended, daemon_error) before pumpEvents reaches its finally block.this.eventPumpremains a stale resolved promise after the pump is dead. -
disconnect() deadlocks when permission callback is in-flight -
abort()terminates the SSE generator but cannot interrupt the JS-levelawait this.onPermissionRequest(request).disconnect()awaits a pump that is blocked on a user callback that will never return. -
handleSessionDied doesn't validate sessionId - On reconnect with
resume: true, the daemon may replay the old session_died event. Nodata.sessionId !== this.session.sessionIdguard exists, so a stale event kills the freshly connected session. -
Concurrent connect() calls orphan the first session - No mutex or connectPromise guard. Two overlapping calls both create sessions. The second overwrites
this.sessionandthis.eventController. The first session's pump is orphaned; if it errors, it aborts the second session's controller (stale reference), killing the live stream withisConnected === trueand noonDisconnectedcallback. -
resolvePermissionOptionId fallback chain completely untested - The four-stage fallback (
allow_oncekind ->proceed_onceid -> namespacedproceed_once->options[0]) is never exercised by any test.
Suggestion (3)
-
event.data cast to SessionNotification without validation -
this.onSessionUpdate(event.data as SessionNotification)with no runtime type guard. Malformed SSE data flows unvalidated to webview consumers. -
questions/metadata casts without validation -
isPermissionRequestDatavalidates outer shape but notrawInputcontent.questionsis only checked asArray.isArray, not element structure. -
EventQueue.fail() does not resolve pending waiters -
fail()setsthis.failurebut doesn't drainthis.waiters. Tests callingfail()with a pending waiter hang until timeout.
Fix guidance for Critical findings:
- Remove the
if (this.session === session)guard -this.eventPump = null;unconditionally in the finally block. - Thread the abort signal through permission resolution and race the user callback against it.
- Guard with
if (data.sessionId !== this.session.sessionId) return;before tearing down. - Add a
connectPromiseguard to serialize concurrent connect() calls. - Add a test with
{ optionId: undefined }return andkind: 'allow_once'option.
| if (this.session === session) { | ||
| this.eventPump = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
[Critical] The if (this.session === session) guard always evaluates to false - clearCurrentSession() sets this.session = null on all three exit paths (session_died, stream_ended, daemon_error) before this finally block runs. this.eventPump remains a stale resolved-promise reference after the pump is dead.
Fix: remove the guard - set this.eventPump = null; unconditionally.
| try { | ||
| await this.eventPump; | ||
| } catch { | ||
| /* pump errors are converted into callbacks */ |
There was a problem hiding this comment.
[Critical] Deadlock scenario: when the pump is inside handlePermissionRequest -> await this.onPermissionRequest(request), abort() terminates the SSE generator but not the JS-level await on the user callback. disconnect() then hangs on await this.eventPump forever.
Fix: thread the abort signal into permission resolution and race onPermissionRequest against it, or use AbortSignal.any() to combine the permission abort with the controller signal.
| } | ||
|
|
||
| private isCancelledOption(optionId?: string): boolean { | ||
| return ( |
There was a problem hiding this comment.
[Critical] On reconnect with resume: true, the daemon may replay the old session_died event from the prior session. This handler never checks data.sessionId against this.session.sessionId, so a stale event tears down the freshly connected session.
Fix: guard with if (data.sessionId !== this.session.sessionId) return; before calling clearCurrentSession().
| await this.disconnect(); | ||
| } | ||
|
|
||
| const factory = options.sessionFactory ?? createSdkDaemonSessionFactory(); |
There was a problem hiding this comment.
[Critical] No mutex - concurrent connect() calls both pass the this.session guard, both create sessions and SSE streams. The second call overwrites this.session and this.eventController. The first session's pump is orphaned; if it errors, it reads the stale this.eventController reference (now belonging to session 2) and aborts the live stream. isConnected stays true with no onDisconnected callback.
Fix: add a private connectPromise: Promise<void> | null = null; guard and return/reuse the in-flight promise.
| async function waitFor(assertion: () => void): Promise<void> { | ||
| let lastError: unknown; | ||
| for (let i = 0; i < 20; i += 1) { | ||
| try { |
There was a problem hiding this comment.
[Critical] No test exercises the resolvePermissionOptionId fallback chain when onPermissionRequest returns { optionId: undefined }. The four-stage fallback (allow_once kind -> proceed_once id -> namespaced proceed_once -> options[0]) is completely untested.
Fix: add a test that provides options with { kind: "allow_once", id: "test" } and returns { optionId: undefined } from onPermissionRequest, verifying allow_once-test is sent.
| await this.handlePermissionRequest(event.data); | ||
| break; | ||
| case 'session_died': | ||
| this.handleSessionDied(event.data); |
There was a problem hiding this comment.
[Suggestion] event.data is cast to SessionNotification with as and no runtime validation. Malformed SSE data flows unvalidated to webview consumers. Consider adding a isSessionNotificationData() guard analogous to isPermissionRequestData().
| }); | ||
| if ( | ||
| !askResponse.optionId || | ||
| this.isCancelledOption(askResponse.optionId) |
There was a problem hiding this comment.
[Suggestion] isPermissionRequestData validates the outer shape but not the inner rawInput content. questions is only checked via Array.isArray, not element structure. Consider adding element-level validation before casting.
|
|
||
| interface FakeSession extends DaemonIdeSessionClient { | ||
| prompt: ReturnType<typeof vi.fn>; | ||
| cancel: ReturnType<typeof vi.fn>; |
There was a problem hiding this comment.
[Suggestion] EventQueue.fail() sets this.failure but does not drain this.waiters. If a waiter is pending when fail() is called, the test hangs until timeout. Consider calling this.close() to drain waiters, or rejecting them with the error.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
PR Review: feat(ide): add daemon connection spike
| Metric | Value |
|---|---|
| Files reviewed | 3 (code + test + design doc) |
| Lines of code | 992 (451 production + 541 test) |
| Build status | clean |
| Test status | pass |
| Deterministic analysis | 0 findings (tsc + eslint clean) |
| Findings | 5 Critical, 3 Suggestion |
| Model | mimo-v2.5-pro |
| Iterations | 2 (reverse audit found 1 additional Critical) |
Critical (5)
-
pumpEvents finally block eventPump cleanup always fails -
this.session === sessionguard always evaluates tofalsebecauseclearCurrentSessionnullsthis.sessionon all three exit paths (session_died, stream_ended, daemon_error) before pumpEvents reaches its finally block.this.eventPumpremains a stale resolved promise after the pump is dead. -
disconnect() deadlocks when permission callback is in-flight -
abort()terminates the SSE generator but cannot interrupt the JS-levelawait this.onPermissionRequest(request).disconnect()awaits a pump that is blocked on a user callback that will never return. -
handleSessionDied doesn't validate sessionId - On reconnect with
resume: true, the daemon may replay the old session_died event. Nodata.sessionId !== this.session.sessionIdguard exists, so a stale event kills the freshly connected session. -
Concurrent connect() calls orphan the first session - No mutex or connectPromise guard. Two overlapping calls both create sessions. The second overwrites
this.sessionandthis.eventController. The first session's pump is orphaned; if it errors, it aborts the second session's controller (stale reference), killing the live stream withisConnected === trueand noonDisconnectedcallback. -
resolvePermissionOptionId fallback chain completely untested - The four-stage fallback (
allow_oncekind ->proceed_onceid -> namespacedproceed_once->options[0]) is never exercised by any test.
Suggestion (3)
-
event.data cast to SessionNotification without validation -
this.onSessionUpdate(event.data as SessionNotification)with no runtime type guard. Malformed SSE data flows unvalidated to webview consumers. -
questions/metadata casts without validation -
isPermissionRequestDatavalidates outer shape but notrawInputcontent.questionsis only checked asArray.isArray, not element structure. -
EventQueue.fail() does not resolve pending waiters -
fail()setsthis.failurebut doesn't drainthis.waiters. Tests callingfail()with a pending waiter hang until timeout.
Fix guidance for Critical findings:
- Remove the
if (this.session === session)guard -this.eventPump = null;unconditionally in the finally block. - Thread the abort signal through permission resolution and race the user callback against it.
- Guard with
if (data.sessionId !== this.session.sessionId) return;before tearing down. - Add a
connectPromiseguard to serialize concurrent connect() calls. - Add a test with
{ optionId: undefined }return andkind: 'allow_once'option.
| toSafeErrorMessage(error), | ||
| ); | ||
| this.eventController?.abort(); | ||
| this.clearCurrentSession(session, 'daemon_error'); |
There was a problem hiding this comment.
[Critical] The if (this.session === session) guard always evaluates to false - clearCurrentSession() sets this.session = null on all three exit paths (session_died, stream_ended, daemon_error) before this finally block runs. this.eventPump remains a stale resolved-promise reference after the pump is dead.
Fix: remove the guard - set this.eventPump = null; unconditionally.
| return await this.ensureSession().setModel(modelId); | ||
| } | ||
|
|
||
| async disconnect(): Promise<void> { |
There was a problem hiding this comment.
[Critical] Deadlock scenario: when the pump is inside handlePermissionRequest -> await this.onPermissionRequest(request), abort() terminates the SSE generator but not the JS-level await on the user callback. disconnect() then hangs on await this.eventPump forever.
Fix: thread the abort signal into permission resolution and race onPermissionRequest against it, or use AbortSignal.any() to combine the permission abort with the controller signal.
| this.eventController?.abort(); | ||
| if (this.session) { | ||
| this.clearCurrentSession(this.session, reason); | ||
| } else { |
There was a problem hiding this comment.
[Critical] On reconnect with resume: true, the daemon may replay the old session_died event from the prior session. This handler never checks data.sessionId against this.session.sessionId, so a stale event tears down the freshly connected session.
Fix: guard with if (data.sessionId !== this.session.sessionId) return; before calling clearCurrentSession().
| onEndTurn: (reason?: string) => void = () => {}; | ||
| onDisconnected: (code: number | null, signal: string | null) => void = | ||
| () => {}; | ||
|
|
There was a problem hiding this comment.
[Critical] No mutex - concurrent connect() calls both pass the this.session guard, both create sessions and SSE streams. The second call overwrites this.session and this.eventController. The first session's pump is orphaned; if it errors, it reads the stale this.eventController reference (now belonging to session 2) and aborts the live stream. isConnected stays true with no onDisconnected callback.
Fix: add a private connectPromise: Promise<void> | null = null; guard and return/reuse the in-flight promise.
| cancel: vi.fn().mockResolvedValue(undefined), | ||
| setModel: vi.fn().mockResolvedValue({}), | ||
| respondToPermission: vi.fn().mockResolvedValue(true), | ||
| }; |
There was a problem hiding this comment.
[Critical] No test exercises the resolvePermissionOptionId fallback chain when onPermissionRequest returns { optionId: undefined }. The four-stage fallback (allow_once kind -> proceed_once id -> namespaced proceed_once -> options[0]) is completely untested.
Fix: add a test that provides options with { kind: "allow_once", id: "test" } and returns { optionId: undefined } from onPermissionRequest, verifying allow_once-test is sent.
| private async handleEvent(event: DaemonIdeEvent): Promise<void> { | ||
| switch (event.type) { | ||
| case 'session_update': | ||
| this.onSessionUpdate(event.data as SessionNotification); |
There was a problem hiding this comment.
[Suggestion] event.data is cast to SessionNotification with as and no runtime validation. Malformed SSE data flows unvalidated to webview consumers. Consider adding a isSessionNotificationData() guard analogous to isPermissionRequestData().
|
|
||
| if (isAskUserQuestion) { | ||
| const askResponse = await this.onAskUserQuestion({ | ||
| sessionId: request.sessionId, |
There was a problem hiding this comment.
[Suggestion] isPermissionRequestData validates the outer shape but not the inner rawInput content. questions is only checked via Array.isArray, not element structure. Consider adding element-level validation before casting.
| } | ||
|
|
||
| fail(error: unknown): void { | ||
| this.failure = error; |
There was a problem hiding this comment.
[Suggestion] EventQueue.fail() sets this.failure but does not drain this.waiters. If a waiter is pending when fail() is called, the test hangs until timeout. Consider calling this.close() to drain waiters, or rejecting them with the error.
| toSafeErrorMessage(error), | ||
| ); | ||
| this.eventController?.abort(); | ||
| this.clearCurrentSession(session, 'daemon_error'); |
There was a problem hiding this comment.
[Critical] The if (this.session === session) guard always evaluates to false - clearCurrentSession() sets this.session = null on all three exit paths (session_died, stream_ended, daemon_error) before this finally block runs. this.eventPump remains a stale resolved-promise reference after the pump is dead.
Fix: remove the guard - set this.eventPump = null; unconditionally.
| return await this.ensureSession().setModel(modelId); | ||
| } | ||
|
|
||
| async disconnect(): Promise<void> { |
There was a problem hiding this comment.
[Critical] Deadlock scenario: when the pump is inside handlePermissionRequest -> await this.onPermissionRequest(request), abort() terminates the SSE generator but not the JS-level await on the user callback. disconnect() then hangs on await this.eventPump forever.
Fix: thread the abort signal into permission resolution and race onPermissionRequest against it, or use AbortSignal.any() to combine the permission abort with the controller signal.
| this.eventController?.abort(); | ||
| if (this.session) { | ||
| this.clearCurrentSession(this.session, reason); | ||
| } else { |
There was a problem hiding this comment.
[Critical] On reconnect with resume: true, the daemon may replay the old session_died event from the prior session. This handler never checks data.sessionId against this.session.sessionId, so a stale event tears down the freshly connected session.
Fix: guard with if (data.sessionId !== this.session.sessionId) return; before calling clearCurrentSession().
| onEndTurn: (reason?: string) => void = () => {}; | ||
| onDisconnected: (code: number | null, signal: string | null) => void = | ||
| () => {}; | ||
|
|
There was a problem hiding this comment.
[Critical] No mutex - concurrent connect() calls both pass the this.session guard, both create sessions and SSE streams. The second call overwrites this.session and this.eventController. The first session's pump is orphaned; if it errors, it reads the stale this.eventController reference (now belonging to session 2) and aborts the live stream. isConnected stays true with no onDisconnected callback.
Fix: add a private connectPromise: Promise<void> | null = null; guard and return/reuse the in-flight promise.
| cancel: vi.fn().mockResolvedValue(undefined), | ||
| setModel: vi.fn().mockResolvedValue({}), | ||
| respondToPermission: vi.fn().mockResolvedValue(true), | ||
| }; |
There was a problem hiding this comment.
[Critical] No test exercises the resolvePermissionOptionId fallback chain when onPermissionRequest returns { optionId: undefined }. The four-stage fallback (allow_once kind -> proceed_once id -> namespaced proceed_once -> options[0]) is completely untested.
Fix: add a test that provides options with { kind: "allow_once", id: "test" } and returns { optionId: undefined } from onPermissionRequest, verifying allow_once-test is sent.
| private async handleEvent(event: DaemonIdeEvent): Promise<void> { | ||
| switch (event.type) { | ||
| case 'session_update': | ||
| this.onSessionUpdate(event.data as SessionNotification); |
There was a problem hiding this comment.
[Suggestion] event.data is cast to SessionNotification with as and no runtime validation. Malformed SSE data flows unvalidated to webview consumers. Consider adding a isSessionNotificationData() guard analogous to isPermissionRequestData().
|
|
||
| if (isAskUserQuestion) { | ||
| const askResponse = await this.onAskUserQuestion({ | ||
| sessionId: request.sessionId, |
There was a problem hiding this comment.
[Suggestion] isPermissionRequestData validates the outer shape but not the inner rawInput content. questions is only checked via Array.isArray, not element structure. Consider adding element-level validation before casting.
| } | ||
|
|
||
| fail(error: unknown): void { | ||
| this.failure = error; |
There was a problem hiding this comment.
[Suggestion] EventQueue.fail() sets this.failure but does not drain this.waiters. If a waiter is pending when fail() is called, the test hangs until timeout. Consider calling this.close() to drain waiters, or rejecting them with the error.
| } | ||
| if (url.username || url.password) { | ||
| throw new Error('Daemon baseUrl must not contain credentials'); | ||
| } |
There was a problem hiding this comment.
[Suggestion] isPermissionRequestData validates the outer shape (requestId, toolCall, options) but not the inner rawInput content. The rawInput flows into resolvePermissionResponse where rawInput['questions'] is cast to AskUserQuestionRequest['questions'] without checking that each element has the required question and options fields.
A malformed rawInput with {questions: [{}]} would pass the guard and produce a broken permission prompt downstream.
| case 'session_update': | ||
| this.onSessionUpdate(event.data as SessionNotification); | ||
| break; | ||
| case 'permission_request': |
There was a problem hiding this comment.
[Suggestion] event.data as SessionNotification is an unsafe cast on untrusted data received over SSE from the daemon. If the daemon sends a session_update event with a different shape (e.g. during a protocol version mismatch), the cast succeeds at runtime and the caller receives data that doesn't conform to the SDK type — potentially crashing the onSessionUpdate callback.
The existing isPermissionRequestData guard in the same switch statement demonstrates the right pattern. Add a similar shape check for session_update events, or at minimum catch the downstream callback to prevent the event handler from swallowing subsequent events.
| }, | ||
| "dependencies": { | ||
| "@agentclientprotocol/sdk": "^0.14.1", | ||
| "@qwen-code/sdk": "*", |
There was a problem hiding this comment.
[Suggestion] "@qwen-code/sdk": "*" resolves to any version. Since the SDK is dynamically imported at runtime, a breaking change in the SDK's export shape would silently produce a runtime "Loaded @qwen-code/sdk does not expose daemon clients" error instead of a clear install-time version mismatch.
Consider pinning to a minimum major version or using a caret range (^1.0.0) to get install-time warnings on breaking changes.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Code Review: DaemonIdeConnection
Well-structured spike that cleanly mirrors the ACP ProcessConnection shape while swapping the child-process transport for an HTTP/SSE daemon session. The test suite is thorough (17 cases, 766 lines) and the separation of concerns (sessionFactory, event pump lifecycle, pumpGeneration guard) is solid.
Key concerns
| Severity | Area | Issue |
|---|---|---|
| Critical | SSRF | validateDaemonBaseUrl doesn't restrict to loopback - workspace settings.json can redirect the daemon URL and exfiltrate the auth token |
| Critical | Resilience | sendPrompt can hang forever if the daemon dies mid-request - no try/catch, no AbortSignal, no timeout |
| Suggestion | Observability | pumpEvents error logs lack session ID; clearCurrentSession is completely silent |
| Suggestion | Robustness | Unsafe as SessionNotification cast on untrusted data; isPermissionRequestData doesn't validate rawInput inner shape |
| Suggestion | Supply chain | @qwen-code/sdk wildcard dependency |
| Suggestion | Debuggability | Permission handler catch loses request context |
Architecture notes
- new Function bypass for esbuild is pragmatic for the spike but should be documented with a SECURITY comment.
- onDisconnected default is a no-op - production consumers MUST override this or silently lose sessions.
- isConnected vs hasActiveSession are identical - differentiate or deduplicate.
|
Generated by GPT-5 model Handled the latest valid review comments in
Validation run locally:
Notes / triage:
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Code Review: DaemonIdeConnection
Overall: Well-structured code with solid error handling. TypeScript compiles cleanly, 18/18 tests pass. Previous review feedback has been addressed.
Critical Issues
-
UI Forgery via
rawInput.questionsinjection (L362): The permission routing logic checksrawInput['questions']to decide whether to invokeonAskUserQuestionvsonPermissionRequest. A malicious daemon can injectquestionsinto anypermission_request(e.g.,kind: 'execute_command') to replace the real approval UI with a fake survey. The user's answer is then mapped to a permissionoptionId, silently approving the original tool call. Fix: gate onrequest.toolCall?.kind === 'ask_user_question'instead of (or in addition to)rawInput.questions. -
clearCurrentSessionis completely silent (L560): The most critical state transition — nulling out eventController, eventPump, session, and firing onDisconnected — has zero logging. Thereasonparameter is available but unused. -
No
sessionIdin any log message: Allconsole.warn/console.debugcalls use the generic[DaemonIdeConnection]prefix with no session identifier. Under concurrent sessions, logs are uncorrelatable.
Suggestions
- Dead fallback code in
resolvePermissionOptionId— the allow_once/proceed_once chain is unreachable with the default callback. handleSessionDiedextractsdata['reason']but never logs it.sendPromptandconnectInternalhave zero observability.- Divergent loopback validation (
isLoopbackHostnameaccepts 127.0.0.0/8, server uses fixed allowlist). - Dual replay cursors (connection vs SDK) are correct but undocumented.
Inline comments below with details and suggested fixes.
| } | ||
| } | ||
|
|
||
| private async handlePermissionRequest( |
There was a problem hiding this comment.
[Critical] UI Forgery via rawInput.questions injection.
This routes on rawInput.questions presence, not request.toolCall.kind. A malicious daemon can inject questions into a permission_request with kind: 'execute_command', replacing the real approval UI with a fake survey. The user's answer maps to a permission optionId, silently approving the original tool call.
Fix — gate on toolCall.kind:
const isAskUserQuestion =
request.toolCall?.kind === 'ask_user_question' &&
isRecord(rawInput) &&
Array.isArray(rawInput['questions']);
if (isAskUserQuestion) {This matches the discriminator used in WebViewProvider.ts:506.
| this.eventController = null; | ||
| this.eventPump = null; | ||
| this.session = null; | ||
| this.onDisconnected(null, reason); |
There was a problem hiding this comment.
[Critical] clearCurrentSession is completely silent.
This tears down all session state and fires onDisconnected with zero logging. At 3 AM when a user reports "my session disappeared," there is no log entry telling you when, why, or what the sessionId was.
Suggested fix:
private clearCurrentSession(
session: DaemonIdeSessionClient,
reason: string,
): void {
if (this.session !== session) {
return;
}
console.log('[DaemonIdeConnection] Clearing session:', {
sessionId: session.sessionId,
reason,
});
this.eventController = null;
this.eventPump = null;
this.session = null;
this.onDisconnected(null, reason);
}| } | ||
| this.eventController = null; | ||
| this.eventPump = null; | ||
| if (session && this.session === session) { |
There was a problem hiding this comment.
[Critical] No sessionId in any log message.
All console.warn/console.debug calls in this file use the generic [DaemonIdeConnection] prefix with no session identifier. When multiple daemon sessions exist (the code explicitly supports concurrent connects), logs from different sessions are indistinguishable.
Suggested fix — add { sessionId } to all log objects, e.g.:
console.warn('[DaemonIdeConnection] Event stream failed:', {
sessionId: session.sessionId,
error: toSafeErrorMessage(error),
});This applies to all ~8 log statements in the file.
| } | ||
|
|
||
| private resolvePermissionOptionId( | ||
| request: RequestPermissionRequest, |
There was a problem hiding this comment.
[Suggestion] Dead fallback code.
With the default onPermissionRequest callback returning { optionId: 'cancel' }, preferredOptionId is always 'cancel', so the if (preferredOptionId) branch at L521 always returns before reaching this fallback chain. The allow_once/proceed_once logic is unreachable in the default configuration.
If a custom callback returns {} (no optionId), this fallback auto-selects allow_once — which could be a privilege escalation vector if the daemon controls the kind field.
Suggested fix — either remove the dead fallback or document the contract that integrators must NOT return {} from onPermissionRequest.
| ? data['reason'] | ||
| : 'session_died'; | ||
| this.eventController?.abort(); | ||
| this.clearCurrentSession(this.session, reason); |
There was a problem hiding this comment.
[Suggestion] handleSessionDied extracts the death reason but never logs it.
The reason variable is computed here and passed to clearCurrentSession, but there's no log statement. When the daemon kills a session (OOM, admin action, crash), the reason is available in the event payload but invisible in the extension host logs.
Suggested fix — add before the abort:
console.log('[DaemonIdeConnection] Session died:', {
sessionId: this.session.sessionId,
reason,
});| return typeof value === 'object' && value !== null; | ||
| } | ||
|
|
||
| function isLoopbackHostname(hostname: string): boolean { |
There was a problem hiding this comment.
[Suggestion] Divergent loopback validation.
This accepts the entire 127.0.0.0/8 range (127.x.y.z), while the canonical isLoopbackBind in packages/cli/src/serve/loopbackBinds.ts uses a fixed allowlist (127.0.0.1, localhost, ::1, [::1]). The two functions answer the same semantic question with different answers and live in separate packages with no shared import.
If someone adds host.docker.internal to the server-side set, they'll never discover this independent reimplementation. The client will continue to reject URLs the server now accepts.
Suggested fix — add a cross-reference comment: // Keep in sync with packages/cli/src/serve/loopbackBinds.ts
| isRecord(value['toolCall']) && | ||
| Array.isArray(value['options']) | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Suggestion] Dual replay cursors — correct but undocumented.
Both DaemonIdeConnection and DaemonSessionClient maintain independent lastSeenEventId cursors. The connection cursor skips advancing when handleEvent returns false (failed permission response). The SDK cursor always advances. On reconnect, a fresh SDK instance is seeded from the connection's cursor.
This design is correct but a developer reading the SDK's subscribeEvents might "clean up" the redundant-looking connection cursor, silently dropping permission requests that fail.
Suggested fix — add a comment explaining the intentional divergence:
// Authoritative replay cursor. Intentionally diverges from the SDK's
// cursor on permission response failures to ensure at-least-once delivery.
private lastSeenEventId: string | undefined;|
|
||
| async cancelSession(): Promise<void> { | ||
| const session = this.session; | ||
| if (!session) { |
There was a problem hiding this comment.
[Suggestion] sendPrompt has zero observability.
No logging on entry, success, or error path. If a user reports "my prompt was sent but nothing happened," there is no log trail showing whether the prompt was dispatched, whether the daemon responded, or what error occurred.
Suggested fix — add entry/exit/error logging with sessionId and stopReason.
| const requestId = data['requestId']; | ||
| const request = data; | ||
| const session = this.session; | ||
| if (!session) { |
There was a problem hiding this comment.
[Suggestion] handlePermissionRequest silently drops permission requests when there is no active session.
When this.session is null (e.g., after a disconnect during permission resolution), the method returns true with no log:
const session = this.session;
if (!session) {
return true; // no console.warn, no debug info
}This makes production debugging impossible — the daemon will timeout waiting for a response, but the IDE side shows no trace of the dropped request.
| if (!session) { | |
| if (!session) { | |
| console.warn('[DaemonIdeConnection] Dropping permission request: not connected', { | |
| requestId: data['requestId'], | |
| }); | |
| return true; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| if (!response) { | ||
| return true; | ||
| } | ||
| if (this.session !== session) { |
There was a problem hiding this comment.
[Suggestion] Permission response silently dropped when session changes during resolution.
After await this.resolvePermissionResponseUntilAbort(...), the code checks if (this.session !== session) return true; — but the user's permission choice (which may have taken seconds to collect) is discarded without any log. The daemon waits for a response that will never arrive.
| if (this.session !== session) { | |
| if (this.session !== session) { | |
| console.warn('[DaemonIdeConnection] Permission response dropped: session changed', { | |
| requestId, | |
| originalSessionId: session?.sessionId, | |
| }); | |
| return true; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| isRecord(data) && typeof data['sessionId'] === 'string' | ||
| ? data['sessionId'] | ||
| : undefined; | ||
| if (!this.session) { |
There was a problem hiding this comment.
[Suggestion] handleSessionDied no longer calls onDisconnected when session is already null.
The old code had a safety net:
if (this.session) {
this.clearCurrentSession(this.session, reason);
} else {
this.onDisconnected(null, reason); // safety net
}The new code returns early without notifying consumers. If a duplicate session_died event arrives (e.g., replay + real-time), the second one is silently swallowed instead of at least logging the condition.
| if (!this.session) { | |
| if (!this.session) { | |
| console.debug('[DaemonIdeConnection] session_died received with no active session', { | |
| eventSessionId, | |
| }); | |
| return; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return typeof value === 'object' && value !== null; | ||
| } | ||
|
|
||
| function isLoopbackHostname(hostname: string): boolean { |
There was a problem hiding this comment.
[Suggestion] isLoopbackHostname rejects IPv4-mapped IPv6 loopback addresses.
::ffff:127.0.0.1 (common in dual-stack environments and container runtimes) fails all three checks — it's not localhost, not ::1, and doesn't match the 127.x.x.x IPv4 pattern after bracket stripping. This causes a confusing error: "Daemon baseUrl must target a loopback address, got '::ffff:127.0.0.1'" even though the address is functionally equivalent to 127.0.0.1.
| function isLoopbackHostname(hostname: string): boolean { | |
| if (normalized.startsWith('::ffff:')) { | |
| const ipv4 = normalized.slice(7); | |
| if (ipv4.startsWith('127.')) { | |
| const parts = ipv4.split('.'); | |
| if (parts.length === 4 && parts.every(p => /^\d+$/.test(p) && +p >= 0 && +p <= 255)) { | |
| return true; | |
| } | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| `Daemon baseUrl must target a loopback address, got "${url.hostname}"`, | ||
| ); | ||
| } | ||
| return url.href; |
There was a problem hiding this comment.
[Suggestion] validateDaemonBaseUrl now returns url.href instead of raw baseUrl.
new URL('http://127.0.0.1:4170').href produces 'http://127.0.0.1:4170/' (with trailing slash). If DaemonClient internally uses string concatenation for URL construction (baseUrl + 'api/...'), this creates double-slash paths like http://127.0.0.1:4170//api/....
Verify that @qwen-code/sdk DaemonClient uses new URL(path, baseUrl) for path construction. If it uses string concatenation, strip the trailing slash:
| return url.href; | |
| return url.href.replace(/\/$/, ''); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| SessionNotification, | ||
| } from '@agentclientprotocol/sdk'; | ||
| import { | ||
| DaemonClient, |
There was a problem hiding this comment.
[Suggestion] Dynamic import bypass removed without preserving rationale comment.
The old code used new Function('specifier', 'return import(specifier)') with an explicit comment: "Uses new Function to bypass esbuild static analysis so @qwen-code/sdk is loaded dynamically at runtime rather than bundled into the extension." Now that the SDK is statically imported, the rationale for why the bypass existed (and what constraints the SDK must satisfy for safe bundling — pure JS, no native addons, no __dirname usage) is lost. Future maintainers upgrading the SDK won't know what can silently break.
Add a comment in esbuild.js or above the import documenting that @qwen-code/sdk is intentionally bundled and must remain pure JS with no native dependencies.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| this.eventController = null; | ||
| this.eventPump = null; | ||
| if (session && this.session === session) { |
There was a problem hiding this comment.
[Suggestion] disconnect() skips onDisconnected callback when session is already null.
The guard if (session && this.session === session) means callers who invoke disconnect() on an already-disconnected connection never receive the onDisconnected(null, 'disconnected') callback. This is an asymmetric API — the method name implies it always completes disconnection, but the callback only fires if there was an active session.
| if (session && this.session === session) { | |
| if (session && this.session === session) { | |
| this.session = null; | |
| this.onDisconnected(null, 'disconnected'); | |
| } else if (!session) { | |
| this.onDisconnected(null, 'disconnected'); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| request, | ||
| askResponse.optionId, | ||
| ); | ||
| if (!optionId) { |
There was a problem hiding this comment.
[Suggestion] Silent cancellation when resolvePermissionOptionId returns undefined.
When askResponse.optionId is provided but doesn't match any option in request.options, resolvePermissionOptionId returns undefined and the response is silently cancelled. The user selected an option, nothing happened, and there is no console warning explaining why. This will be very hard to debug in production.
| if (!optionId) { | |
| if (!optionId) { | |
| if (askResponse.optionId) { | |
| console.warn('[DaemonIdeConnection] Preferred optionId not in request options, cancelling:', | |
| askResponse.optionId); | |
| } | |
| return { outcome: { outcome: 'cancelled' } }; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| this.onEndTurn(response.stopReason); | ||
| return response; | ||
| } catch (error) { | ||
| this.onEndTurn('error'); |
There was a problem hiding this comment.
[Suggestion] sendPrompt conflates AbortError with genuine prompt failures.
When disconnect() aborts the shared eventController, the AbortSignal passed to session.prompt() triggers an AbortError. The catch block calls onEndTurn('error') without distinguishing deliberate disconnections from real failures. Callers listening to onEndTurn will see spurious error states on every clean disconnect while a prompt is in flight.
| this.onEndTurn('error'); | |
| } catch (error) { | |
| if (!(error instanceof DOMException && error.name === 'AbortError')) { | |
| this.onEndTurn('error'); | |
| } | |
| throw error; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
b3e660f to
fe40ad8
Compare
|
Generated by GPT-5 model Handled the latest valid IDE daemon adapter review comments in Fixes included:
Local validation run after the change:
CI status at this check: latest Classify PR and Lint are green; macOS/Ubuntu/Windows tests and CodeQL are still running for the new head. |
| : undefined; | ||
| } | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Critical: Dead fallback code in resolvePermissionOptionId
The auto-select fallback chain at lines 612–619 (allow_once kind → proceed_once id → namespaced proceed_once → options[0]) is unreachable dead code.
Why: Both callers (line 507 for AskUserQuestion, line 537 for regular permission) guard with !response.optionId || isCancelledOption(response.optionId) before calling resolvePermissionOptionId, ensuring preferredOptionId is always truthy when it reaches this function. The if (preferredOptionId) guard at line 606 then always short-circuits, making the fallback block dead code.
This also masks a behavioral divergence from AcpConnection: when preferredOptionId is truthy but absent from request.options, AcpConnection (line 426) falls through to its auto-select chain, while DaemonIdeConnection returns undefined (cancelling the request). This divergence is undocumented.
Suggested fix: Remove the dead fallback chain and add a comment documenting the intentional difference:
private resolvePermissionOptionId(
request: RequestPermissionRequest,
preferredOptionId?: string,
): string | undefined {
const options = Array.isArray(request.options) ? request.options : [];
if (options.length === 0) {
return undefined;
}
// Unlike AcpConnection, we cancel on mismatch rather than falling
// through to a daemon-controlled auto-select chain.
if (preferredOptionId) {
return options.some((option) => option.optionId === preferredOptionId)
? preferredOptionId
: undefined;
}
return undefined;
}
| () => {}; | ||
|
|
||
| async connect(options: DaemonIdeConnectionOptions): Promise<void> { | ||
| while (this.connectPromise) { |
There was a problem hiding this comment.
Suggestion: Untested AbortError path in sendPrompt
The isAbortError guard at line 184 is the primary cancellation behavior — it determines whether onEndTurn('error') is called during user abort. The test suite has no test for session.prompt() rejecting with an AbortError. A regression here would cause spurious end-turn callbacks during abort flows.
Suggested fix: Add a test:
it('does not call onEndTurn when the prompt is aborted', async () => {
const events = new EventQueue();
const session = createFakeSession(events);
const connection = new DaemonIdeConnection();
const onEndTurn = vi.fn();
connection.onEndTurn = onEndTurn;
await connection.connect({
baseUrl: 'http://127.0.0.1:4170',
sessionFactory: vi.fn().mockResolvedValue(session),
});
const abortError = new DOMException('Aborted', 'AbortError');
session.prompt.mockRejectedValueOnce(abortError);
await expect(connection.sendPrompt('abort me')).rejects.toThrow();
expect(onEndTurn).not.toHaveBeenCalled();
events.close();
await connection.disconnect();
});
| return undefined; | ||
| } | ||
|
|
||
| if (preferredOptionId) { |
There was a problem hiding this comment.
Critical: Behavioral divergence from AcpConnection
When preferredOptionId is truthy but absent from request.options:
AcpConnection(line 426):if (preferredOptionId && options.some(...))→ falls through to auto-select fallback chainDaemonIdeConnection(line 606):if (preferredOptionId)→ returnsundefined(cancels)
The refactoring that restructured this guard produced a subtly different behavior. Since both callers currently guard against empty optionId (making this path dead — see comment on line 612), the divergence has no runtime impact today. However, if a future caller invokes resolvePermissionOptionId(request) without a preferred option (e.g. for a default-allow flow), the AcpConnection version would auto-select while the DaemonIdeConnection version would cancel — a latent correctness risk.
Suggested fix: After removing the dead fallback (see comment on line 612), add a cross-reference comment: // Keep semantics in sync with AcpConnection.resolvePermissionOptionId. If the divergence is intentional, document it explicitly.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
本地完整跑了一遍（19/19 vitest + lint + prettier + build 都过，check-types 重建 core dist 后过；CI 全绿）。13 轮 review 的 critical 我对照 HEAD fe40ad852 源码逐条对账，真有运行时风险的全部修了（详见前次评论里我贴的 inline 表）。
下面 2 处是 spike → wire-up 之前应该收掉的清洁性问题，不是运行时 bug，但 merge 前希望清掉，避免给未来的 wire-up PR 留坑：
resolvePermissionOptionId的 fallback chain 当前无 caller 走得到（dead code）- SDK static import + extension 入口未引 + tree-shake → VSIX 是否能在 wire-up 后正确包含 SDK 仍未实测
这 2 处收完就 approve。
| // explicit allow_once kind first, then tolerate namespaced proceed_once. | ||
| options.find((option) => option.optionId?.includes('proceed_once')) | ||
| ?.optionId || | ||
| options[0]?.optionId |
There was a problem hiding this comment.
[Cleanup before wire-up] resolvePermissionOptionId 的 fallback chain（allow_once kind → proceed_once id → namespaced proceed_once → options[0]）当前无 caller 走得到。
两个 caller（resolvePermissionResponse 走常规权限路径 L532-537，以及 AskUserQuestion 路径 L507）都遵循同一个模式：先在外层 !response.optionId || isCancelledOption(...) 早退到 cancelled，否则把 response.optionId 作为 preferredOptionId 传进来。preferredOptionId 一旦非空，下面 L606-609 的 strict-match 分支直接 return，永远不会落到 L612-619 的 fallback chain。
之前 round-9/10 review 标的 "fallback chain 未测"——根因不是缺测试，是这段代码根本走不到。要么补一个真用得上的 caller（比如某个流场景允许 caller 不指定优先选项、让 bridge 智能选），要么直接删掉这段 fallback 减少未来维护误读。两条都行，我倾向删（之前 PR 4203 同款判断逻辑你也走的极简）。
| import { | ||
| DaemonClient, | ||
| DaemonSessionClient as SdkDaemonSessionClient, | ||
| } from '@qwen-code/sdk'; |
There was a problem hiding this comment.
[Cleanup before wire-up] @qwen-code/sdk 是 static import，esbuild.js 又只把 vscode 列 external，理论上 esbuild 会把 SDK inline 进 extension.cjs。但当前 extension.cjs 里 grep -c "DaemonIdeConnection\|DaemonClient\|SdkDaemonSessionClient" 都是 0 —— 整个适配器被 tree-shake 掉了（因为 extension 入口还没引）。
这本身合理（PR 描述里写了 default-off + 未 wire 到 QwenAgentManager），但意味着 VSIX 里有没有 SDK 是未经实测的。等做 wire-up 那个 PR 时，本仓库用 vsce package --no-dependencies 打包，.vscodeignore 只放行 dist/，所以 SDK 必须靠 esbuild 完整 inline 进 bundle 才能跑得起来。
建议这个 PR 顺便加一个 build-time assertion 或一个空的 void DaemonIdeConnection; 之类的 retain hint 强制 esbuild 把它打进 extension.cjs，这样 bundle 步骤就能在本 PR 验证；或者明确在 PR 描述里把"wire-up 时验证 VSIX 含 SDK"列成 follow-up 必检项。
|
Processed the latest review items in #4199 with a narrow adapter-only change:
Validation run locally:
I did not change the previously discussed false-positive areas: Generated by GPT-5 Codex |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Re-approving after 1d9360f405 fix(ide): tighten daemon adapter review gaps. After the 40-review-deep iteration on this PR, my local re-audit lines up with the commit message — the substantive Critical concerns I raised across the 5/16–5/17 rounds are addressed in the current head.
Verified addressed in current code
- Concurrent
connect()mutex (was: no serialization → two SSE pumps + lost session ref):connectPromisefield +while (this.connectPromise)serialization loop at top ofconnect(). validateDaemonBaseUrlhostname restriction (was: any hostname acceptable → SSRF / data exfil to arbitrary daemon): now restricts to loopback explicitly with "Daemon baseUrl must target a loopback address, got X" —daemonIdeConnection.ts:117.onDisconnectedcallback never fired ondisconnect()(was: webview UI never learned the session ended): now invoked at the two cleanup paths (disconnect()and the pump's terminal cleanup).sendPrompt()had no try/catch (was: prompt failure left webview spinning forever): now wrapssession.prompt(), callsonEndTurn('error')on non-abort errors.- Permission TOCTOU race + abort deadlock (was:
await onPermissionRequestcouldn't be interrupted by SSE abort): newresolvePermissionResponseUntilAbort(request, signal)handles signal-aware cancellation, and the post-awaitif (this.session !== session)guard drops responses for stale sessions. if (this.session === session)always-false guard (was: pump cleanup could clobber newer pump's state): replaced withpumpGenerationgenerational guard.- Unknown event types silently discarded (was: zero observability for daemon changes): now
console.debug('[DaemonIdeConnection] Ignoring daemon event', ...)records the event type. - Malformed permission data silently dropped: now
console.warn('[DaemonIdeConnection] Malformed permission request data').
Local verification on 1d9360f405
- Anti-corruption checks (3850/4253 reflex):
daemonIdeConnection.ts631 lines,daemonIdeConnection.test.ts917 lines,extension.ts422 lines. All LF, all/**license headers, 0 mojibake. npx vitest run src/services/daemonIdeConnection.test.ts: 19 tests passed (PR body lists 5; the test suite grew 4× through review iteration).- Remote CI: Lint / CodeQL / Classify PR / Test mac · ubuntu SUCCESS; Test windows IN_PROGRESS at time of approval.
Note on the open review threads
GitHub still shows ~30 unresolved Critical-tagged threads. Most of those are stale — the same underlying concern (e.g. the deadlock pattern, the missing mutex) appears across 5+ review rounds, each anchored to a slightly different line as the code shifted. Could you batch-resolve those at your end so the UI state matches reality? I've covered the substantive ones above; if I missed anything that's still actually live in the current code, please leave a fresh comment so it stands out.
Scope reminders
- This is the spike adapter, default-off, not wired into
QwenAgentManageror webview. - File scope is contained to
vscode-ide-companion/src/services/daemonIdeConnection.{ts,test.ts}plus a 4-lineextension.tsregistration. - Subsequent PRs need to handle: feature flag wiring, settings/env resolution, webview flow, IDE file-service boundary, reverse RPC for editor/browser/clipboard — all called out as out-of-scope in the PR description, which I'm holding you to.
LGTM.
4 participants
Summary
DaemonIdeConnectionspike in the VS Code extension host, plus unit coverage for daemon session creation, SSE event consumption, prompt forwarding, permission responses, cancel, model switch, and session death handling.httpServer + SSEwithout moving the default VS Code ACP subprocess path yet.QwenAgentManager.Validation
session_update,permission_request, andsession_diedframes.npm run buildpassed; it still reports the pre-existingsrc/utils/editorGroupUtils.tscurly warning and stale Browserslist data.cd packages/vscode-ide-companion && npx vitest run src/services/daemonIdeConnection.test.ts.src/services/daemonIdeConnection.test.ts (5 tests)andTest Files 1 passed.Scope / Risk
qwen servesmoke, noQwenAgentManagerswitch, no session list/load/delete parity, no daemon set-mode route, no IDE file-service boundary, and no reverse RPC for local editor/browser/clipboard.Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Related to #3803, #4175, and #4201.
Supersedes the docs-only IDE draft #4198 with a locally verifiable adapter spike.