Skip to content

Python: Separate hosted and local shell calls - #8294

Open
Eduard van Valkenburg (eavanvalkenburg) wants to merge 2 commits into
microsoft:mainfrom
eavanvalkenburg:turbo-engine
Open

Python: Separate hosted and local shell calls#8294
Eduard van Valkenburg (eavanvalkenburg) wants to merge 2 commits into
microsoft:mainfrom
eavanvalkenburg:turbo-engine

Conversation

@eavanvalkenburg

@eavanvalkenburg Eduard van Valkenburg (eavanvalkenburg) commented Sep 11, 2026

Copy link
Copy Markdown
Member

Motivation & Context

Hosted shell response items must remain provider transcript content rather than entering the local function invocation path. Local shell execution should require an explicit local item and preserve its configured approval boundary.

Description & Review Guide

  • What are the major changes? Keep hosted shell calls informational, allow only valid explicit local shell items into local execution, and preserve existing local shell approval defaults.
  • What is the impact of these changes? Hosted and local shell paths remain separate in streaming and non-streaming responses without changing existing factory defaults. The OpenAI package passes 488 tests, syntax checks, strict source typing, and all test type checkers.
  • What do you want reviewers to focus on? The response-item classification boundary and preservation of configured local approval behavior.

Related Issue

No related public issue.

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.

🔵 Needs a closer look

The security-sensitive local command execution boundary warrants final human validation despite strong regression coverage.

Pull request overview

Separates hosted shell transcript items from explicit local shell execution while preserving approval safeguards.

Changes:

  • Keeps hosted shell calls informational in streaming and non-streaming paths.
  • Validates local shell items and defaults plain executors to approval-required.
  • Adds regression coverage and updates the function-loop specification.
File summaries
File Description
python/packages/openai/agent_framework_openai/_chat_client.py Enforces shell classification and approval boundaries.
python/packages/openai/tests/openai/test_openai_chat_client.py Tests hosted/local separation, validation, and approvals.
docs/specs/004-python-function-calling-loop.md Documents the shell execution invariant and tests.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 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)): 8bfe46b38415
Model: gpt-5.6-sol-fast

Overview

The change cleanly separates hosted and local shell response items, validates local item identity and command shape, and adds streaming/non-streaming coverage for the execution boundary. It also defaults plain local callables to approval, but that changes the behavior of a stable public factory for existing callers that omit approval_mode; this needs explicit breaking-change or migration treatment.

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/openai/agent_framework_openai/_chat_client.py

Comment thread python/packages/openai/agent_framework_openai/_chat_client.py Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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

Development

Successfully merging this pull request may close these issues.

3 participants