Repository navigation
Conversation
…n cancel - polling no longer truncates a growing artifact: snapshots are reconciled against what was already absorbed instead of being treated as final chunks - a cancelled task raises A2ATaskCancelledError with the partial text - the server closes the streamed text artifact with last_chunk=True - only the final turn's text is returned across tool round trips - reconnect recovers text written while disconnected from the task snapshot - polling emits a chunk for an answer carried only in the completion message
Lancetnik
left a comment
There was a problem hiding this comment.
AI-assisted review by Codex. The three correctness findings below were reproduced through the public Agent.ask() API with scripted HTTPX responses, using both the PR head and its base (0796ee4).
Standards
Two nonblocking observations:
- test/a2a/test_conformance.py:133–156 adds _ChunkingConfig/_ChunkingClient although test/AGENTS.md requires TestConfig for mocked LLM responses. TestConfig already accepts chunk events followed by a final ModelResponse; the history-dependent tool-turn double has a separate reason to exist.
- New nested subscriber collectors in test_conformance.py:74,164,266 and test_empty_text_chunks.py:31 conflict with the root AGENTS.md ban on runtime nested functions. The testing guidelines themselves contain similar examples, so this is a low-priority consistency issue rather than a merge blocker.
Spec
Three verified correctness findings are attached inline. The most serious is a regression: reusing an open text artifact ID after a client-tool round trip returns no final answer. The shorter-replacement and mixed-artifact cases remain inconsistent with the PR's promised reconciliation behavior.
Validation: the focused changed-area tests passed (49 tests). The A2A suite passed with 261 passed and 1 skipped, excluding test_card_input_modes_anthropic.py because the existing local Anthropic SDK is too old; model-dependent tests were deselected. A2A SDK 1.2.3 was installed into a temporary directory. The three public-API reproductions still fail at the reviewed head; the reused-ID case succeeds on the base.
Standards: 2 nonblocking observations (test-double convention and runtime closures). Spec: 3 findings; worst is loss of the final answer after a tool continuation.
| artifact = update.artifact | ||
| if artifact.artifact_id in state.seen_artifact_ids: | ||
| artifact_id = artifact.artifact_id | ||
| if artifact_id in state.closed_artifact_ids or artifact_id in state.prior_artifact_ids: |
There was a problem hiding this comment.
[P1] Allow actual updates to an artifact reused after a tool continuation
The new prior_artifact_ids guard drops every later update for an ID seen before INPUT_REQUIRED, including an artifact that is still open and legitimately reused in the next turn. Public-API reproduction: first SendStreamingMessage emits art='pre-' (append=False, last_chunk=False), requests step(), and enters INPUT_REQUIRED; the continuation emits the same art='end' and completes. The tool executes and both requests finish, but reply.body is None instead of 'end'. The same reproduction returns 'end' on the base commit, so this is a regression. It also contradicts the promised final-turn-only result across tool round trips. Distinguish historical snapshot replay from fresh updates rather than permanently excluding all previously touched IDs.
| return text | ||
| # Not an append: the update restates the whole artifact; an older restatement changes nothing. | ||
| old = self.text_segments[slot] | ||
| if old.startswith(text): |
There was a problem hiding this comment.
[P2] Honor explicit full replacements that shorten the text
The PR promises that a full re-send replaces the earlier artifact, but old.startswith(text) treats every shorter prefix as stale. Sending an open artifact with 'hello world', then an explicit append=False replacement with 'hello', and completing the task returns 'hello world'. A replacement with an empty string is ignored too. A prefix comparison cannot establish that an update is older; live replacements need to update the stored artifact even when they shorten or clear its contents. Please cover shortening and clearing, in addition to the existing growing-prefix replacement test.
| state.pending_calls.append(a2a_event.call) | ||
| else: | ||
| state.closed_artifact_ids.add(artifact_id) | ||
| elif first_sight or a2a_event.append: |
There was a problem hiding this comment.
[P2] Reconcile text in mixed artifacts as well as text-only artifacts
An artifact with a text part plus a URL/data part is classified as A2ATaskArtifactUpdate, so it reaches this fallback. After first sight, subsequent full snapshots (append=False) are silently skipped. I reproduced this through Agent.ask() with text+image-URL art='hello', then the same art='hello world': the reply remains 'hello'. Polling snapshots take this same path, so issue #3465's requirement to return the completed artifact's full text remains broken for mixed artifacts. Reconcile the textual portion by artifact identity here too, while keeping tool-call deduplication separate.
- Let an artifact id reused after a tool round trip carry the new answer instead of dropping every update for an id seen before. - Treat a non-append update as an authoritative replacement, including one that shortens or clears the text, and reconcile the text of mixed artifacts by id too. - When the task finishes while the stream is down, read it back with GetTask instead of failing on the refused SubscribeToTask. - Document the reconnect behaviour and the server's closing chunk.
There was a problem hiding this comment.
AI-assisted review generated with Codex. Re-reviewed the same head; the findings below were checked against executable reproductions. The originally reported test-collector issue is withdrawn, and the cancellation observation is now a nonblocking documentation/completeness note.
Reviewed commit
b65e1f36a1acdd43bf42c440a95fe6a0be13a864against base024006bf553be93bb174cb786edbfd7fae3774cc. The head has not changed since the original submission of this review.Standards
No confirmed findings. The original P3 about nested test collectors was withdrawn because the more specific
test/AGENTS.mdGOOD example endorses that pattern.Spec
Three confirmed P2 regressions remain, detailed inline: an authoritative replacement can lose its prefix; the final turn’s chunk can disappear; a later mixed-artifact replacement can lose a newly introduced tool call.
The external-cancellation observation is a nonblocking P3 concerning the scope of the new documentation’s closing promise. Missing
last_chunkon cancellation is pre-existing behavior, and this review does not establish it as a protocol violation.Validation: the original unchanged PR A2A suite passed (
272 passed, 1 skipped) with A2A SDK 1.2.3 and Anthropic SDK 1.15.0 in an isolated dependency directory. On re-review, the five targeted regression checks again failed on this head and passed against the base implementation with the same test harness. An additional public-APIAgent.ask()check confirmed that the mixed-artifact scenario requests human input instead of executing the tool; it fails on this head and passes on the base. The real executor cancellation reproduction again confirmed the absence of a closing artifact update.Standards: 0 confirmed findings. Spec: 3 P2 and 1 nonblocking P3; the most consequential issue is silent truncation of the final answer.
| """Collect chunk events; the executor holds its remaining output until the first one lands.""" | ||
| chunks: list[str] = [] | ||
|
|
||
| @stream.where(ModelMessageChunk).subscribe |
There was a problem hiding this comment.
AI-assisted correction after re-review.
Withdrawn: runtime nested test collectors
I missed the more specific test/AGENTS.md guidance, whose GOOD example explicitly uses a nested async callback and nested decorated tools inside a test. Given that test-specific example, treating this pattern as a documented-standard violation was not justified. No change is requested for this comment.
| fresh = content[len(known) :] | ||
| else: | ||
| fresh = "" | ||
| mine = content[len(prior) :] if content.startswith(prior) else content |
There was a problem hiding this comment.
[P2] Preserve the full replacement when the new turn shares the previous text prefix
The existing reused-artifact scenario becomes corrupt if the continuation answer happens to start with the previous turn’s text: first emit
art="pre-", request a client tool, then sendart="pre-final"withappend=Falseand complete. The reply is"final", although this update replaces the artifact with"pre-final". I reproduced this through an actual Agent tool round trip in both streaming and polling; both checks pass on the base commit. Removing any matchingpriorprefix conflates a cumulative replay with an authoritative new answer. Please track turn/replacement boundaries rather than infer them solely from text prefix equality.
| elif content.startswith(known): | ||
| fresh = content[len(known) :] | ||
| else: | ||
| fresh = "" |
There was a problem hiding this comment.
[P2] Emit the final answer chunk when an artifact ID is reused across turns
With the same tool-round-trip setup, emit
art="pre-"before the tool and replace it withart="end"on continuation. The finalModelResponsecontains"end", but the entireModelMessageChunksequence is only["pre-"]: comparing against the previous turn’s content makesfreshempty. Consumers rendering chunk events never receive the final answer. This reproduces in streaming and polling, and the corresponding checks pass on the base commit. Newly arrived text for the current turn must not be suppressed merely because the artifact existed in an earlier turn.
| text = "".join(part.text for part in artifact.parts) | ||
| if fresh := state.absorb_text(artifact_id, text, a2a_event.append): | ||
| await context.send(ModelMessageChunk(fresh)) | ||
| if (first_sight and artifact_id not in state.prior_text) or a2a_event.append: |
There was a problem hiding this comment.
[P2] Detect new tool calls in replacements of mixed artifacts
Send an open artifact containing text plus an image, then replace the same ID with text plus a
tool-call+jsonpart (append=False,last_chunk=True), followed byINPUT_REQUIRED. At the second updatefirst_sightis false, so this condition drops the newly introduced tool call. I reproduced an emptypending_callslist atINPUT_REQUIRED; the outer loop consequently requests human input instead of executing the tool. The same test delivers the call on the base commit. Please deduplicate tool calls by their own identity while still discovering calls first introduced by later full artifact updates.
| timestamp: str | None = None, | ||
| metadata: dict[str, Any] | None = None, | ||
| ) -> None: | ||
| if state != TaskState.TASK_STATE_WORKING and self._text_pieces and not self._closed: |
There was a problem hiding this comment.
AI-assisted clarification after re-review; downgraded from P2 to a nonblocking P3.
[P3] Clarify whether artifact closing covers external cancellation
AgentExecutor.cancel()constructs a plainTaskUpdaterat line 254, so the new_ClosingTaskUpdaterdoes not participate. Running the real executor until its first text chunk, then callingcancel()and cancelling the execution coroutine in SDK order, still producesSUBMITTED → WORKING → artifact(last_chunk=False) → CANCELED, without a closing artifact update.This is a mismatch with the new documentation’s broad promise to close text before the task leaves
working. It is pre-existing cancellation behavior, however, andCANCELEDalready reports that execution stopped; this observation does not establish a protocol violation or a new functional regression. Either narrow the documentation to the transitions covered by this updater or cover external cancellation too. This comment should not block the PR.
Lancetnik
left a comment
There was a problem hiding this comment.
AI-assisted re-review with Codex, based on executable reproductions of the changed behavior.
Reviewed
c53c874379173cc354d4d42ac436b59d72ff2ce6against base938f671b7f511698eb2defea60439473197162e0, with particular attention to changes sinceb65e1f3.The previous findings about prefix truncation, missing final chunks, newly introduced calls in mixed replacements, and external-cancellation closing are fixed in the checked scenarios. Six independent regression checks now pass, and the real executor cancellation reproduction now emits the closing artifact update.
Standards
One nonblocking P3: the remaining custom
_ToolTurnsConfig/_ToolTurnsClientLLM mock conflicts with the documentedTestConfigconvention (inline).Spec
One P2: the first pure tool-call chunk unconditionally closes an artifact even when
last_chunk=False, causing later calls in that artifact to disappear in streaming mode (inline).Validation: the original A2A suite passes (
280 passed, 1 skipped, A2A SDK 1.2.3 and Anthropic SDK 1.15.0). An additional publicAgent.ask()reproduction sends two calls as two chunks of the same artifact: streaming executes only the first on this head, whereas streaming executes both on the base commit. Polling on this head executes both.Standards: 1 nonblocking P3 (custom LLM mock). Spec: 1 P2 (silently dropped tool call).
| elif isinstance(a2a_event, A2AToolCallArtifact): | ||
| state.pending_calls.append(a2a_event.call) | ||
| state.add_call(artifact_id, a2a_event.call) | ||
| state.closed_artifact_ids.add(artifact_id) |
There was a problem hiding this comment.
[P2] Keep a tool-call artifact open until its last chunk
This marks every
A2AToolCallArtifactas closed regardless oflast_chunk. A server can sendcall-0in artifactcallswithappend=False, last_chunk=False, thencall-1in the same artifact withappend=True, last_chunk=True, followed byINPUT_REQUIRED. The second update is discarded by the closed-ID guard, so an actualAgent.ask()executes onlycall-0. I reproduced both tool executions in streaming mode on the base commit; on this head streaming runs one, while polling runs both because the cumulative artifact contains two Parts and takes the general branch.seen_callsalready deduplicates individual calls. Please close the artifact only whenlast_chunksays it is complete, and cover a growing artifact whose first chunk is a single tool-call Part.
| assert await _text_artifact_updates(client) == [] | ||
|
|
||
|
|
||
| class _ToolTurnsConfig(ModelConfig): |
There was a problem hiding this comment.
[P3, nonblocking] Use the shared TestConfig for the scripted model turns
_ToolTurnsConfig/_ToolTurnsClientmock LLM responses in agent-based tests, whereastest/AGENTS.mdexplicitly requiresag2.testing.TestConfigfor that purpose.TestConfignow supports intermediateModelMessageChunkevents followed byToolCallEventorModelResponse, as the streaming test at lines 208–209 already demonstrates. The pre-/mid-/end script can use that shared seam instead of the two custom mock classes. This is a test-convention note, not a functional blocker.
A tool-call artifact was closed on its first update regardless of last_chunk, so later calls streamed into the same artifact were dropped and only the first client tool ran in streaming mode. A resubscribe snapshot taken while the task is INPUT_REQUIRED now surfaces its tool calls and ends the turn as input-required, instead of hiding the calls and finishing with the partial text. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Lancetnik
left a comment
There was a problem hiding this comment.
AI-assisted re-review with Codex; the findings below were checked against the current code and executable public-API reproductions.
Reviewed
f4059bc6298752eb0af0fc4985c41525fefde6ddagainst base938f671b7f511698eb2defea60439473197162e0, including the changes sincec53c874.The previous P2 about prematurely closing a growing tool-call artifact is fixed: the independent reproduction now executes both calls in streaming and polling. The earlier prefix, chunk-delivery, and mixed-replacement reproductions also continue to pass.
Standards
The previously reported nonblocking P3 about
_ToolTurnsConfig/_ToolTurnsClientinstead ofTestConfigremains attest/a2a/test_conformance.py:232–259. No new standards findings; no duplicate inline comment added.Spec
One P2 remains in the newly promised cancellation behavior: a streaming response containing a canceled Task snapshot and no separate status update returns normally instead of raising
A2ATaskCancelledError. This is missing coverage of the new cancellation contract, not a claim that the base branch already raised for this case. Details and the protocol reference are inline.Validation: the unchanged A2A test suite passes (
283 passed, 1 skipped, A2A SDK 1.2.3 and Anthropic SDK 1.15.0). Eight independent prior regression checks pass. An additionalAgent.ask()test using the real A2A SDK and an HTTPX transport that emits a single canceled Task snapshot fails withDID NOT RAISE A2ATaskCancelledError.Standards: 1 retained nonblocking P3 (custom LLM mock). Spec: 1 P2 (a canceled streaming task can still look like a successful reply).
| # A resubscribe opens with the task as it stands, covering what streamed while disconnected, | ||
| # including a request for input that was made while the stream was down. | ||
| status = a2a_event.task.status | ||
| input_required = status.state == TaskState.TASK_STATE_INPUT_REQUIRED |
There was a problem hiding this comment.
[P2] Classify cancellation carried by the Task snapshot too
This branch reads the snapshot status but only acts on
INPUT_REQUIRED. IfSendStreamingMessageemits a singleTaskwhose status isTASK_STATE_CANCELED, with an artifact containingpartial, and then closes,Agent.ask()returns a normal reply containingpartial;state.terminal_taskis never set andA2ATaskCancelledErroris not raised. I reproduced this through the real SDK and publicAgent.ask()path.This stream shape is allowed by A2A §3.1.2: the initial Task may be followed by no update events, so a separate
TaskStatusUpdateEventcannot be assumed. Please apply the terminal-state classification to the snapshot status as well and preserve the Task, including its partial artifacts, on the exception. This is an uncovered case of the PR’s new cancellation contract; the base branch also returned normally for this case.
A2AServer builds a fresh LLM client per request, so a single TestConfig replays its first turn on every client-tool continuation. Add ag2.a2a.testing.ToolRoundsConfig, which picks the round from the history (one Turn or TestConfig per tool round), and drop the hand-written LLM mocks in test/a2a (StatelessScript, RecordingConfig, _ToolTurnsConfig, _ChunkingScript, VariablesRecordingConfig). Add ModelCall.messages so TrackingConfig can assert the full history sent to the model. Also replace private transport imports with a2a SDK APIs and tidy assertions per test/CLAUDE.md
…on a no-op Client: - Read the outcome from any task snapshot, polled or streamed: a task that arrives already finished, or a stream that closes on a final snapshot, now raises its cancelled/failed/rejected error or returns the completed task's status message. A status message on a working task is a progress note and stays out of the reply. - If the stream closes before the task has settled (a cancel recorded after the server stopped streaming), poll get_task until it does instead of returning the partial text as the answer. - A terminal error's task carries every received artifact under its own id rather than one merged "partial" artifact. - A non-append update that restates the previous turn's text unchanged adds nothing, live or from a snapshot. Server: - The turn's completion and a cancel share one TaskUpdater. Whichever claims a terminal state first wins; a later transition is a no-op instead of a RuntimeError the SDK would turn into FAILED written over COMPLETED (a2aproject/a2a-python#1397). - Lifecycle events are mirrored onto the AG2 stream from the updater, so a cancel that loses no longer emits a stray COMPLETED. - CancelTask answers TaskNotCancelableError when the task finished some other way while the cancel was in flight, matching the SDK's cross-replica path (spec §3.1.5). - Cancelling a task with no turn running (waiting for a client tool) leaves closing it to the SDK, which marks it CANCELED.
Lancetnik
left a comment
There was a problem hiding this comment.
AI-assisted review with Codex of commit 8244adb. Both inline findings were independently reproduced against this checkout.
Standards: no material violations found.
Behavior/spec: two P2 findings, covering an unchanged reconnect snapshot after a client-tool continuation and cancellation racing the closing artifact on INPUT_REQUIRED.
Validation: 309 A2A tests passed (295 in the sandbox, then the remaining 14 socket-dependent tests passed outside it); 618 tests covering A2UI and the changed testing utilities also passed. The two additional reproductions expose cases outside the passing suite.
| elif given is None and content == prior: | ||
| reply_text = "" | ||
| else: | ||
| reply_text = content |
There was a problem hiding this comment.
[P2] Preserve the current turn's suffix when a reconnect snapshot repeats already received content
This branch reintroduces text consumed by an earlier tool turn. Reproduced through the HTTPX/SSE transport: turn 1 emits artifact art = "pre-" and requests a client tool; the continuation appends "end" to the same artifact, then the connection drops. SubscribeToTask returns an unchanged snapshot with art = "pre-end", followed by COMPLETED. The append correctly sets this turn's reply to "end", but this snapshot falls through to reply_text = content, and the final reply becomes "pre-end".
This is distinct from the documented polling limitation: every append update was received and the snapshot adds no content. Please preserve the existing per-turn text when the snapshot equals the full content already received, while retaining the intended handling of actual replacements. A regression test combining a reused artifact id, a client-tool continuation, and reconnect would cover this.
| if self._text_pieces: | ||
| await self.add_artifact( | ||
| parts=[Part(text="")], | ||
| artifact_id=self._text_artifact_id, | ||
| append=True, | ||
| last_chunk=True, | ||
| ) | ||
| await super().update_status(state, message, timestamp, metadata) |
There was a problem hiding this comment.
[P2] Synchronize cancellation with the nonterminal transition across the closing-artifact await
The initial ended check does not cover a cancellation that lands while INPUT_REQUIRED is awaiting its closing artifact. Reproduced via AgentExecutor.execute() using TestConfig to emit text followed by a ClientTool call and an EventQueue that delays enqueueing the closing text artifact: while requires_input() is suspended here, executor.cancel() sets the updater's terminal state to CANCELED. Releasing the enqueue then resumes the original transition at super().update_status(INPUT_REQUIRED, ...), which raises RuntimeError: Task t is already in a terminal state. That exception escapes execute(); the delayed closing artifact also arrives after CANCELED in this reproduction.
The existing tests cover competing terminal transitions, but INPUT_REQUIRED never claims ended, so it can still reach the base updater after cancellation won. Please serialize the transition/close sequence, or otherwise revalidate it after suspension, and add this race to the tests.
…rom snapshots - executor: cancel that lands while a non-working transition is closing the streamed text now waits for it under a lock, instead of ending the task underneath it (previously raised "already in a terminal state"). - client: after a tool round trip appends to an existing artifact, a resubscribe snapshot restating it no longer leaks the earlier turn's text into the reply.
Lancetnik
left a comment
There was a problem hiding this comment.
AI-assisted re-review with Codex of da6a15ede9e781f8a5ea21ef10afbd5c60b60aea against base 59bba8fddd1b23a29ec50e19dd0a3a6c9845467b.
Standards
No findings. The previous TestConfig note is resolved: the custom model mocks were replaced by ToolRoundsConfig using TestConfig.
Spec
One P2 regression in the new transition lock: awaiting lifecycle subscribers while holding that lock deadlocks a subscriber that cancels the same task on INPUT_REQUIRED. Details are inline.
The previously reported canceled-snapshot case and the earlier artifact/tool-call reproductions now pass. The append/reconnect boundary is also preserved in the checked reproduction.
Validation: 930 tests passed across test/a2a, test/a2ui, test/agent/test_test_config.py, and test/test_plugin/test_invocation.py. Ten independent previous regression checks also passed. The additional lifecycle-cancellation test times out on this head with the callback waiting on asyncio.Lock.acquire(); the same test passes on both the PR base
59bba8fand the previously reviewedf4059bc. It uses a normal agent tool to install a public lifecycle subscription and calls the executor’s public cancel method; no internal object is patched.Standards: 0 findings. Spec: 1 P2, a cancellation deadlock in a lifecycle subscriber.
| last_chunk=True, | ||
| ) | ||
| await super().update_status(state, message, timestamp, metadata) | ||
| await self._lifecycle.send( |
There was a problem hiding this comment.
[P2] Release the transition lock before awaiting lifecycle subscribers
_lifecycle.send()waits for user subscriber code while_transitionis still held. Registering a subscriber from an ordinary tool viacontext.stream.where(A2ATaskStatusUpdate).subscribe(...), then having it awaitexecutor.cancel()for the same task onINPUT_REQUIRED, now deadlocks: the transition waits for the observer, whilecancel()callsupdater.cancel()and waits to acquire the transition lock again. The task cannot finish or complete the requested cancellation.I reproduced the timeout using a real Agent/AgentExecutor and a recording EventQueue, without patching internals. The identical test passes on the PR base
59bba8fand onf4059bc; the failing stack here is the observer’s cancel call blocked in asyncio.Lock.acquire(). Please preserve serialization of the wire transition while invoking lifecycle subscribers outside the transition lock, and cover cancellation requested by an observer.
A lifecycle subscriber can await cancellation of the task whose status it is handling. Holding the transition lock while dispatching that subscriber makes cancellation wait on its own caller. Keep text-artifact closing and wire status updates serialized, and release the lock before notifying lifecycle subscribers. Cover cancellation from INPUT_REQUIRED and the no-op cancellation of a COMPLETED task. Validation: 932 tests passed across A2A, A2UI, and the affected testing utilities; Ruff checks and formatting passed. Both new regression cases failed with TimeoutError before the fix, and the original reproduction passes after it.
Queue lifecycle notifications in wire order and drain them with one shielded dispatcher. Reentrant transitions enqueue their notifications instead of recursively notifying later subscribers. Account for an eager task factory completing dispatch before task registration returns. Serialize artifact delivery with closing transitions. Finish each queue write and its text/client-call bookkeeping before propagating caller cancellation: SDK legacy queues may publish to their primary consumer before a tapped child accepts the event. Keep queued terminal lifecycle notifications even when a producer or subscriber is canceled or fails. Add regression coverage for multiple observers, nested cancellation, producer cancellation, callback failures, eager dispatch, in-flight artifacts, partial SDK queue writes, and recovery after sender cancellation. Validation: 947 affected tests passed, including 21 cancel-race cases.
…olate observer failures Client: - a COMPLETED status.message that repeats the artifact text is not counted twice - a tool call written while a stream was down is recovered from the resubscribe snapshot - tool calls on a task that did not wait for input are logged and ignored instead of sent to a finished task; the INPUT_REQUIRED-only snapshot gate is removed, so streaming and polling treat calls the same - rename the client's _TERMINAL_STATES to _SETTLED_STATES (it includes interrupted states) Server: - a failing lifecycle observer is logged, not raised, so it cannot turn a cancel or a completed task into FAILED - observer subscriptions live as long as the updater can still deliver Docs: ADR 0027 (cancel racing completion keeps the first terminal state); note the cross-replica and parked-task limits of cancel and observers.
Human-input continuations reuse the drive state. Reset the previous INPUT_REQUIRED finish reason and artifact closure flags before the next response, so completed tasks cannot surface client calls and later turns can reuse artifact ids for valid tool requests or replies. Close request observer registrations after execution, outstanding writes, and deferred lifecycle notifications finish. Cover registration/startup failures and cancelled producers, and isolate observer cleanup failures from the task's already committed terminal state. Validation: 971 related A2A, A2UI, test-config and plugin tests passed; regressions were reproduced before fixing and original public repros now pass. Changed-file Ruff checks and repository mypy (255 files) passed. Repository lint also reports two existing reimplemented-builtin errors in unchanged ag2/events/conditions.py. Implemented and reviewed with AI assistance.
Codecov Report❌ Patch coverage is
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Why are these changes needed?
The A2A client could silently return a wrong answer, and a cancelled task had no typed error. Found by auditing
ag2.a2aagainst the A2A v1.0 spec.streaming=Falsekept only the prefix it saw on its first poll. Live against OpenAI through an AG2 server: streaming returned 149 characters, polling returned1, 2, 3,(8).TASK_STATE_CANCELEDfell through as a normal reply.What changed
The reply is the text of the final turn only across tool round trips. Following the A2A spec, a task snapshot is the current full content of each artifact and an update without
appendreplaces the artifact: even when the new text is shorter or empty, and even when a later turn reuses an artifact id and begins with the earlier turn's text. A snapshot that is exactly the text an earlier turn consumed is a replay and adds nothing. Each artifact appears once, in order of first appearance, text-only and mixed artifacts alike.Beyond the tickets, both found by the live checks and both covered by tests:
SubscribeToTaskopens with the task as it stands. The streaming path ignored that snapshot, so text produced while disconnected was lost. This is also present onmain(reproduced there).COMPLETEDstatus.messageproduced noModelMessageChunk, unlike streaming.Found by review and by live checks against a non-AG2 server's conventions (reused artifact ids, mixed artifacts), all covered by tests:
(artifact_id, call_id)instead of by "first sight of the artifact".ModelMessageChunkconsumers, and a continuation that began with the earlier turn's text lost that prefix.AgentExecutor.cancel()bypassed the closinglast_chunk=Trueupdate.Failed / rejected / auth-required behaviour is unchanged.
Evidence
Live,
gpt-4o-mini, direct provider access, real sockets:1, 2, 3,)status.messageA2ATaskCancelledError, partial equals received chunksMatrix run against the AG2 server and against an independent server (
@a2a-js/sdk1.3.0 on Node, separate process, streaming a plain OpenAI call, no AG2 conventions), over JSON-RPC, REST and gRPC, streaming and polling:status.message, artifact re-sent in full): answer correct, chunks equal the answer;A second live pass, same model, direct, real sockets, against the AG2 server and a custom executor that writes real OpenAI text into artifacts the way a non-AG2 server may (the AG2 server uses a fresh artifact id per turn, so it never exercises these). Run on the code before the review fixes and on this branch:
' Done.'(prefix lost)HumanInputNotProvidedError, tool never runok(streaming and polling)A2ATaskCancelledError, one closinglast_chunk=True, nothing after it, partial equals received chunksNot checked live: Go / Java / .NET servers. Client-tool turns are an AG2 extension, so they were exercised against the AG2 server and the custom executor above, not a third-party SDK. Live scripts are not part of the suite.
Tests (
uv run pytest test/a2a -q): 280 passed, 1 skipped. New ones, roughly:Cancel racing the task's own completion
The SDK does not order
AgentExecutor.cancel()againstexecute().ActiveTask.cancelonly checks that its consumer has not yet processed a terminal event, so a cancel can arrive after the turn has already enqueuedCOMPLETED. In a2a-sdk 1.2.1 through 1.3.0, ifcancel()raises there,_mark_task_as_failedwritesFAILEDover theCOMPLETEDthe client already received on the stream. We reported this upstream with a self-contained repro: a2aproject/a2a-python#1397.Until the SDK fixes this, the AG2 server follows the spec itself:
COMPLETEDis "a terminal state" (§4.1.3), and a streaming client is told it is final because the stream "MUST close when the task reaches a terminal state" (§3.1.2). The turn andcancel()share one_ClosingTaskUpdater. Whichever claims a terminal state first wins, and any later transition does nothing. The baseTaskUpdaterraisesRuntimeErrorinstead, and that is what triggers the SDK'sFAILEDoverwrite. AG2-side observers get lifecycle events from the same updater, so they never see theCOMPLETEDthat lost.cancel()with no turn running writes nothing. The turn may just have ended with its terminal event not yet consumed. The SDK re-reads the task aftercancel()returns and closes it if it is still open.CancelTaskanswersTaskNotCancelableErrorwhen the cancel loses. §3.1.5 is ambiguous here: "success is not guaranteed … returns its updated state" againstTaskNotCancelableErrorfor a task "already completed, failed, or canceled". The SDK answers both ways: its local path returns theCOMPLETEDtask as a successful cancel, and its cross-replica path (_cancel_remote) raisesTaskNotCancelableError. We picked the error, so a client never reads a successful cancel that cancelled nothing, and both paths now give the same answer. That question is also open in [Bug]: CancelTask racing the task's own completion overwrites COMPLETED with FAILED a2aproject/a2a-python#1397. If upstream settles it the other way, only_RequestHandlerintransports/_common.pyneeds to change.Covered by
test/a2a/test_cancel_race.py(cancel lands after completion → staysCOMPLETED; cancel with no running turn → writes nothing; turn finishing after a cancel → wire and observers see onlyCANCELED) and by theTaskNotCancelableErrorcase intest_conformance.py.Merge Danger
Door: two-way. No schema, storage or wire-format change; revert restores the old behaviour.
Blast Radius: A2A client and server users.
A2ATaskCancelledError(subclass ofA2ATaskTerminalError), is raised where a cancelled task used to return normally. Code that catchesA2ATaskTerminalErrornow also sees cancellations.parse_task_artifactno longer setslast_chunk=Trueon polled artifacts.last_chunk=Trueupdate on its text artifact; clients that did not wait for it are unaffected.A2ATaskCancelledErroris importable fromag2.a2a.errorsonly, like the other task errors; it is not re-exported fromag2.a2a.append. A server that appends to the same artifact id across tool round trips is read per spec as that artifact's current full content, so a polling client returns both turns' text for it; streaming honoursappendand returns only the new text. The AG2 server is unaffected (fresh artifact id per turn).CancelTaskon a task that finished while the cancel was in flight now raisesTaskNotCancelableErrorinstead of returning the finished task (see above).AgentExecutornow keeps a per-instance map of running tasks' updaters (removed when the turn ends) socancel()can close the text artifact.Related issue number
Closes #3465
Upstream: a2aproject/a2a-python#1397 (cancel racing completion overwrites
COMPLETEDwithFAILED)Follow-up to #3451 (
fix(a2a): classify empty text artifacts by field presence)Checks
AI assistance
🤖 Generated with Claude Code