Skip to content

Python: Isolate Anthropic response parsing state - #8242

Closed
Eduard van Valkenburg (eavanvalkenburg) wants to merge 1 commit into
microsoft:mainfrom
eavanvalkenburg:provider-stream-isolation
Closed

Python: Isolate Anthropic response parsing state#8242
Eduard van Valkenburg (eavanvalkenburg) wants to merge 1 commit into
microsoft:mainfrom
eavanvalkenburg:provider-stream-isolation

Conversation

@eavanvalkenburg

Copy link
Copy Markdown
Member

Motivation & Context

A reusable Anthropic client can serve multiple responses concurrently. Each response needs its own parsing context so streamed tool content is interpreted using the request that produced it.

Description & Review Guide

  • What are the major changes? Introduces private response-local parsing state, threads it through request preparation and response parsing, and adds deterministic concurrent-stream regressions covering local and hosted tools, overlapping call IDs, argument fragments, approval requests, and a sibling stream failure.
  • What is the impact of these changes? Concurrent streaming and non-streaming responses retain their own tool aliases, call identity, arguments, and hosted/local classification without changing public APIs.
  • What do you want reviewers to focus on? The lifetime and propagation of the response-local state and the deterministic stream interleaving in the regression tests.

Related Issue

N/A — no matching public issue was found.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and the title prefix in sync automatically.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new dataclass uses a default_factory pattern that can break or confuse static type-checking, and a small guard is needed to avoid accumulating argument fragments under an empty call-id key.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR makes Anthropic streaming/tool parsing state response-local so concurrent responses from a reusable AnthropicClient don’t leak tool aliases, call identity, or argument-fragment parsing across streams.

Changes:

  • Introduces _AnthropicRequestState and threads it through request preparation and both streaming/non-streaming response parsing paths.
  • Updates tool parsing to use request-local aliases and call/content-type tracking rather than instance attributes.
  • Adds deterministic concurrency regression tests for overlapping call IDs, hosted/local tool mixes, approval requests, and sibling-stream failures.
File summaries
File Description
python/packages/anthropic/agent_framework_anthropic/_chat_client.py Adds per-request parsing state and uses it throughout tool preparation and response/stream parsing.
python/packages/anthropic/tests/test_anthropic_client.py Updates existing parsing tests to pass request state where needed and adds concurrent streaming regression coverage.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +1607 to +1610
call_id = request_state.last_call_id_name[0] if request_state.last_call_id_name else ""
request_state.accumulated_tool_arguments[call_id] = (
request_state.accumulated_tool_arguments.get(call_id, "") + content_block.partial_json
)
Comment on lines +106 to +107
tool_name_aliases: dict[str, str] = field(default_factory=dict[str, str])
accumulated_tool_arguments: dict[str, str] = field(default_factory=dict[str, str])

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAF Automated Review — Iteration 1

Result: Findings reported
Scope: full PR (1 commit(s)): d93df81c8feb
Model: gpt-5.6-sol-fast

Overview

The review found 1 verified inline finding(s).

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.

Affected areas: python/packages/anthropic/agent_framework_anthropic/_chat_client.py

else:
call_id = self._last_call_id_name[0] if self._last_call_id_name else ""
call_id = request_state.last_call_id_name[0] if request_state.last_call_id_name else ""
request_state.accumulated_tool_arguments[call_id] = (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each local-tool input_json_delta at _chat_client.py:1608-1609 rebuilds the full accumulated string despite having no production reader, making endpoint-controlled fragmentation O(n²) and potentially blocking the event loop; any safe fix must preserve response-local, complete per-call arguments asserted by the regression test.

@eavanvalkenburg

Copy link
Copy Markdown
Member Author

Closing as a duplicate of #8237.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants