From 36fe0e32b50715fb0db8bf21f1b591e37cefa05f Mon Sep 17 00:00:00 2001 From: Johnny Wilson Dougherty <192861341+JohnnyWilson16@users.noreply.github.com> Date: Fri, 6 Nov 2026 17:53:23 +0530 Subject: [PATCH 1/2] fix(core): clear function invocation budget state from session on failed turn (#9677) --- ...issue-9677-tool-approval-budget-cleanup.md | 95 +++++++++++ ...-11-06-issue-9819-windows-shell-parsing.md | 160 ++++++++++++++++++ .../packages/core/agent_framework/_tools.py | 72 ++++---- .../tests/core/test_harness_tool_approval.py | 74 ++++++++ 4 files changed, 372 insertions(+), 29 deletions(-) create mode 100644 docs/superpowers/plans/2026-11-06-issue-9677-tool-approval-budget-cleanup.md create mode 100644 docs/superpowers/plans/2026-11-06-issue-9819-windows-shell-parsing.md diff --git a/docs/superpowers/plans/2026-11-06-issue-9677-tool-approval-budget-cleanup.md b/docs/superpowers/plans/2026-11-06-issue-9677-tool-approval-budget-cleanup.md new file mode 100644 index 00000000000..5f612b7fad0 --- /dev/null +++ b/docs/superpowers/plans/2026-11-06-issue-9677-tool-approval-budget-cleanup.md @@ -0,0 +1,95 @@ +# Tool Approval Budget State Cleanup on Turn Failure Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Ensure `ToolApprovalMiddleware` per-attempt function-invocation budget state in `session.state` is cleaned up after provider errors, timeouts, and cancellations, preventing budget exhaustion on retry turns while preserving resumable pending approvals. + +**Architecture:** In `_tools.py`, integrate `_clear_budget_state_from_session` into exception, task cancellation, and generator exit paths of the tool execution loop. The cleanup will check `invocation_session.state[_TOOL_APPROVAL_STATE_KEY]`: if active approval requests or pending pause batches are waiting for human resolution, the budget is preserved; otherwise, stale per-turn iteration and duration counts are purged. + +**Tech Stack:** Python 3.10+, `pytest`, `pytest-asyncio`, `agent_framework`. + +**Spec:** GitHub Issue #9677 (https://github.com/microsoft/agent-framework/issues/9677), triage planning directive by maintainer `eavanvalkenburg`. + +## Global Constraints + +- Never clear budget state when a run pauses for human-in-the-loop approval (`_PENDING_APPROVAL_REQUESTS_KEY` or `_PENDING_MIXED_PAUSE_BATCH_KEY`). +- Maintain exact streaming and non-streaming parity. +- Must not touch public agent APIs, middleware interfaces, or `AgentSession` contracts. +- Tests must use mocked LLM chat clients without cloud credentials or network access. + +## Review Focus + +- Turn retries immediately following a transient provider exception. +- Turn retries following an `asyncio.CancelledError` or client timeout. +- Preservation of pending approval requests across session serialization. +- Streaming generator early-break / generator close cleanup. +- Preservation of legitimate cross-turn multi-agent memory in `session.state`. + +--- + +### Task 1: Add Reproducing Regression Tests for Issue #9677 + +**Files:** +- Modify/Create: `python/packages/core/tests/core/test_tool_approval_budget.py` + +**Interfaces:** +- Consumes: `Agent`, `AgentSession`, `ToolApprovalMiddleware`, mock chat client. +- Produces: Test functions validating budget clearance on failure and budget retention on approval pause. + +- [ ] **Step 1: Write reproducing unit tests** + +```python +import pytest +from agent_framework import Agent, AgentSession, Message +from agent_framework._harness._tool_approval import ToolApprovalMiddleware +from agent_framework._tools import _FUNCTION_INVOCATION_BUDGET_STATE_KEY + + +@pytest.mark.asyncio +async def test_failed_turn_clears_budget_state_on_session(mock_failing_chat_client) -> None: + session = AgentSession() + agent = Agent( + client=mock_failing_chat_client, + middleware=[ToolApprovalMiddleware()], + ) + + with pytest.raises(RuntimeError, match="transient provider error"): + await agent.run("call tool", session=session) + + # Budget key must be cleared from session after failure + assert _FUNCTION_INVOCATION_BUDGET_STATE_KEY not in session.state +``` + +- [ ] **Step 2: Run test to verify failure on current main** + +Run: `uv run pytest packages/core/tests/core/test_tool_approval_budget.py` +Expected: FAIL with `assert '_function_invocation_budget_state' not in session.state` failing. + +--- + +### Task 2: Implement Unwinding Budget Cleanup in `_tools.py` + +**Files:** +- Modify: `python/packages/core/agent_framework/_tools.py` + +**Interfaces:** +- Consumes: `_clear_budget_state_from_session(invocation_session, budget_state)` +- Produces: Hardened error and cancellation handlers in runtime invocation loops. + +- [ ] **Step 1: Update non-streaming tool invocation loop error handling** + +Ensure that on any caught unhandled exception, task cancellation, or abnormal loop termination, `_clear_budget_state_from_session(invocation_session, budget_state)` is invoked in a `finally` or exception boundary. + +- [ ] **Step 2: Update streaming generator termination handlers** + +Ensure that streaming generator cancellation and generator `aclose()` trigger the same `_clear_budget_state_from_session` check. + +- [ ] **Step 3: Run regression tests to verify pass** + +Run: `uv run pytest packages/core/tests/core/test_tool_approval_budget.py` +Expected: PASS. + +- [ ] **Step 4: Run full core test suite** + +Run: `uv run pytest packages/core/tests/core/test_tools.py` +Expected: All tests pass. diff --git a/docs/superpowers/plans/2026-11-06-issue-9819-windows-shell-parsing.md b/docs/superpowers/plans/2026-11-06-issue-9819-windows-shell-parsing.md new file mode 100644 index 00000000000..2c3f84eda79 --- /dev/null +++ b/docs/superpowers/plans/2026-11-06-issue-9819-windows-shell-parsing.md @@ -0,0 +1,160 @@ +# Windows Command Line Tokenization for LocalShellTool Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fix Windows shell path and argument tokenization in `LocalShellTool` and `AGENT_FRAMEWORK_SHELL` by replacing POSIX `shlex.split` with a Windows-native `CommandLineToArgvW` tokenizer when running on or targeting Windows. + +**Architecture:** Implement a specialized `_split_windows_command_line(cmd: str) -> list[str]` tokenizer in `_resolve.py` that preserves literal backslashes, double-quoted substrings with embedded whitespace, escaped quotes (`\"`), and explicit empty arguments (`""`). Route string-based shell resolution through this tokenizer when `sys.platform == "win32"` while preserving standard POSIX `shlex.split` behavior on Unix platforms. + +**Tech Stack:** Python 3.10+, `pytest`, `agent_framework_tools.shell`. + +**Spec:** GitHub Issue #9819 (https://github.com/microsoft/agent-framework/issues/9819), maintainer review on closed PR #9858. + +## Global Constraints + +- Never alter POSIX shell parsing on Linux or macOS (`shlex.split` remains active). +- Never add new external dependencies; use standard library parsing logic. +- Must satisfy `CommandLineToArgvW` rules: $2N$ backslashes before a quote yield $N$ backslashes without escaping the quote; $2N+1$ backslashes before a quote yield $N$ backslashes plus a literal quote. Backslashes elsewhere are literal. +- Explicit empty quoted arguments (e.g. `""`) must produce an empty string element `""` in argv. +- Regression tests must run deterministically on Linux/macOS CI using `monkeypatch.setattr(resolve_module.sys, "platform", "win32")`. + +## Review Focus + +- Unquoted Windows paths containing spaces and backslashes (e.g., `C:\Program Files\PowerShell\7\pwsh.exe`). +- Adjacent quoted and unquoted fragments (e.g., `--rcfile="C:\Program Files\bash.rc"`). +- Explicit empty argument tokens (e.g., `--flag "" -v`). +- Escaped quotation marks within quoted parameters (e.g., `--opt="val\"ue"`). +- Network UNC paths with consecutive leading backslashes (e.g., `\\server\share\tool.exe`). + +--- + +### Task 1: Add Reproducing Regression Tests in `test_shell_resolve.py` + +**Files:** +- Modify: `python/packages/tools/tests/test_shell_resolve.py` + +**Interfaces:** +- Consumes: `agent_framework_tools.shell._resolve.resolve_shell` +- Produces: Test functions validating Windows path preservation and argument tokenization. + +- [ ] **Step 1: Write the failing tests** + +```python +def test_win32_shell_override_preserves_backslashes(monkeypatch: pytest.MonkeyPatch) -> None: + import agent_framework_tools.shell._resolve as resolve_module + + monkeypatch.setattr(resolve_module.sys, "platform", "win32") + path = r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe" + argv = resolve_shell(path, interactive=False) + assert argv == [path, "-Command"] + + +def test_win32_env_shell_override_preserves_backslashes(monkeypatch: pytest.MonkeyPatch) -> None: + import agent_framework_tools.shell._resolve as resolve_module + + monkeypatch.setattr(resolve_module.sys, "platform", "win32") + path = r"C:\msys64\usr\bin\bash.exe" + monkeypatch.setenv("AGENT_FRAMEWORK_SHELL", path) + argv = resolve_shell(None, interactive=False) + assert argv == [path, "-c"] + + +def test_win32_quoted_shell_with_spaces_and_empty_arg(monkeypatch: pytest.MonkeyPatch) -> None: + import agent_framework_tools.shell._resolve as resolve_module + + monkeypatch.setattr(resolve_module.sys, "platform", "win32") + cmd = r'"C:\Program Files\PowerShell\7\pwsh.exe" -NoProfile "" --opt="val\"ue"' + argv = resolve_shell(cmd, interactive=False) + assert argv[0] == r"C:\Program Files\PowerShell\7\pwsh.exe" + assert argv[1] == "-NoProfile" + assert argv[2] == "" + assert argv[3] == r'--opt=val"ue' + assert argv[-1] == "-Command" +``` + +- [ ] **Step 2: Run test to verify failure** + +Run: `uv run pytest packages/tools/tests/test_shell_resolve.py -k "win32"` +Expected: FAIL with backslashes stripped or tokenization mismatch. + +--- + +### Task 2: Implement Windows `CommandLineToArgvW` Tokenizer in `_resolve.py` + +**Files:** +- Modify: `python/packages/tools/agent_framework_tools/shell/_resolve.py` + +**Interfaces:** +- Consumes: `shell: str | Sequence[str] | None`, `AGENT_FRAMEWORK_SHELL` +- Produces: `_split_windows_command_line(cmd: str) -> list[str]` and updated `resolve_shell` routing. + +- [ ] **Step 1: Write `_split_windows_command_line` implementation** + +```python +def _split_windows_command_line(cmd: str) -> list[str]: + """Parse a Windows command line string into argv following CommandLineToArgvW rules.""" + args: list[str] = [] + buf: list[str] = [] + in_quotes = False + has_token = False + i = 0 + n = len(cmd) + + while i < n: + c = cmd[i] + if c in " \t" and not in_quotes: + if has_token: + args.append("".join(buf)) + buf = [] + has_token = False + i += 1 + continue + + if c == "\\": + num_slashes = 0 + while i < n and cmd[i] == "\\": + num_slashes += 1 + i += 1 + if i < n and cmd[i] == '"': + buf.append("\\" * (num_slashes // 2)) + if num_slashes % 2 == 1: + buf.append('"') + has_token = True + i += 1 + else: + in_quotes = not in_quotes + has_token = True + i += 1 + else: + buf.append("\\" * num_slashes) + has_token = True + elif c == '"': + in_quotes = not in_quotes + has_token = True + i += 1 + else: + buf.append(c) + has_token = True + i += 1 + + if has_token: + args.append("".join(buf)) + return args +``` + +- [ ] **Step 2: Update `resolve_shell` and `is_powershell` in `_resolve.py`** + +In `_split_shell_override(value: str) -> list[str]`: +Route to `_split_windows_command_line(value)` if `sys.platform == "win32"`, else `shlex.split(value)`. +In `is_powershell`: +Normalize backslashes via `argv[0].replace("\\", "/")` before `os.path.basename` to ensure win32 paths detect properly when tested on POSIX platforms. + +- [ ] **Step 3: Run tests to verify they pass** + +Run: `uv run pytest packages/tools/tests/test_shell_resolve.py` +Expected: ALL PASS. + +- [ ] **Step 4: Run repo linting and typing checks** + +Run: `uv run ruff check packages/tools` and `uv run pyright packages/tools` +Expected: 0 errors. diff --git a/python/packages/core/agent_framework/_tools.py b/python/packages/core/agent_framework/_tools.py index 03634de4c5d..2b540c76738 100644 --- a/python/packages/core/agent_framework/_tools.py +++ b/python/packages/core/agent_framework/_tools.py @@ -6548,20 +6548,27 @@ def get_response( # Dispatch to the shape-specific loop; both loops follow the same approval -> model -> execution phases. if not stream: - return self._get_response_with_function_invocation( - super_get_response=super_get_response, - messages=messages, - options=mutable_options, - request_kwargs=request_kwargs, - compaction_strategy=compaction_strategy, - tokenizer=tokenizer, - execute_function_calls=execute_function_calls, - invocation_session=invocation_session, - approval_session_is_authoritative=approval_session_is_authoritative, - budget_state=budget_state, - max_errors=max_errors, - middleware_pipeline=function_middleware_pipeline, - ) + + async def _run_non_streaming() -> ChatResponse[Any]: + try: + return await self._get_response_with_function_invocation( + super_get_response=super_get_response, + messages=messages, + options=mutable_options, + request_kwargs=request_kwargs, + compaction_strategy=compaction_strategy, + tokenizer=tokenizer, + execute_function_calls=execute_function_calls, + invocation_session=invocation_session, + approval_session_is_authoritative=approval_session_is_authoritative, + budget_state=budget_state, + max_errors=max_errors, + middleware_pipeline=function_middleware_pipeline, + ) + finally: + _clear_budget_state_from_session(invocation_session, budget_state) + + return _run_non_streaming() response_format = mutable_options.get("response_format") invalidation_error: list[ResponseInvalidatedException] = [] @@ -6571,22 +6578,29 @@ def finalize_stream(updates: Sequence[ChatResponseUpdate]) -> ChatResponse[Any]: raise invalidation_error[0] return self._finalize_function_invocation_updates(updates, response_format=response_format) + async def _run_streaming() -> AsyncIterable[ChatResponseUpdate]: + try: + async for update in self._stream_response_with_function_invocation( + super_get_response=super_get_response, + messages=messages, + options=mutable_options, + request_kwargs=request_kwargs, + compaction_strategy=compaction_strategy, + tokenizer=tokenizer, + execute_function_calls=execute_function_calls, + invocation_session=invocation_session, + approval_session_is_authoritative=approval_session_is_authoritative, + budget_state=budget_state, + max_errors=max_errors, + middleware_pipeline=function_middleware_pipeline, + invalidation_error=invalidation_error, + ): + yield update + finally: + _clear_budget_state_from_session(invocation_session, budget_state) + return ResponseStream( - self._stream_response_with_function_invocation( - super_get_response=super_get_response, - messages=messages, - options=mutable_options, - request_kwargs=request_kwargs, - compaction_strategy=compaction_strategy, - tokenizer=tokenizer, - execute_function_calls=execute_function_calls, - invocation_session=invocation_session, - approval_session_is_authoritative=approval_session_is_authoritative, - budget_state=budget_state, - max_errors=max_errors, - middleware_pipeline=function_middleware_pipeline, - invalidation_error=invalidation_error, - ), + _run_streaming(), finalizer=finalize_stream, ) diff --git a/python/packages/core/tests/core/test_harness_tool_approval.py b/python/packages/core/tests/core/test_harness_tool_approval.py index d2170fbff4d..6d72c84923f 100644 --- a/python/packages/core/tests/core/test_harness_tool_approval.py +++ b/python/packages/core/tests/core/test_harness_tool_approval.py @@ -3504,3 +3504,77 @@ class Answer(BaseModel): assert response.text == bad_text with pytest.raises(ValueError): _ = response.value + + +@pytest.mark.parametrize("streaming", [False, True], ids=["non-streaming", "streaming"]) +async def test_tool_approval_clears_budget_on_turn_failure( + chat_client_base: MockBaseChatClient, + streaming: bool, +) -> None: + """A failed turn must clear per-attempt budget state from session so retries get full budget.""" + from agent_framework._tools import _FUNCTION_INVOCATION_BUDGET_STATE_KEY + + def sample_tool(value: str) -> str: + return f"tool:{value}" + + agent = Agent( + client=chat_client_base, + tools=[sample_tool], + middleware=[ToolApprovalMiddleware(auto_approval_rules=[lambda _: True])], + ) + session = AgentSession(session_id="retry-budget-test") + + if streaming: + chat_client_base.streaming_responses = [ + [ + ChatResponseUpdate( + role="assistant", + contents=[ + Content.from_function_call( + call_id="c1", + name="sample_tool", + arguments='{"value": "1"}', + ) + ], + ) + ] + ] + orig_stream = chat_client_base._get_streaming_response + + def failing_stream(**kwargs: Any) -> Any: + if chat_client_base.call_count == 1: + raise RuntimeError("transient provider error") + return orig_stream(**kwargs) + + chat_client_base._get_streaming_response = failing_stream + with pytest.raises(RuntimeError, match="transient provider error"): + stream = agent.run("call tool", stream=True, session=session) + async for _ in stream: + pass + else: + chat_client_base.run_responses = [ + ChatResponse( + messages=Message( + role="assistant", + contents=[ + Content.from_function_call( + call_id="c1", + name="sample_tool", + arguments='{"value": "1"}', + ) + ], + ) + ) + ] + orig_non_stream = chat_client_base._get_non_streaming_response + + async def failing_non_stream(**kwargs: Any) -> Any: + if chat_client_base.call_count == 1: + raise RuntimeError("transient provider error") + return await orig_non_stream(**kwargs) + + chat_client_base._get_non_streaming_response = failing_non_stream + with pytest.raises(RuntimeError, match="transient provider error"): + await agent.run("call tool", session=session) + + assert _FUNCTION_INVOCATION_BUDGET_STATE_KEY not in session.state From 09f8ccd0c60f6e32d4ada660d3aacdc704299527 Mon Sep 17 00:00:00 2001 From: Johnny Wilson Dougherty Date: Fri, 6 Nov 2026 18:29:04 +0530 Subject: [PATCH 2/2] Refactor streaming response handling in _tools.py Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- python/packages/core/agent_framework/_tools.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/python/packages/core/agent_framework/_tools.py b/python/packages/core/agent_framework/_tools.py index 2b540c76738..4adf44fe4d0 100644 --- a/python/packages/core/agent_framework/_tools.py +++ b/python/packages/core/agent_framework/_tools.py @@ -6579,8 +6579,9 @@ def finalize_stream(updates: Sequence[ChatResponseUpdate]) -> ChatResponse[Any]: return self._finalize_function_invocation_updates(updates, response_format=response_format) async def _run_streaming() -> AsyncIterable[ChatResponseUpdate]: + updates: AsyncIterable[ChatResponseUpdate] | None = None try: - async for update in self._stream_response_with_function_invocation( + updates = self._stream_response_with_function_invocation( super_get_response=super_get_response, messages=messages, options=mutable_options, @@ -6594,10 +6595,16 @@ async def _run_streaming() -> AsyncIterable[ChatResponseUpdate]: max_errors=max_errors, middleware_pipeline=function_middleware_pipeline, invalidation_error=invalidation_error, - ): + ) + async for update in updates: yield update finally: - _clear_budget_state_from_session(invocation_session, budget_state) + try: + close = getattr(updates, "aclose", None) + if close is not None: + await close() + finally: + _clear_budget_state_from_session(invocation_session, budget_state) return ResponseStream( _run_streaming(),