agentHost: render Claude SDK-initiated reply turns - #336753
Aleksandr Logunov (Longarithm) wants to merge 8 commits into
Conversation
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: TylerLeonhardtMatched files:
|
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved routing, attribution, cancellation, and usage-accounting issues can produce missing or incorrect turn state.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds protocol turns for Claude SDK-initiated replies after the prompt queue drains.
Changes:
- Adopts unsolicited top-level SDK output as a new hidden turn.
- Completes or cancels adopted turns across lifecycle paths.
- Adds reducer-backed queue and agent tests.
File summaries
| File | Description |
|---|---|
| src/vs/platform/agentHost/test/node/claudePromptQueue.test.ts | Tests unsolicited-turn queue behavior. |
| src/vs/platform/agentHost/test/node/claudeAgent.test.ts | Tests resumed replies and cancellation paths. |
| src/vs/platform/agentHost/node/claude/claudeSdkPipeline.ts | Creates and manages SDK-initiated turns. |
| src/vs/platform/agentHost/node/claude/claudePromptQueue.ts | Supports non-prompt in-flight turn entries. |
Review details
Suppressed comments (1)
src/vs/platform/agentHost/node/claude/claudeSdkPipeline.ts:565
- The new
dispose()behavior is not exercised by the added lifecycle tests: they cover abort and stream failure, but never dispose while an SDK-initiated turn is active. Add a regression test that tears down an active adopted turn and verifies one cancellation, no later completion, and a closed reducer state.
override dispose(): void {
this._cancelSdkTurn();
super.dispose();
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| private _adoptSdkTurn(message: SDKMessage): void { | ||
| if (!this._queue.isEmpty || (message.type !== 'assistant' && message.type !== 'stream_event') || message.parent_tool_use_id !== null) { |
There was a problem hiding this comment.
The current guard only handles an empty prompt queue; it does not distinguish a delayed SDK continuation from output for a newly admitted user prompt. I checked that QueueDrainContribution can admit another turn once the active turn ends. I have not reproduced the claimed SDK ordering yet, so I cannot claim this case is fixed. It needs coverage with queued-message admission and evidence of the SDK's prompt/output ordering before choosing an ownership rule; assigning a fresh turn to every top-level message would also misattribute normal user replies.
There was a problem hiding this comment.
Still open; no ownership change for this case. I checked the SDK declarations: user_message_uuid can correlate the first reply frame with a client prompt, but it is optional and absent on later frames, synthetic turns and older producers. Absence alone would not safely identify a separate continuation. I have not obtained a live trace proving the claimed queued-prompt ordering, and a fabricated fake-SDK sequence would not establish that ordering. The PR description now explicitly limits the fix to idle-parent continuations and records this unresolved case. The other review fixes pass 366 tests plus targeted type checking and hygiene.
|
@microsoft-github-policy-service agree |
Use agent origin for resumed replies, clear reported credit totals at turn completion, and route registered subagent content and task notifications while the parent is idle. Avoid duplicate client cancellations and cancel provider turns before session event teardown. Cover credit reports before first output, edit attribution, idle child completion, abort during rebind, stream failure and disposal.
|
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved turn-lifecycle, replay-boundary, and child file-attribution bugs can leave state inconsistent or lose attribution.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/vs/platform/agentHost/node/claude/claudePromptQueue.ts:116
- This four-sentence JSDoc exceeds the repository's 1–2 short-sentence limit for API documentation. Keep the essential queue invariant concise; the implementation already makes the sentinel details clear.
/**
* Register a turn the SDK started on its own — one the client never
* sent (e.g. the model resuming after a background task finishes).
* Seeds an in-flight entry so {@link peekParent} tags the turn's actions
* with `turnId` and {@link settleHead} fires its completion on `result`.
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Balanced
|
There was a problem hiding this comment.
🔵 Needs a closer look
Cross-cutting turn ownership, cancellation, and persistence still have unresolved correctness risks without live SDK validation.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 5
- Review effort level: Balanced
|
Fixes #332073
Problem and scope
Claude can start a parent reply after a background task finishes and the prompt queue has drained. Without a new protocol turn, the chat reducer drops that output. This PR gives idle-parent continuations their own turn and preserves that boundary on restore.
It does not resolve the separate review question about delayed output arriving after a new user prompt has already been admitted. That ownership thread remains open.
Behavior
MessageKind.Agent; the response remains eligible for file-change attribution.The message loop uses an early-return adoption helper. In-flight entries do not require a fabricated SDK user message.
Restore, fork, and truncate
The SDK does not persist result events, so its transcript alone cannot recover the live boundary. The pipeline stores the first canonical assistant UUID and the adopted protocol turn ID in the chat's metadata database. Replay uses this marker to restore the separate turn, its ID, Agent origin, and hidden request.
Boundary metadata is optional:
Fork and truncate use the same boundary map when resolving SDK anchors. Fork copies only inherited markers into the destination chat's metadata. Transcript messages without stored markers retain existing replay behavior; this does not repair old transcripts retroactively.
Related work and remaining question
Alternative to #332075: that proposal attributes output to the spawning turn; this PR starts a new turn when the queue is empty.
For the queued-user ownership race raised in review, the SDK declares an optional
user_message_uuidon the first reply frame. Its absence can also mean a later frame, a synthetic turn, or an older producer, so absence alone is not a reliable ownership rule. A live SDK ordering trace and host turn-admission coverage are still needed. The queue regression covers a different ordering: an SDK turn is adopted first, then a new user prompt is admitted before its result.Validation
687 tests pass across
claudeAgent,claudePromptQueue,claudeMapSessionEvents,claudeSdkMessageRouter,claudeSdkPipeline,claudeReplayMapper,agentSideEffects, andsessionDatabase.Regression coverage includes:
Targeted native TypeScript checking, repository hygiene, formatting, and
git diff --checkpass. The current diff is +546/-69 across 19 files, down from +555/-63 before these review fixes. Validation reused existing dependencies and Electron; no full application build or live SDK/UI reproduction was run for this upstream branch.