feat(acp): expose tool-call preparation lifecycle - #6819
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: Real feature request with a well-specified linked issue (#6775). The observability gap during tool argument generation is documented with comparisons to OpenCode and Codex, both of which already expose tool-call progress before complete arguments are available. Not a theoretical concern — it's a user-facing stall in ACP clients when large tool inputs are being generated. Direction: Aligned with project scope. ACP lifecycle improvements are squarely within Size: Core paths touched (Anthropic provider, OpenAI converter, GeminiChat, Session). Breakdown:
729 production lines crosses the 500-line threshold — flagging for maintainer awareness. This is a Approach: The design feels right. WeakMap-based metadata store keeps the response schema clean and avoids serialization issues. One observation: the four stream loops in Session.ts now carry near-identical try/catch/finally blocks for preparation cleanup. Not a concern for this PR, but something to watch for future consolidation if a fifth stream path appears. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 关联 issue #6775 描述清晰，有与 OpenCode 和 Codex 的对比。ACP 客户端在工具参数生成期间无可观察活动是真实的用户体验问题，不是理论性担忧。 方向： 与项目范围一致。ACP 生命周期改进属于 规模： 触及核心路径（Anthropic provider、OpenAI converter、GeminiChat、Session）。生产代码约 729 行，测试约 1507 行，设计文档 220 行。729 行生产代码超过 500 行阈值——标记维护者关注。作为 方案： 设计合理。WeakMap 元数据存储保持响应 schema 干净，避免序列化问题。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewReviewed the full diff against my independent proposal. The PR's approach matches what I would have designed: WeakMap metadata store, per-stream preparation tracker, cleanup on retry/fallback/cancel, and ID normalization to correlate preparation with the complete call. No critical blockers found. The implementation is well-structured and the abstractions are justified by the lifecycle complexity. A few observations:
Test ResultsRan all 8 focused test suites from the PR's test plan: All preparation lifecycle paths verified: pending emission, resolution correlation, discard on retry/fallback/cancel/stream-error, TodoWrite suppression, ID normalization across turns, and preparation-only chunks not suppressing transport retry or model fallback. Real-Scenario TestingNot applicable for this PR. The feature exposes preparation events through the ACP protocol ( 中文说明代码审查将完整 diff 与独立方案进行对比。PR 的方法与我的设计一致：WeakMap 元数据存储、每流 preparation tracker、重试/回退/取消时清理、以及 ID 归一化确保 preparation 与完整调用关联。 未发现关键阻塞问题。 实现结构良好，抽象因生命周期复杂度而合理。 几个观察：
测试结果运行了 PR 测试计划中的全部 8 个定向测试套件：861 条全部通过，0 失败。所有 preparation 生命周期路径均已验证。 真实场景测试不适用。本功能通过 ACP 协议暴露 preparation 事件（ — Qwen Code · qwen3.7-max |
|
Stepping back to look at the whole picture. This PR solves a real, documented problem: ACP clients see no activity while tool arguments are being generated, making healthy requests appear stalled. The linked issue (#6775) is well-specified, the design doc is thorough, and the implementation matches what I would have proposed independently. The code is straightforward — WeakMap metadata store, a tracker class with clear lifecycle semantics, and cleanup hooks at every stream boundary. The The version bumps across all packages are unrelated housekeeping but harmless. Why I'm not auto-approving: 729 production lines touch core provider paths (Anthropic streaming, OpenAI converter, GeminiChat, ACP Session). The code quality is high and I found no bugs, but this is a fork PR crossing the 500-line core threshold. The scale warrants a maintainer's architectural judgment — particularly on whether the The direction is right, the execution is clean, the tests are comprehensive. This needs a maintainer's sign-off on the architectural choices, not a bot's rubber stamp. 中文说明退一步看全局。 这个 PR 解决了真实的、有文档记录的问题：ACP 客户端在工具参数生成期间看不到任何活动，导致正常请求看起来像卡住了。关联 issue #6775 描述清晰，设计文档详尽，实现与我的独立方案一致。 代码简洁——WeakMap 元数据存储、生命周期语义清晰的 tracker 类、每个流边界的清理钩子。 版本升级是无关的维护操作，无害。 未自动批准的原因： 729 行生产代码触及核心 provider 路径（Anthropic 流、OpenAI 转换器、GeminiChat、ACP Session）。代码质量高且未发现 bug，但这是来自 fork 的 PR，超过 500 行核心阈值。规模需要维护者的架构判断——特别是 方向正确，执行干净，测试全面。需要维护者对架构选择签字确认。 — Qwen Code · qwen3.7-max |
|
⏸️ Deferring to maintainer — this is a fork PR with 729 production lines touching core provider paths (Anthropic streaming, OpenAI converter, GeminiChat, ACP Session). The code quality is high, all 861 focused tests pass, and I found no bugs or blockers. However, the scale crosses the 500-line core threshold, so it needs a maintainer's architectural judgment rather than automated approval. Key questions for the maintainer:
Direction, execution, and test coverage all look solid. Needs a human call on the architecture. — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
added
status/in-review
| resp.type === StreamEventType.RETRY || | ||
| resp.type === StreamEventType.MODEL_FALLBACK | ||
| ) { | ||
| await preparationTracker.discard(true); |
There was a problem hiding this comment.
[Critical] The inline await preparationTracker.discard(true) during RETRY/MODEL_FALLBACK is not wrapped in a try-catch, unlike finalizeToolCallPreparations in the finally block which was designed to protect cleanup errors from masking the primary stream outcome. If emitPreparationDiscarded throws, the error propagates through catch { streamFailed = true; throw } and terminates the model stream — classifying an ACP cleanup failure as a stream failure on a healthy model response. This pattern exists at all 4 stream paths (lines 2049, 2391, 3329, 3661).
| await preparationTracker.discard(true); | |
| try { | |
| await preparationTracker.discard(true); | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error); | |
| debugLogger.warn( | |
| `Failed to discard preparations during ${resp.type}; continuing stream: ${message}`, | |
| ); | |
| } | |
| functionCalls.length = 0; |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 8513f68. All four RETRY/MODEL_FALLBACK paths now call the existing protected finalization helper with include-resolved cleanup enabled. The helper performs discard(true), catches ACP update failures, logs a warning, and lets the healthy stream continue. Added parameterized Session regressions for both RETRY and MODEL_FALLBACK cleanup failures.
| } | ||
| } | ||
| if ( | ||
| resp.type === StreamEventType.RETRY || |
There was a problem hiding this comment.
[Suggestion] RETRY events now clear functionCalls alongside MODEL_FALLBACK at all 4 stream paths. This is a correct bugfix (prevents stale function calls from the abandoned attempt from being executed), but it extends beyond the stated preparation-tracking scope and is not called out in the PR description or design doc. Consider explicitly noting this behavioral change.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Documented in both the design document and PR description. RETRY now clears complete function calls collected from the abandoned attempt across all four ACP stream paths, matching the existing MODEL_FALLBACK behavior and preventing stale calls from executing with the replacement attempt.
|
|
||
| const emitted = await this.emitter.emitStart({ | ||
| callId: preparation.callId, | ||
| toolName: preparation.toolName, |
There was a problem hiding this comment.
[Suggestion] When emitStart returns false (suppressed, e.g. for TodoWrite), the callId is not added to pending. On the next observe() with the same callId, emitStart is invoked again redundantly — including its resolveToolMetadata registry lookup. Consider maintaining a separate suppressed: Set<string> to short-circuit repeated calls:
| toolName: preparation.toolName, | |
| if (this.pending.has(callId) || this.suppressed.has(callId)) continue; |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 8513f68. The tracker now keeps a separate suppressed call-ID set and skips both pending and suppressed IDs on subsequent observations. The tracker regression now verifies emitStart is called only once for a suppressed preparation.
| // Try to find which tool call this belongs to based on the index | ||
| // Look for an existing tool call at this index that's not complete | ||
| if (this.buffers.has(index)) { | ||
| if (this.pendingIndexRemaps.has(index)) { |
There was a problem hiding this comment.
[Suggestion] The pendingIndexRemaps continuation path short-circuits routing without checking whether the remapped slot already has a complete buffer. The non-remapped path (lines 136-152) performs a JSON.parse completeness check and calls findMostRecentIncompleteIndex(). Add a matching completeness guard here to prevent potential buffer corruption if a provider emits complete JSON followed by more fragments before the ID arrives.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 8513f68. The pending remap path now checks depth and JSON completeness before routing a continuation to the remapped slot, then falls back to findMostRecentIncompleteIndex when that slot is complete. Added a regression proving a later fragment cannot corrupt the completed remapped buffer.
| // The preparation-only chunk precedes the complete tool call chunk. | ||
| expect(chunks[3]?.functionCalls).toBeUndefined(); | ||
| expect(chunks[4]?.candidates?.[0]?.content?.parts?.[0]).toEqual({ | ||
| functionCall: { id: 't1', name: 'tool', args: { x: 1 } }, |
There was a problem hiding this comment.
[Suggestion] No test covers concurrent multi-tool preparation from a single Anthropic stream (two content_block_start events for distinct tool_use blocks at different indices). Add a test that verifies two preparation chunks are emitted with correct {callId, toolName} metadata and precede their respective function call chunks.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added a concurrent Anthropic multi-tool stream test. It covers two tool_use starts at distinct indices, verifies both preparation metadata entries, and verifies each preparation chunk precedes its corresponding complete function call chunk.
| return normalized; | ||
| } | ||
|
|
||
| export function reserveModelToolCallId( |
There was a problem hiding this comment.
[Suggestion] reserveModelToolCallId has zero dedicated unit tests. Coverage is only indirect through geminiChat integration tests. Add unit tests covering: (a) fresh rawId reservation, (b) idempotency on repeated calls, (c) normalizeModelToolCallIds with a reservedIds map redirecting a colliding rawId to the reserved suffixed ID.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added dedicated reserveModelToolCallId coverage for a fresh reservation, repeated idempotent reservation, and normalization of a colliding raw ID through the reserved suffixed ID.
| }); | ||
| }); | ||
|
|
||
| it('passes resolved paths to read_many_files tool', async () => { |
There was a problem hiding this comment.
[Suggestion] The preparation lifecycle suite has no Session-level test for TodoWrite preparation suppression. The design doc states "TodoWrite keeps its current special handling and does not emit a tool card," but the only suppression test exists at the tracker level (tool-call-preparation-tracker.test.ts), not at the Session integration level. Add a test that streams a preparation response with toolName: 'TodoWrite' and asserts no tool_call update is emitted.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added a Session-level TodoWrite suppression test. It streams TodoWrite preparation metadata and verifies that neither tool_call nor tool_call_update is emitted for that call ID.
| ): void { | ||
| if (preparations.length > 0) { | ||
| preparationsByResponse.set(response, preparations); | ||
| } |
There was a problem hiding this comment.
[Suggestion] setToolCallPreparations silently no-ops when preparations.length === 0, so calling setToolCallPreparations(response, []) does NOT clear previously attached metadata from the WeakMap. Either unconditionally call set (letting the empty array be stored), or document the no-op-on-empty contract in a JSDoc comment to avoid surprising future callers.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 8513f68 by unconditionally storing the supplied preparation list in the WeakMap. Added a regression that attaches metadata, sets an empty list, and verifies the previous metadata is cleared.
e44b2f8 to
8513f68
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
| throw error; | ||
| } finally { | ||
| try { | ||
| await finalizeToolCallPreparations( |
There was a problem hiding this comment.
[Critical] When the stream succeeds (streamFailed=false), finalizeToolCallPreparations runs with preservePrimaryOutcome=false, so any ACP cleanup error from tracker.discard() propagates into the outer catch block. That catch was designed for stream-level failures and unconditionally fires StopFailure hooks (lines ~2118-2136), producing a false failure signal to hook consumers when the stream actually succeeded. If the cleanup error happens to carry status 429, it would also be misclassified as RequestError("Rate limit exceeded"). The same pattern exists in the Stop Hook continuation path at line 2441.
| await finalizeToolCallPreparations( | |
| try { | |
| await finalizeToolCallPreparations( | |
| preparationTracker, | |
| streamFailed || pendingSend.signal.aborted, | |
| 'main prompt', | |
| ); | |
| } catch (cleanupError) { | |
| if (streamFailed || pendingSend.signal.aborted) { | |
| debugLogger?.warn('preparation cleanup failed during stream error', cleanupError); | |
| } else { | |
| debugLogger?.warn('preparation cleanup failed after successful stream', cleanupError); | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 924f473. Preparation cleanup failures are now logged as warnings and never replace the model stream outcome, including successful main and Stop Hook continuation streams. The regression test now verifies that a normally completed prompt still returns end_turn when the ACP discard update fails.
| @@ -94,6 +94,7 @@ export class ToolCallEmitter extends BaseEmitter { | |||
| rawInput: params.args ?? {}, | |||
There was a problem hiding this comment.
[Suggestion] When a preparation is emitted via observe() (sends sessionUpdate: 'tool_call' with phase: 'preparing'), and then the full function call arrives and execution proceeds, emitStart() is called again for the same callId — sending a second sessionUpdate: 'tool_call' creation frame (not tool_call_update). The design doc specifies preparing → in_progress as a lifecycle transition, but both frames use the creation event type. Whether ACP clients treat the second tool_call as an upsert or display a duplicate card depends on client implementation. Consider either suppressing the second emitStart for already-prepared calls, or transitioning via tool_call_update instead.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 924f473. ToolCallEmitter now tracks successfully emitted preparing call IDs and emits tool_call_update when execution starts for the same ID. Calls without a preparation frame still use the existing tool_call creation event. Added emitter and Session coverage for the transition.
| 'main prompt', | ||
| ); | ||
| } finally { | ||
| await messageDisplay?.finish(); |
There was a problem hiding this comment.
[Suggestion] messageDisplay?.finish() is called here in the inner finally, and again in the outer finally at line 2156. The method is idempotent (guarded by a finished flag), so there is no functional bug, but the duplication is dead code that obscures which finally "owns" the cleanup contract. Same pattern at lines 2447 vs 2486 (Stop Hook). Consider removing one of the two calls.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 924f473. The inner finish calls were removed from the main prompt and Stop Hook continuation paths; the outer finally blocks remain the single owners of MessageDisplay finalization.
| }; | ||
| } | ||
|
|
||
| setToolCallPreparations(response, preparations); |
There was a problem hiding this comment.
[Suggestion] setToolCallPreparations(response, preparations) is called unconditionally for every chunk, storing an empty array in the WeakMap for the common case where preparations is empty. The Anthropic generator only calls setToolCallPreparations when preparations are non-empty (line 1089). Adding a guard would align the two paths:
| setToolCallPreparations(response, preparations); | |
| if (preparations.length > 0) { | |
| setToolCallPreparations(response, preparations); | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 924f473. The OpenAI converter now attaches preparation metadata only when the collected preparation list is non-empty.
| // Try to find which tool call this belongs to based on the index | ||
| // Look for an existing tool call at this index that's not complete | ||
| if (this.buffers.has(index)) { | ||
| if (this.pendingIndexRemaps.has(index)) { |
There was a problem hiding this comment.
[Suggestion] When a continuation chunk finds the remapped buffer is already complete (JSON.parse succeeds), actualIndex is redirected via findMostRecentIncompleteIndex(), but the stale pendingIndexRemaps entry for this provider index is never deleted. Subsequent continuations at the same index re-enter this branch, re-parse the complete buffer, and re-redirect — wasting cycles on repeated JSON.parse calls. Add this.pendingIndexRemaps.delete(index) after the successful parse:
| if (this.pendingIndexRemaps.has(index)) { | |
| if (this.pendingIndexRemaps.has(index)) { | |
| actualIndex = this.pendingIndexRemaps.get(index)!; | |
| const existingBuffer = this.buffers.get(actualIndex)!; | |
| const existingDepth = this.depths.get(actualIndex)!; | |
| if (existingDepth === 0 && existingBuffer.trim()) { | |
| try { | |
| JSON.parse(existingBuffer); | |
| this.pendingIndexRemaps.delete(index); | |
| actualIndex = this.findMostRecentIncompleteIndex(); | |
| } catch { | |
| // The remapped buffer is still incomplete; append below. | |
| } | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
I am not deleting this mapping because it is also the pending association for providers that stream a complete name/arguments payload before the stable ID. Removing it after JSON becomes complete would cause a later ID to be assigned to another slot and detach it from the buffered arguments. 924f473 adds the regression test associates a late stable ID with its completed remapped slot; the existing continuation test still verifies that later fragments do not corrupt that completed slot.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| }); | ||
|
|
||
| expect(execute).toHaveBeenCalledOnce(); | ||
| expect(debugLoggerWarnSpy).toHaveBeenCalledWith( |
There was a problem hiding this comment.
[Suggestion] The four cleanup-failure tests in this suite assert on debugLoggerWarnSpy without calling mockClear() first. The established pattern elsewhere in this file (18 call sites at lines 4857, 4973, 5073, etc.) is to call debugLoggerWarnSpy.mockClear() before asserting. Without clearing, toHaveBeenCalledWith(expect.stringContaining('cleanup failed')) matches against accumulated calls from the entire suite run, not just the current test. The unique warning string makes a false positive unlikely today, but these assertions would silently pass even if the production code path were removed, as long as an earlier test logged a matching warning. Add debugLoggerWarnSpy.mockClear() at the top of each of these four tests.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in a3061e1. Each of the four cleanup-failure tests now clears debugLoggerWarnSpy before exercising its own production path.
Why: ACP clients receive no signal while providers stream tool arguments, making long calls appear stalled and delaying tool-identity policy decisions. What: - attach transient preparation metadata for Anthropic and OpenAI-compatible streams - emit correlated ACP pending, execution, and discarded lifecycle updates - preserve normalized call IDs across partial chunks and provider ID reuse - clear abandoned retry calls and keep cleanup failures from terminating healthy retry/fallback streams - deduplicate suppressed preparations and protect completed remapped parser buffers - cover multi-tool Anthropic streams, ID reservation, TodoWrite suppression, retry cleanup, cancellation, and stream failure Impact: The metadata is additive and consumed only by ACP. It exposes no partial arguments, is not persisted to conversation history, and does not move permissions, hooks, scheduling, or execution ahead of complete function calls. Tests: - Core provider and stream suites: 649 passed - ACP lifecycle suites: 316 passed - npm run build - npm run typecheck - npm run lint:ci - changed-file Prettier and git diff checks Refs: QwenLM#6775
Why: - ACP cleanup failures must not convert a successful model stream into a failed prompt. - A prepared tool call must be updated in place when execution starts instead of creating a second card. What: - Preserve the primary stream outcome when preparation cleanup fails and remove duplicate message display finalization. - Track prepared call IDs so execution starts use tool_call_update, guard empty preparation metadata, and cover late stable IDs. Impact: - Ordinary tool calls keep their existing tool_call start frame. - Streaming parser production behavior is unchanged.
overrides参数在createPreparationResponse中被声明但从未使用——所有11个调用点仅传递callId且toolName. 该as GenerateContentResponse强制类型转换会绕过对始终为空对象的结构化类型检查。 Co-authored-by: qwen-code-ci-bot <qwen-code-ci@service.alibaba.com>
3a54b18 to
85f56c5
Compare
| function createPreparationResponse( | ||
| callId: string, | ||
| toolName: string, | ||
| const response = {} as GenerateContentResponse; |
There was a problem hiding this comment.
[Critical] createPreparationResponse has a syntax error — const declaration inside the parameter list is invalid TypeScript, and the body references overrides which is not declared. This appears to be a merge/rebase artifact. The build fails with TS1359: Identifier expected and all tests in the tool preparation stream lifecycle describe block cannot run.
| const response = {} as GenerateContentResponse; | |
| function createPreparationResponse( | |
| callId: string, | |
| toolName: string, | |
| ): GenerateContentResponse { | |
| const response = {} as GenerateContentResponse; | |
| core.setToolCallPreparations(response, [{ callId, toolName }]); | |
| return response; | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in a3061e1. createPreparationResponse has a valid two-parameter signature and initializes response inside the function body. The focused suite and full build now compile successfully.
| params.subagentMeta, | ||
| ); | ||
| const updatesPreparedCall = | ||
| params.phase !== 'preparing' && |
There was a problem hiding this comment.
[Suggestion] emitStart has no self-guard against duplicate preparation frames. If called twice with phase: 'preparing' for the same callId, updatesPreparedCall is false both times (the set add is idempotent), so two tool_call frames are emitted. The ToolCallPreparationTracker.observe() guards this upstream today, but the emitter's public API has no defense-in-depth. Consider an early-return when phase === 'preparing' && this.preparedCallIds.has(callId).
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in a3061e1. emitStart now returns false before metadata resolution when the same call ID already has a preparing frame. Added a regression test verifying that only one creation frame is emitted.
| error: Error, | ||
| subagentMeta?: SubagentMeta, | ||
| ): Promise<void> { | ||
| this.preparedCallIds.delete(callId); |
There was a problem hiding this comment.
[Suggestion] emitError and emitResult both call this.preparedCallIds.delete(callId), but no test verifies this cleanup. If the delete were removed, a retried tool call with the same ID would silently emit tool_call_update (upgrade frame) instead of tool_call (new frame). Consider adding a test that emits a preparation, calls emitError/emitResult, then verifies a subsequent emitStart(phase:'preparing') produces a fresh tool_call frame.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Covered in a3061e1 with a parameterized result/error test. After either terminal path, reusing the ID for a new preparation emits a fresh tool_call frame.
| } | ||
|
|
||
| /** Resolves preparations once their complete function calls arrive. */ | ||
| resolve(functionCalls: readonly FunctionCall[]): void { |
There was a problem hiding this comment.
[Suggestion] resolve() guards with if (functionCall.id && this.pending.has(functionCall.id)), correctly skipping function calls with no ID or empty string. However, no test exercises this guard with id: undefined or id: ''. A regression test would lock in this defensive behavior.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Covered in a3061e1. The new tracker test verifies that both undefined and empty-string function call IDs leave the preparation unresolved and therefore discardable.
Why: - A malformed test helper prevented the preparation lifecycle suite from compiling. - Duplicate preparing frames and state cleanup need direct regression coverage. What: - Repair the preparation response helper and isolate cleanup warning assertions. - Suppress duplicate preparing frames and cover terminal cleanup plus missing tool call IDs. Impact: - Normal preparation and execution transitions remain unchanged. - Repeated preparation frames for the same call ID are now ignored.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
Resolve the OpenAI streaming conflicts by preserving upstream invalid-stream and nameless-call validation while retaining preparation metadata, collision remapping, and late stable-ID association.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM — reviewed all 10 chunks across the full diff. All 7 acceptance criteria from #6775 verified. Build passes, 986 focused tests pass. All 3 prior Critical blockers resolved. No new Critical findings.
— qwen3.7-max via Qwen Code /review
✅ Local verification report — tool-call preparation lifecycle (maintainer merge reference)Built this PR from source and exercised it end-to-end on macOS at head What was run against a real build:
1. Focused test suites — green on macOSThe author validated on Windows only (macOS/Linux marked
2. Cross-layer E2E — real converter → real ACP wireA harness feeds an OpenAI-compatible stream (stable
3. Built-binary
|
| # | 证据 | 结果 |
|---|---|---|
| 1 | 全部 24 个改动文件的定向测试套件（macOS，即被标记 |
986 通过（core 661 + cli 325） |
| 2 | 跨层 E2E：真实 OpenAI 转换器 → 真实 ACP emitter/tracker | 4/4 场景复现 wire 帧 |
| 3 | 完整 qwen --acp 二进制往返，经过真实 @agentclientprotocol/sdk |
通过（happy path） |
| 4 | main 与本 PR 的前后对比（同一套 harness） |
preparing 帧确为新增；旧行为保持不变 |
1. 定向测试套件 —— macOS 全绿
作者仅在 Windows 上验证（macOS/Linux 标记
packages/core：661 测试 / 7 套件；packages/cli：325 测试 / 3 个 ACP 套件；Node 22.23.1；Darwin 24.6。
2. 跨层 E2E —— 真实转换器 → 真实 ACP wire
harness 向真实的 OpenAIContentConverter + StreamingToolCallParser 输入一个 OpenAI 兼容流（先给出稳定的 id+name，参数放在后续块 —— 正是触发该生命周期的形态），再把产生的 chunk 按 Session.ts 处理 ACP turn 的逐块循环，喂给真实的 ToolCallEmitter + ToolCallPreparationTracker。捕获到的真实 wire 帧（见上方“e2e scenarios”截图）：
- Happy path：
tool_call{pending, rawInput:{}, _meta.phase:"preparing"}→ 同一toolCallId通过tool_call_update{in_progress, 完整输入}升级 →completed；无preparationDiscarded。 - 被 RETRY / 取消放弃：
tool_call_update{failed, _meta.preparationDiscarded:true}，且被放弃 attempt 的functionCalls[]被清空。 - provider 先给 name 后给 id：parser 重映射槽位，准备帧只在稳定 id 到达后触发一次（不会提前/无法关联）。
3. 二进制 --acp 往返 + 前后对比
启动真实的 node packages/cli/dist/index.js --acp，指向一个会流式返回 read_file 工具调用的 fake OpenAI 兼容服务器，用真实的 ClientSideConnection 驱动，并对 main 与本 PR 跑同一套 harness（见上方“before/after”截图）：
- 变更前（
main）：客户端为该工具调用收到的第一帧是携带完整输入的tool_call{in_progress}—— 只有参数生成结束后才出现工具身份。 - 变更后（本 PR）：客户端在
id+name一旦确定时就额外收到tool_call{pending, _meta.phase:"preparing"}，随后同一个toolCallId进入既有执行生命周期。这证明这些增量_meta字段能通过真实 JSON-RPC 序列化以及ClientSideConnection边界处 SDK 的 Zod 校验。
我还确认了实验的隔离性：main 构建出的 core 确实没有 tool-call-preparation.js 模块，因此缺少 preparing 帧反映的是特性缺失，而非 harness 差异。
环境
macOS 14（Darwin 24.6.0）· Node 22.23.1 · PR head 19ad1f04 · 前置基线 main @ 42d7d28。OpenAI 兼容路径经由本地 fake 服务器 —— 无需真实模型或网络。
范围与说明
- 放弃 →
preparationDiscarded路径是在模块 + 单元层面验证的（E2E 场景 2–3 以及 325 个 cli 测试，覆盖了四类 ACP turn 路径下的 RETRY / MODEL_FALLBACK / 取消 / 流错误），未经过 spawn 出的二进制；二进制往返覆盖的是 happy path。 - Anthropic provider 的准备帧发送由通过的 core 套件覆盖（真实
AnthropicContentGenerator）；跨层 E2E 用的是 OpenAI 转换器。 - 本地未重跑：完整
npm run preflight以及 Windows/Linux 矩阵 —— 这些以 CI 为准。
3 participants
What this PR does
This PR exposes an additive ACP tool-call preparation lifecycle for Anthropic and OpenAI-compatible streaming providers. Once a provider supplies a stable tool call ID and tool name, ACP emits a pending tool call with
phase: preparing; the existing execution update later reuses the same ID with complete arguments. If an attempt is abandoned by retry, fallback, cancellation, or stream failure, ACP emits a terminal update withpreparationDiscarded: trueso clients can remove the transient state.The preparation metadata is stored out of band, contains no partial arguments, is not persisted to conversation history, and is ignored by non-ACP consumers. Tool parsing, permission checks, hooks, scheduling, and execution still wait for a complete function call.
When a provider emits
RETRY, ACP now clears complete function calls collected from the abandoned attempt, matching the existingMODEL_FALLBACKbehavior. This prevents stale calls from the failed attempt from executing alongside calls from the replacement attempt.The PR is slightly above the repository's 2,000-line review guideline because the lifecycle crosses two provider streams and four ACP turn paths, with most changed lines in regression tests. The production change remains focused on one protocol capability and cannot be split without temporarily exposing an incomplete lifecycle that leaves pending calls uncorrelated or undiscarded.
Why it's needed
Generating large structured tool arguments can take much longer than executing the tool. ACP clients currently receive no activity during that interval, so a healthy request appears stalled and clients cannot observe the tool identity until argument generation has finished. This fills that observability gap without exposing incomplete JSON or moving execution earlier.
Reviewer Test Plan
How to verify
Use an Anthropic or OpenAI-compatible streaming provider that delays tool argument chunks after sending a stable call ID and tool name. Confirm ACP first emits a pending
tool_callwith emptyrawInput, the tool name, andphase: preparing; after argument parsing completes, confirm the existing in-progress update uses the sametoolCallIdand contains the complete input. Abort or force a retry before completion and confirm the pending call receives a failed terminal update withpreparationDiscarded: true. Providers that do not expose stable identity early should retain the existing lifecycle.Evidence (Before & After)
Before: ACP emits no tool update while arguments are streaming and first reports the call only after the complete function input is available.
After: ACP emits
tool_call { status: "pending", rawInput: {}, _meta: { phase: "preparing", toolName } }, then either upgrades the same ID through the existing execution lifecycle or emitstool_call_update { status: "failed", _meta: { phase: "preparing", preparationDiscarded: true } }when the preparation is abandoned.Tested on
Environment (optional)
Windows 11, Node.js 22.16.0. Focused Core provider/stream suites passed 649 tests; focused ACP lifecycle suites passed 316 tests. Repository build, workspace typecheck, full lint, changed-file Prettier, and diff checks passed.
Risk & Scope
npm run preflightattempt exceeded the local 30-minute wrapper timeout during the full-suite phase; the focused 965 tests and the remaining build, typecheck, lint, formatting, and diff gates passed, while CI remains the source of truth for the full cross-platform matrix._metafields are additive, and providers without early stable identity behave as before.Linked Issues
Closes #6775
中文说明
本 PR 做了什么
本 PR 为 Anthropic 和 OpenAI-compatible 流式 provider 增加了增量式 ACP 工具调用准备生命周期。当 provider 给出稳定的工具调用 ID 和工具名后，ACP 会发送带有
phase: preparing的 pending 工具调用；随后现有执行更新会复用同一个 ID 并携带完整参数。如果某次尝试因重试、模型回退、取消或流错误而被放弃，ACP 会发送带有preparationDiscarded: true的终态更新，客户端可以据此移除临时状态。准备元数据采用带外存储，不包含部分参数，不写入会话历史，也不会被非 ACP 消费方处理。工具解析、权限检查、Hook、调度和执行仍然等待完整的 function call。
当 provider 发出
RETRY时，ACP 现在会清空被放弃 attempt 已收集的完整 function call，与既有MODEL_FALLBACK行为保持一致，避免失败 attempt 的旧调用与替代 attempt 的调用一起执行。本 PR 略高于仓库 2,000 行的评审建议，因为该生命周期横跨两个 provider 流和四类 ACP turn 路径，并且大多数变更行属于回归测试。生产代码仍聚焦于一个协议能力；如果拆分，会阶段性产生无法关联或无法清理 pending 调用的不完整生命周期。
为什么需要
生成大型结构化工具参数可能比执行工具本身耗时更长。ACP 客户端目前在这段时间内收不到任何活动，因此正常请求看起来像卡住了，而且客户端只有在参数生成完成后才能观察到工具身份。本改动在不暴露不完整 JSON、也不提前执行工具的前提下补齐了这段可观察性。
评审测试计划
如何验证
使用 Anthropic 或 OpenAI-compatible 流式 provider，让它在发送稳定调用 ID 和工具名后延迟发送工具参数片段。确认 ACP 首先发送一个
rawInput为空、包含工具名和phase: preparing的 pendingtool_call；参数解析完成后，确认现有 in-progress 更新使用同一个toolCallId并包含完整输入。在完成前中止请求或触发重试，确认 pending 调用收到带有preparationDiscarded: true的 failed 终态更新。无法提前提供稳定身份的 provider 应保持现有生命周期。证据（变更前后）
变更前：ACP 在参数流式生成期间不会发送工具更新，只有完整 function 输入可用后才首次报告调用。
变更后：ACP 先发送
tool_call { status: "pending", rawInput: {}, _meta: { phase: "preparing", toolName } }，随后使用同一个 ID 进入现有执行生命周期；如果准备过程被放弃，则发送tool_call_update { status: "failed", _meta: { phase: "preparing", preparationDiscarded: true } }。测试平台
环境（可选）
Windows 11，Node.js 22.16.0。Core provider/stream 定向测试通过 571 条，ACP 生命周期定向测试通过 314 条。仓库构建、workspace 类型检查、全量 lint、变更文件 Prettier 和 diff 检查均通过。
风险与范围
npm run preflight在全量测试阶段超过本地 30 分钟外层超时；定向 885 条测试以及其余 build、typecheck、lint、格式和 diff 门禁均通过，完整跨平台矩阵以 CI 为准。_meta字段均为增量字段，无法提前提供稳定身份的 provider 行为保持不变。关联 Issue
Closes #6775