Python: validate local shell approval binding - #8233
Python: validate local shell approval binding#8233Eduard van Valkenburg (eavanvalkenburg) wants to merge 1 commit into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
There was a problem hiding this comment.
🟢 Approval recommended
The test-only changes comprehensively validate the stated approval-binding scenarios without runtime or API impact.
Pull request overview
Adds deterministic end-to-end regression coverage for secure LocalShellTool approval binding.
Changes:
- Verifies exact approved argv execution and variable-change rejection.
- Tests substituted approval fields and replay isolation across turns and sessions.
File summaries
| File | Description |
|---|---|
python/packages/tools/tests/test_local_shell_tool.py |
Adds mocked subprocess approval-binding security tests. |
Review details
- Files reviewed: 1/1 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[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: No findings
Scope: full PR (1 commit(s)): 94ff9d9be89a
Model: gpt-5.6-sol-fast
Overview
This test-only PR adds end-to-end LocalShellTool coverage across variable expansion, policy approval, immutable pending-request rebinding, subprocess execution, and replay handling. Existing occurrence identity, resolved-argument signatures, session-scoped state, and consume-on-bind behavior provide concrete guardrails, and the new assertions verify that substituted fields cannot alter executed argv. No PR-introduced Critical, High, or Medium defect was established.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.
| options: Mapping[str, Any], | ||
| **kwargs: Any, | ||
| ) -> Awaitable[ChatResponse]: | ||
| assert not stream |
There was a problem hiding this comment.
Seems to me like you can probably get rid of stream? I also don't see _inner_get_response called anywhere directly in the test. Is this an API enforced by BaseChatClient ?
| if mutation == "request_id": | ||
| create_process.assert_not_awaited() | ||
| else: | ||
| assert create_process.await_count == 1 | ||
| assert create_process.await_args is not None | ||
| assert create_process.await_args.args == (_TEST_SHELL, "-c", _APPROVED_COMMAND) |
There was a problem hiding this comment.
At a glance, it's not entirely obvious to me which code path is the one meant to fail the test. Maybe it would be clearer if we raised something instead? Not sure if that is a common pattern.
Motivation & Context
Add end-to-end coverage that verifies a local shell invocation supplied through a labeled variable remains bound to the command reviewed during approval and cannot be replayed across turns or sessions.
Description & Review Guide
LocalShellToolregression coverage for exact approved argv, changed variable resolution, substituted approval fields, and replay isolation.Related Issue
N/A - no high-confidence overlapping open issue was found.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and the title prefix in sync automatically.