Repository navigation
fix(anthropic): flush parser-completed tool calls before turn finish - #17750
Conversation
Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test fc024e7 |
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Track completion and flush calls in order lib/llm/src/protocols/anthropic/stream_converter.rs, lib/llm/tests/anthropic_http_replay.rs, docs/fern/pages/reference/components/frontend-configuration.mdx |
The converter records completion per call and emits only an ordered prefix of ready calls during completion-driven drains. Finish-triggered and final drains retain fallback behavior. Tests cover completion ordering, delayed identity, fragmented output, finish handling, and single-tool mode. The documentation describes the release conditions and fallback. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Severity of issue fixed: Medium
Merge Risk: 
Minimal · up to fc024
-
The reviewed streaming change has no identified issue requiring resolution before merge.
Pre-merge checks: 
4 · 
1
-
Failed checks: 1 warning Passed checks: 4 passed-
Check name Status Explanation Title check The title clearly identifies the main change: flushing parser-completed Anthropic tool calls before turn completion. Linked Issues check Directly linked issue #17748is open and active, so its coding requirements apply. Requirements and evidence at head fc024e7:
1. [#17748] Consume existing per-call completion evidence. `process_chun…Out of Scope Changes check The whole-PR diff from merge base cb5f9a6 to head fc024e7 touches three code or doc areas, and each connects to #17748:
-lib/llm/src/protocols/anthropic/stream_converter.rs: the core change. It …Description check The pull request description includes all required sections: Overview, Key Changes, Validation, Issues and Ongoing Work, issue closure, and the checklist. It provides detailed scope, behavior, testing… Full details: Linked Issues check
-
Explanation
Directly linked issue
#17748is open and active, so its coding requirements apply. Requirements and evidence at head fc024e7:- [
#17748] Consume existing per-call completion evidence.process_chunknow readschunk.tool_call_completionfor choice 0 and setsis_completeandneeds_flushon the indexed, un-emitted call.record_tool_callreturnsis_complete, so late identity can trigger a flush. Verified in the diff. - [
#17748] Release only an ordered, complete prefix. Indrain_buffered_tool_events, whenonly_completedis true, the loopbreaks at the first call that is not complete, not emit-ready, or not flush-pending. Calls are not released out of order. Verified in the diff. - [
#17748] Require complete identity and valid object input before early emission. Theonly_completedpath requiresis_emit_ready()(backend_id and name non-empty). The repair path is gated byis_final, so repaired input is never used early. Invalid or incomplete JSON breaks the loop.serde_json::Mapparsing rejects non-object input. Verified in the diff. - [
#17748] Keep finish/EOF fallback when no evidence exists.only_completedis false oncetool_flush_requestedis set, and the non-completed path is unchanged. The legacy-fallback test asserts this. - [
#17748] Preserve single-tool limits, per-fragment usage, and duplicate suppression. Thesingle_tool && has_emittedbreak, per-fragmentusage_snapshotretention, andhas_emittedskip are retained. The test asserts usage values[1, 3, 2]. - [
#17748] Tests.parser_completion_waits_for_ordered_valid_calls_and_late_identitycovers metadata-only completion, repeated completion, late identity, and an incomplete-argument barrier.parser_completion_keeps_legacy_fallback_and_single_tool_limitcovers the parameterless single-tool case and the no-evidence fallback.completed_tool_block_precedes_gated_tail(parameterized) uses the existing gate in the HTTP replay harness, with a two-second drain timeout, to check that the firstcontent_block_stoparrives before the tail is released.
Unverified points: I did not run tests. I did not inspect the
parallel-tools.ssefixture contents. The issue asks for error-path coverage. The diff adds no explicit terminal-error test. The incomplete-argument path is covered, and error handling is unchanged by this diff, so this is not treated as an unmet requirement. Completion evidence for a tool index with no recorded state is ignored. The parser emits call chunks before completion, so this is not shown to be a gap. - [
Full details: Out of Scope Changes check
-
Explanation
The whole-PR diff from merge base cb5f9a6 to head fc024e7 touches three code or doc areas, and each connects to
#17748:lib/llm/src/protocols/anthropic/stream_converter.rs: the core change. It addsis_completestate, completion handling inprocess_chunk, the orderedonly_completedbarrier, and the new tests. All hunks serve the early per-call release objective or its tests.lib/llm/tests/anthropic_http_replay.rs: replaces the single usage-tail test with the parameterizedcompleted_tool_block_precedes_gated_tail, adds a per-tool completion case, and adds a two-second drain timeout. These changes support the HTTP replay validation that the issue requests.docs/fern/pages/reference/components/frontend-configuration.mdx: adds five lines that document the Messages streaming release rule. Documentation of the changed behavior is a legitimate supporting change.
No change in the reviewed diff addresses unrelated behavior. No request or worker wire fields, public declarations, or dependencies changed, per the summaries. The
ignored_fileslist is empty.
Full details: Description check
-
Explanation
The pull request description includes all required sections: Overview, Key Changes, Validation, Issues and Ongoing Work, issue closure, and the checklist. It provides detailed scope, behavior, testing results, limitations, and related work.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 6 functions across 2 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
✅ Dynamo PR CI passed — run 41372691351 (attempt 1) on
|
Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test c0f905d |
Overview
Messages streaming waits for the turn-level tool finish before publishing any tool blocks, even when the parser has already completed the first call. Release an ordered prefix of parser-completed calls so clients can consume the first tool while later calls are still generating.
Key Changes
61 added nonblank, noncomment production lines in the Messages converter, excluding tests and documentation.
{}can release early. Warn once per call if non-empty arguments arrive after emission or finalization, without logging their contents. No request or worker wire fields change.Validation
A request such as the following can produce two
read_filecalls:{ "model": "MODEL", "max_tokens": 128, "stream": true, "tools": [{ "name": "read_file", "input_schema": { "type": "object", "properties": {"path": {"type": "string"}}, "required": ["path"] } }], "messages": [{"role": "user", "content": "Read /a and /b"}] }complete=true; second call and turn finish are held behind a gatecompleted_tool_block_precedes_gated_tailobservescontent_block_stopthrough a realreqweststream before releasing the existing semaphore gate, then receives the second block without duplicationparser_completion_waits_for_ordered_valid_calls_and_late_identitypreserves call order through metadata-only and repeated completion, invalid-argument barriers, late identity, and the original fragment usage snapshotsdisable_parallel_tool_use=trueparser_completion_keeps_legacy_fallback_and_single_tool_limitwaits until EOF and preserves the single-tool limit{}and discard later argumentsv2_parser_arguments_survive_anthropic_conversion_at_every_splitfeeds native and named guided calls throughapply_streamand the Messages converter for 12 families at every UTF-8 split; the old condition fails this regressionThe unchanged-parent converter fails the first-call-before-finish assertion in a source-isolated replay; the modified converter emits the first call's three SSE events immediately. Native CPU validation passes all 118 Anthropic unit tests and 15 HTTP replay tests. The complete HTTP replay suite takes 2.78 seconds on macOS arm64 with default features disabled.
The HTTP backend supplies completion evidence explicitly to qualify early HTTP delivery. The new CPU adapter-to-converter regression additionally covers real V2 parser output for Qwen3, Kimi K2/K3, DeepSeek V4/V4.1, Gemma4, GLM4.7, Muse Glimmer/Spark, Nemotron v3, GPT-OSS, and Beam. Early release requires V2 completion evidence; V1 paths retain the fallback. This does not qualify live-model serving. GPU validation and latency/throughput benchmarking remain unrun; no speedup percentage is claimed.
Issues and Ongoing Work
Closes #17748.
tool_call_completion, Anthropic completed tools, andstream_converterfound no implementation of this Messages behavior. fix(preprocessor): finalize every reasoning parser at end of stream #14934 changes reasoning-parser EOF finalization, not the Messages converter.Pull Request Checklist
/pr-checklistskill, including independent design, testing, and code-quality/performance reviews.Summary by CodeRabbit