Skip to content

Python: Preserve shell outcome fidelity - #8934

Merged
Evan Mattson (moonbox3) merged 3 commits into
microsoft:mainfrom
eavanvalkenburg:shell-outcome-fidelity
Oct 2, 2026
Merged

Evan Mattson (moonbox3) merged 3 commits into
microsoft:mainfrom
eavanvalkenburg:shell-outcome-fidelity

Conversation

@eavanvalkenburg

Copy link
Copy Markdown
Member

Motivation & Context

Shell execution outcomes were crossing Python provider boundaries as display text or default exit outcomes, causing non-zero exits, stderr, and timeouts to be reported as successful commands. This restores structured shell result fidelity for local OpenAI Responses execution and Foundry hosting.

Description & Review Guide

  • What are the major changes? Preserve structured shell fields alongside the existing single text tool result, serialize them through both OpenAI shell output formats, and map Foundry timeout outcomes correctly in both directions.
  • What is the impact of these changes? Non-zero exits remain non-zero, stderr remains separate, timeouts use the protocol timeout outcome, and successful command display and middleware result shape remain compatible.
  • What do you want reviewers to focus on? The singleton text-content compatibility boundary and the OpenAI/Foundry outcome conversions.

Related Issue

Fixes #8929
Fixes #8930

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 title prefix in sync automatically.

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.

Copilot review overview

🟢 Approval recommended

The conversions preserve compatibility while regression tests cover successful, failed, and timed-out shell executions.

Review effort: Balanced
Findings: None

What changed in this PR

Preserves structured shell outcomes across local tools, OpenAI Responses, and Foundry hosting.

Changes:

  • Retains stdout, stderr, exit codes, and timeout state alongside model-facing text.
  • Correctly converts OpenAI and Foundry shell outcomes.
  • Adds regression tests and updates the function-loop specification.
File Description
python/​packages/​tools/​tests/​test_local_shell_tool.py Tests structured local-shell results.
python/​packages/​tools/​tests/​test_docker_shell_tool.py Tests structured Docker-shell results.
python/​packages/​tools/​agent_framework_tools/​shell/​_types.py Converts shell results to structured content.
python/​packages/​tools/​agent_framework_tools/​shell/​_tool.py Returns structured local-shell content.
python/​packages/​tools/​agent_framework_tools/​shell/​_docker.py Returns structured Docker-shell content.
python/​packages/​openai/​tests/​openai/​test_openai_chat_client.py Covers OpenAI shell serialization.
python/​packages/​openai/​agent_framework_openai/​_chat_client.py Preserves structured OpenAI shell outcomes.
python/​packages/​foundry_hosting/​tests/​test_responses.py Covers Foundry outcome round-trips.
python/​packages/​foundry_hosting/​agent_framework_foundry_hosting/​_responses.py Maps Foundry exit and timeout outcomes.
docs/​specs/​004-python-function-calling-loop.md Documents the restored fidelity invariant.

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

@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)): abccb3e779c7
Model: gpt-5.6-sol

Overview

The change preserves structured shell outcomes through the local tool, OpenAI, and Foundry boundaries while retaining the existing model-facing text, with focused coverage for success, non-zero exit, timeout, and persistence paths. Type guards and provider-specific outcome conversion constrain malformed metadata and keep timeout handling distinct from normal exits. However, returning Content directly from the shell wrappers breaks raw-value integrations such as Monty, where commands now execute but their results are rejected instead of reaching sandbox code.

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/tools/agent_framework_tools/shell/_tool.py

Comment thread python/packages/tools/agent_framework_tools/shell/_tool.py Outdated
@manjunathshiva

Copy link
Copy Markdown
Contributor

One regression I hit while checking this against the original repro: when a command's output is truncated, the OpenAI shell_call_output no longer tells the model. format_for_model() appends [output truncated], but the structured stdout is the raw, already-truncated ShellResult.stdout, and the payload is built from the structured fields. With LocalShellTool(max_output_bytes=64) running Write-Output ('x' * 500), main sends the marker inside the text, while this branch sends 64 bytes of output and no marker. Appending the marker to the structured stdout when truncated is set, or carrying truncated and appending it in the payload builder, would keep it. Exit codes, stderr, and the Foundry timeouts in both directions behave correctly in the same check.

@eavanvalkenburg

Eduard van Valkenburg (eavanvalkenburg) commented Oct 1, 2026 •

Copy link
Copy Markdown
Member Author

Manjunath Janardhan (@manjunathshiva) Addressed in 8315fc4e4. The shell result now carries the truncated flag through the hidden structured metadata, and the OpenAI payload builder restores [output truncated] in structured stdout (including when stdout is otherwise empty). Both shell_call_output and local_shell_call_output are covered, while the raw/Monty string path stays unchanged.

@manjunathshiva

Copy link
Copy Markdown
Contributor

Thanks, verified on 8315fc4e4 with the same repro: [output truncated] now reaches both shell_call_output and local_shell_call_output, including the stderr-only case. Exit codes, stderr, and Foundry timeouts in both directions still behave, and the skip_parsing=True path returns a JSON-safe string.

@moonbox3
Evan Mattson (moonbox3) added this pull request to the merge queue Oct 2, 2026
Merged via the queue into microsoft:main with commit a91fe66 Oct 2, 2026
49 checks passed

This branch was previously deployed

1 inactive deployment
github-app-auth — 8315fc4e Deployed Oct 1, 2026 by eavanvalkenburg via add_label #24183
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python

Projects

None yet

4 participants