Feat: port TaskResultAggregator from adk-python to the A2A agent executor - #582
Open
AmaadMartin wants to merge 5 commits into
Open
Feat: port TaskResultAggregator from adk-python to the A2A agent executor#582AmaadMartin wants to merge 5 commits into
AmaadMartin wants to merge 5 commits into
Conversation
added 5 commits
August 3, 2026 09:47
The A2A protocol's TaskState union includes 'auth-required' but the local mirror enum in a2a_event.ts omitted it. Additive; no switch over TaskState exists, and isTerminalTaskStatusUpdateEvent is deliberately unchanged since auth-required is an interrupt state, not a terminal one.
Folds the TaskStatusUpdateEvents emitted during one agent run into a single final task state using adk-python's precedence table (failed > auth-required > input-required > working), and rewrites each observed status update to 'working' in place so a terminal intermediate state cannot end event aggregation in the A2A request handler before the run is over. applyAggregatedTaskState is a module-level function rather than a private method so the resolution logic is directly unit-testable.
… task One aggregator per execute() call (a field would leak one request's task state into another's, since the executor instance is shared across requests). Matching adk-python, the pre-loop working event and the catch-branch failure event are not routed through the aggregator.
Review feedback. applyAggregatedTaskState read the aggregator's state and message and had exactly one caller, so a free function bought nothing over a method and leaked the WORKING sentinel outside the class; it is now resolveFinalStatus(fallback). Named for what it returns rather than applyTo, which would imply mutation. The executor test's Runner double moves into a stubRunner() helper, confining the partial-double cast to one site and replacing the two pre-existing occurrences of the same shape. Assertions are untouched.
…t yet Review round 2 flagged the interim cost of live-but-inert code in the executor. A reader of the processEvent call cannot otherwise tell that convertAdkEventToA2AEvent emits artifact updates exclusively, so the aggregator records nothing until the converter port lands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
N/A — no existing issue.
Problem: When an ADK agent runs behind A2A it can emit many
TaskStatusUpdateEvents during a single request. adk-python folds that stream into one final task state using a documented precedence, and rewrites the intermediate events so a mid-stream terminal state cannot end event aggregation early in the A2A request handler. adk-js has no equivalent —TaskResultAggregatorreturns zero hits acrosscore/srcanddev/src, andcore/src/a2a/agent_executor.tscontains no task-state precedence logic at all.Solution: Port
TaskResultAggregator(src/google/adk/a2a/executor/task_result_aggregator.py) tocore/src/a2a/task_result_aggregator.tsand wire it intoA2AAgentExecutor.executethe waya2a_agent_executor.py::_handle_requestwires the Python one.failed>auth-required>input-required>working. A lower-priority update never overwrites a higher-priority one already recorded.TaskStatusUpdateEventhasstatus.stateforced toworkingin place before it is forwarded; the aggregated state is tracked separately.event.finaland every other field are left untouched, matching Python.working.taskState/taskStatusMessagegetters expose the result.execute()call — a local, never a field onA2AAgentExecutor. The executor instance is shared across A2A requests (see the pre-existingagentPartialArtifactIdsMapfield), so a field would leak one request's task state into another's.resolveFinalStatus(fallback)returns the fallback object unchanged while the aggregated state isworking, so the executor's default event stream is byte-identical to before this change.core/src/a2a/a2a_event.tsgainsTaskState.AUTH_REQUIRED = 'auth-required', which the local mirror enum was missing. It is a member of@a2a-js/sdk'sTaskStateunion, so the compiler enforces that the value is backed by the package. Purely additive: noswitchoverTaskStateexists in the repo, andisTerminalTaskStatusUpdateEventis deliberately not changed —auth-requiredis an interrupt state, not a terminal one.Scope — the wiring is not yet load-bearing.
convertAdkEventToA2AEventcurrently emitsTaskArtifactUpdateEventexclusively, so todayprocessEventin the run loop observes only artifact-update events and the aggregated state always staysworking. TheresolveFinalStatuscall is therefore a no-op on every path adk-js can produce right now; it becomes load-bearing once the converter is ported to emit intermediateauth-required/input-requiredstatus updates the wayevent_converter.pydoes for EUC and long-running function calls. That converter port is a distinct, larger change and is deliberately out of scope here —convertAdkEventToA2AEventis untouched.Cross-language parity decisions (per "know which side wins a conflict"):
finalflag.processEvent/taskStatenaming,privatemembers with no_prefix, getters instead of@property, and module layout._compatindirection exists only to bridge a2a-sdk 0.3.x vs 1.x enum shapes; adk-js pins a single SDK line (@a2a-js/sdk^0.3.10incore/package.json, lockfile resolves 0.3.13), so there is nothing to bridge. Python's@a2a_experimentaldecorator is dropped because no module undercore/src/a2a/uses the repo's@experimentaldecorator.workingand a status message with parts was captured, it republishes those parts as aTaskArtifactUpdateEventand then emitscompleted. adk-js does not need it: its converter already publishes agent content as artifact updates directly, so republishing would duplicate content on the wire.resolveFinalStatusinstead defers togetFinalTaskStatusUpdate(adkEvents, context), which already derives completed / input-required / failed from the ADK event stream.Collision check (required before implementation):
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000returned 480 open PRs. Filtering titles and branch names foraggregat|task ?result|a2a|task state|auth-required|input-requiredsurfaced 5 candidates (#384, #314, #300, #277, #190);gh pr diff --name-onlyon each confirmed no overlap with the files touched here. #384 is the nearest neighbour — it widensgetTaskInputRequiredEvent's parameter incore/src/a2a/event_processor_utils.ts, a file this PR does not modify. Nothing on the fork implements a task-result aggregator, so this branches frommainrather than stacking.Complexity review rounds 1–2
Round 2 confirmed both actionable findings fixed, and recorded that the remaining one is a scoping call further review rounds cannot settle. Its one concrete residual cost — "live-but-inert code in
agent_executor.tsin the interim" — is now mitigated by asimplicity:note at theprocessEventcall site stating thatconvertAdkEventToA2AEventemits artifact updates exclusively, so a reader is not misled into thinking status updates flow today. No behaviour change.A maintainer decision is required to merge this. See the third item below.
Round 1 findings
Fixed —
resolveFinalStatusis now a method, not a free function. The reviewer was right and my original rationale ("module-level so it's directly unit-testable") was weak: the function read the aggregator's state and message, had exactly one caller, and leaked theWORKINGsentinel outside the class. A public method is equally testable, and the repo's own rule ("extract a method that does not usethis") points the other way here because it does usethis. Net -12 lines and one fewer exported symbol. I named itresolveFinalStatusrather than the suggestedapplyTobecause it returns a new event and mutates nothing, and the guidelines require a name that matches what the function does.Fixed — the
as unknown as () => Runnercast is confined to onestubRunner()helper. It now backs all three call sites, replacing the two pre-existing occurrences of the identical shape as well as mine, so the file has one such cast where it previously had three. Only mock wiring moved; no assertion was touched.Declined, with reasoning — "drop the module and wiring; land them with the first real caller." The reviewer's factual analysis is correct and I do not dispute it:
convertAdkEventToA2AEventreturns onlyartifact-updateevents, soprocessEvent's guard never passes andresolveFinalStatusalways takes its fallback branch. I verified this independently before the reviewer did and reported it above as mutation 7. I am not taking the remedy, for three reasons:event_converter.pyto emit intermediateauth-required/input-requiredstatus updates is a distinct, larger change that this task is explicitly scoped to exclude, with a separate task tracking it. Landing the aggregator now means the converter port is a small diff against a tested, reviewed precedence table instead of one large change carrying both.That said, the reviewer is applying the rubric correctly and the disagreement is a genuine scoping judgement, not a defect — so it is flagged here and in
.foundry_result.jsonrather than silently resolved. If the maintainer prefers the strict reading, the right action is to close this PR and fold the module into the converter port; nothing here is a prerequisite for that work, and the module plus its test file move as a unit.One thing the review did not flag that follows from its own logic: after the refactor the
taskState/taskStatusMessagegetters have no production reader either, sinceresolveFinalStatusreads the private fields directly. They are kept deliberately — they are required by the spec, they mirror Python'stask_state/task_status_messageproperties, and they are the only way a test can observe the "refresh the message while stillworking" rule, whichresolveFinalStatuscannot expose because it returns the fallback unchanged in exactly that state.No suppressions in
src/. Oneas unknown as () => Runnerappears in the added executor test, at thevi.mocked(Runner).mockImplementationboundary:agent_executor_test.tsmocks the whole runner module, so the double is structurally partial (Runneralso requiresagent,pluginManager,runEphemeral,saveArtifactsand theRUNNER_SIGNATURE_SYMBOLbrand). This is the file's pre-existing idiom — it already occurs 3 times — and the added test is the only new occurrence. The other three casts the first draft copied were removed: the session now uses thecreateSessionfactory, and theRunnerConfigliteral needs no cast because every field is optional.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
New file
core/test/a2a/task_result_aggregator_test.ts— 20 cases. Cases 1–13 are direct ports of every case intests/unittests/a2a/executor/test_task_result_aggregator.py; 14–17 close branches the Python suite leaves uncovered; 18–20 coverresolveFinalStatus. Case 8 passes both aTaskArtifactUpdateEventand aTaskwhosestatus.stateisfailed, and asserts theTaskis left untouched — the regression guard against duck-typing onstatusinstead of onkind(an A2ATaskalso hasstatus.state, and Python'sisinstancecheck excludes it).core/test/a2a/agent_executor_test.ts— exactly one newitadded; no existing test modified or deleted.Coverage of the new module is 100% statements / branches / functions / lines:
Mutation proof. Each mutation was applied to the source, the suite re-run, and the source restored:
&& this.state !== TaskState.FAILEDfrom the auth branchkeeps failed when auth-required arrives afterwardsexpected 'auth-required' to be 'failed'&& this.state !== TaskState.AUTH_REQUIREDfrom the input branchkeeps auth-required when input-required arrives afterwardsexpected 'input-required' to be 'auth-required'event.status.state = TaskState.WORKING;records a failed event and rewrites it to workingexpected 'failed' to be 'working'isTaskStatusUpdateEventguard with a duck-typed'artifact' in eventignores events that are not status updates, including a failed Taskexpected 'failed' to be 'working'resolveFinalStatusalways returnfallbackoverrides the fallback state and message once a signal is aggregated(+1)expected 'completed' to be 'failed'resolveFinalStatusalways override (drop theworkingearly return)publishes a completed final status when the run only produces artifact updatesexpected 'working' to be 'completed'Disclosed negative result. A seventh mutation — removing the
resolveFinalStatus(...)call from the executor's close-out entirely — leaves all 6 executor tests passing. That is not a gap in the test, it is the scope note above made measurable: because the converter emits only artifact updates today, the aggregated state never leavesworkingand the call is genuinely a no-op on every reachable path. The executor test pins the postcondition it can pin (the aggregator must not corrupt thecompletedclose-out, mutation 6);resolveFinalStatus's own behaviour is pinned by mutation 5 against the direct unit tests. There is no way to drive a status-update event throughexecute()without changingconvertAdkEventToA2AEvent, which is out of scope.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
No live A2A server run is needed: the change has no wire, network, or filesystem surface, adds no dependency, and produces no observable difference in the executor's default event stream. Verification is the targeted suite below plus a side-by-side reread of
task_result_aggregator.pyagainsttask_result_aggregator.ts, confirming every branch, every guard and the untouchedfinalflag correspond one to one.CI note. All checks green on
106383d6:run-tests,run-tests (ubuntu-latest),run-tests (macos-latest),run-tests (windows-latest)andcheck-license(run 30839065584).Earlier revisions of this branch saw red macOS and Windows legs. Those were pre-existing runner flakes in files this change does not touch, and the green run on effectively the same code confirms it: macOS failed on
tests/integration/app_loader/app_loader_test.ts > should discover apps vs agents across directories and standalone files(also failing onfeat/ui-widget-event-actionsrun 30834213956 andfeat/agent-to-mcp-serverrun 30832794899), and Windows oncore/test/code_executors/unsafe_local_code_executor_test.ts > should execute shell code and return stdout(also failing onfeat/application-integration-toolset-part1run 30831832751 andfeat/ui-widget-event-actionsrun 30837081190). One Windows leg was cancelled by matrix fail-fast rather than failing on its own.tests/integration/app_loader/app_loader_test.ts > should discover apps vs agents across directories and standalone files. Also fails onfeat/ui-widget-event-actions(run 30834213956) andfeat/agent-to-mcp-server(run 30832794899).core/test/code_executors/unsafe_local_code_executor_test.ts > should execute shell code and return stdout, a process-spawn timeout. Also fails onfeat/application-integration-toolset-part1(run 30831832751) andfeat/ui-widget-event-actions(run 30837081190). On the latest run the Windows leg was cancelled by matrix fail-fast rather than failing on its own.Nothing here touches
core/src/code_executorsor the app loader. The failed jobs were re-run and failed identically on the same unrelated tests.a2a_event_test.tsandevent_processor_utils_test.tsare included because this change edits theTaskStateenum they exercise.npm run ts:checkreports pre-existing errors elsewhere in the repo (a stalecore/distvscore/srcdeclaration-identity artifact, e.g.BASE_AGENT_SIGNATURE_SYMBOLincore/test/a2a/agent_card_test.ts); the count did not increase and none of them are ina2a_event.ts,agent_executor.tsortask_result_aggregator.ts.Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.