fix(aws-strands): carry the client's tool failure onto the reconciled toolResult status - #2363
Conversation
contextablemark
left a comment
contextablemark
left a comment
There was a problem hiding this comment.
Good fix overall—the client error flag is carried through ID resolution and stamped into both persisted and in-memory history.
I think we should include the empty-content failure you identified before merging. It currently takes the successful-void fallback, leaving the persisted placeholder marked as successful, so issue #2361 remains behaviorally incomplete.
The inline suggestions keep successful void results unchanged while routing failed void results through reconciliation, and parameterize your existing end-to-end test rather than duplicating its setup.
I verified the suggestions against current main: the empty case fails before the gate change, then the full suite passes with 212 passed and 2 skipped, and the package builds successfully.
| resolved_native_results: Dict[str, Tuple[str, bool]] = {} | ||
| corrected_native_ids: set[str] = set() | ||
| has_nonvoid_frontend_result = any( | ||
| (r["text"] or "").strip() for r in frontend_results |
There was a problem hiding this comment.
An errored result with empty content still takes the successful-void path because this gate only examines text. Including is_error preserves successful void handling while allowing the failed placeholder to be reconciled.
| (r["text"] or "").strip() for r in frontend_results | |
| (r["text"] or "").strip() or r["is_error"] for r in frontend_results |
| @pytest.mark.asyncio | ||
| async def test_client_reported_failure_lands_as_an_error_status(self, tmp_path): | ||
| # The placeholder was written by the proxy tool with a hardcoded | ||
| # "success" status. Reconciliation must overwrite the status as well as | ||
| # the text, or the model is told a failed frontend tool succeeded. | ||
| from strands.session.file_session_manager import FileSessionManager | ||
|
|
||
| sm = FileSessionManager(session_id="thread-errstatus", storage_dir=str(tmp_path)) | ||
| instance = await _run_session_continuation( | ||
| sm, | ||
| "default", | ||
| messages=[ | ||
| _payload_assistant("wire-1", "approve"), | ||
| _payload_tool("wire-1", "tool failed: invalid id", error="invalid id"), | ||
| ], | ||
| tools=[_frontend_tool("approve")], | ||
| wire_map={"wire-1": "native-1"}, | ||
| store=[_store_tool_use("native-1", "approve"), _store_placeholder("native-1")], | ||
| ) | ||
| assert instance.stream_prompts == [None] | ||
| block = _result_content(sm, "default", 1)[0]["toolResult"] | ||
| assert block["content"] == [{"text": "tool failed: invalid id"}] | ||
| assert block["status"] == "error" |
There was a problem hiding this comment.
Could we make the existing end-to-end case cover both non-empty and empty failures? This proves that an empty failure actually enters reconciliation and persists the error status without duplicating the fixture.
| @pytest.mark.asyncio | |
| async def test_client_reported_failure_lands_as_an_error_status(self, tmp_path): | |
| # The placeholder was written by the proxy tool with a hardcoded | |
| # "success" status. Reconciliation must overwrite the status as well as | |
| # the text, or the model is told a failed frontend tool succeeded. | |
| from strands.session.file_session_manager import FileSessionManager | |
| sm = FileSessionManager(session_id="thread-errstatus", storage_dir=str(tmp_path)) | |
| instance = await _run_session_continuation( | |
| sm, | |
| "default", | |
| messages=[ | |
| _payload_assistant("wire-1", "approve"), | |
| _payload_tool("wire-1", "tool failed: invalid id", error="invalid id"), | |
| ], | |
| tools=[_frontend_tool("approve")], | |
| wire_map={"wire-1": "native-1"}, | |
| store=[_store_tool_use("native-1", "approve"), _store_placeholder("native-1")], | |
| ) | |
| assert instance.stream_prompts == [None] | |
| block = _result_content(sm, "default", 1)[0]["toolResult"] | |
| assert block["content"] == [{"text": "tool failed: invalid id"}] | |
| assert block["status"] == "error" | |
| @pytest.mark.parametrize( | |
| "content", ["tool failed: invalid id", ""], ids=["with-text", "empty"] | |
| ) | |
| @pytest.mark.asyncio | |
| async def test_client_reported_failure_lands_as_an_error_status( | |
| self, tmp_path, content | |
| ): | |
| # The placeholder was written by the proxy tool with a hardcoded | |
| # "success" status. Reconciliation must overwrite the status as well as | |
| # the text, or the model is told a failed frontend tool succeeded. | |
| from strands.session.file_session_manager import FileSessionManager | |
| sm = FileSessionManager(session_id="thread-errstatus", storage_dir=str(tmp_path)) | |
| instance = await _run_session_continuation( | |
| sm, | |
| "default", | |
| messages=[ | |
| _payload_assistant("wire-1", "approve"), | |
| _payload_tool("wire-1", content, error="invalid id"), | |
| ], | |
| tools=[_frontend_tool("approve")], | |
| wire_map={"wire-1": "native-1"}, | |
| store=[_store_tool_use("native-1", "approve"), _store_placeholder("native-1")], | |
| ) | |
| assert instance.stream_prompts == [None] | |
| block = _result_content(sm, "default", 1)[0]["toolResult"] | |
| assert block["content"] == [{"text": content}] | |
| assert block["status"] == "error" |
| @@ -1023,7 +1032,7 @@ async def run(self, input_data: RunAgentInput) -> AsyncIterator[Any]: | |||
| # empty toolResult. When reconciling, void placeholders in the same | |||
There was a problem hiding this comment.
Please keep the nearby explanation aligned with the broadened gate.
| # empty toolResult. When reconciling, void placeholders in the same | |
| # empty toolResult. A failed void result is the exception: it must | |
| # reconcile so its status replaces the proxy's hardcoded success. | |
| # When reconciling, void placeholders in the same |
… toolResult status _build_strands_history stamps toolResult.status from ToolMessage.error, but it runs only on the replay_history branch. With a session_manager configured, the run reconciles the persisted history instead, and reconciliation rewrote only the content - leaving the proxy's hardcoded success status in place. A failed frontend tool still reached the model as a success on that path. Widen the reconcile channel to carry the flag alongside the text: resolve_native_ids returns (text, is_error) per native toolUseId, and _correct_message writes the status where it rewrites the content.
An errored result with empty content took the successful-void fallback, so its persisted placeholder kept the proxy's hardcoded success status. The gate now also admits results carrying the error flag, leaving successful void handling unchanged. The end-to-end case is parameterized over non-empty and empty content instead of duplicating the fixture, proving the empty failure reaches reconciliation and persists the error status.
5a5cac9 to
c6b64f3
Compare
|
All three suggestions applied, rebased onto current You are right that leaving the empty case out made the fix behaviourally incomplete. The gate now reads My numbers match yours: 212 passed, 2 skipped. Checked the direction as well — with the gate narrowed back to text only, the One consequence worth naming, since it is the reason the old gate looked reasonable: a failed void result now clears its placeholder to On the ordering with #2387: nothing in my branch touches |
Python Preview PackagesVersion
Install with uvAdd the TestPyPI index to your [[tool.uv.index]]
name = "testpypi"
url = "https://test.pypi.org/simple/"
explicit = trueThen install the packages you need: # Core SDK
uv add 'ag-ui-protocol==0.0.0.dev1786713500' --index testpypi
# Integrations (each already depends on the matching ag-ui-protocol preview)
uv add 'ag-ui-langgraph==0.0.0.dev1786713500' --index testpypi
uv add 'ag-ui-crewai==0.0.0.dev1786713500' --index testpypi
# NOTE: ag-ui-agent-spec depends on pyagentspec (git-only, not on PyPI).
# You will need to install pyagentspec separately from its git repo.
uv add 'ag-ui-agent-spec==0.0.0.dev1786713500' --index testpypi
uv add 'ag_ui_adk==0.0.0.dev1786713500' --index testpypi
uv add 'ag_ui_strands==0.0.0.dev1786713500' --index testpypiInstall with pippip install \
--index-url https://test.pypi.org/simple/ \
--extra-index-url https://pypi.org/simple/ \
ag-ui-protocol==0.0.0.dev1786713500
Commit: 77029cf |
@ag-ui/a2a-middleware
@ag-ui/a2ui-middleware
@ag-ui/event-throttle-middleware
@ag-ui/mcp-apps-middleware
@ag-ui/mcp-middleware
@ag-ui/a2a
@ag-ui/adk
@ag-ui/ag2
@ag-ui/agno
@ag-ui/aws-strands
@ag-ui/claude-agent-sdk
@ag-ui/claude-managed-agents
@ag-ui/crewai
@ag-ui/langchain
@ag-ui/langgraph
@ag-ui/llamaindex
@ag-ui/mastra
@ag-ui/pydantic-ai
@ag-ui/vercel-ai-sdk
@ag-ui/watsonx
@ag-ui/a2ui-toolkit
create-ag-ui-app
@ag-ui/client
@ag-ui/core
@ag-ui/encoder
@ag-ui/proto
commit: |
contextablemark
left a comment
contextablemark
left a comment
There was a problem hiding this comment.
The requested empty-content reconciliation and regression coverage are now included. All checks pass.
2 participants
Fixes #2361. Follow-up to #2317, on the branch that fix does not reach.
The gap
_build_strands_historystampstoolResult.statusfromToolMessage.errorafter #2317, but it has one production caller, inside thereplay_historybranch. When asession_manager_provideris configured, the run takesreconcile_session_resultsinstead and the model reads the persisted history. ThosetoolResultblocks were written by_proxy_funcwith"status": "success"hardcoded (client_proxy_tool.py:58), and_correct_messagerewrote onlycontent— so on that path a failed frontend tool was still asserted to the model as a success.There was no channel for the flag either:
pending_resultswasMapping[str, str], nativetoolUseId-> text.The change
Widen the reconcile channel to carry the flag next to the text, as sketched on the issue:
agent.py:1015— eachfrontend_resultsentry also carriesis_error, read frommsg.error.session_reconcile.py—resolve_native_idsreturns nativetoolUseId->(text, is_error).session_reconcile.py—_correct_messagewritestool_result["status"]in the same place where it rewritestool_result["content"]. The status is always written, not only on failure, since the value it replaces is the proxy's placeholder rather than a real result.agent.py:1119— theresolved_non_voidcomprehension unpacks the new value shape.session_reconcileis not exported from__init__.py, so the signature change stays inside the package. No public API moves.Tests
pytestonintegrations/aws-strands/python: 206 passed, 2 skipped — 199 before this branch, plus 7 new.New cases, in
test_session_reconcile.py:status: "error"on the persisted blockstatus: "success"agent.messagescopy is stamped too, not just the storeAnd in
test_session_manager.py, end to end throughrun(): a clientToolMessagewitherrorset reaches the store asstatus: "error"with the real text, and a successful one stays"success".Checked that the tests fail for the right reason: with the
tool_result["status"]line removed, 4 of them fail and the rest of the suite stays green.Scope
Two things left out on purpose, both noted on the issue before I started:
The legacy branch (
agent.py:1138). It has the same shape, but there the real result reaches the model as a synthetic user message, so there is notoolResultblock to stamp. That is a different change.Void results.
reconcile_session_resultsis gated onhas_nonvoid_frontend_result(agent.py:1037) andToolMessage.contentis a requiredstr, so a failed tool returningcontent=""witherrorset never reaches reconciliation — it falls to the legacy path and the placeholder keeps"success". Widening that gate changes when reconciliation runs at all, which is more than this issue asks for. I can fold it in here instead if you prefer one pass.