Python: [BREAKING] Require approval for file-access tools with read-only auto-approval

[westey-m]

Motivation & Context

The harness FileAccessProvider exposes tools that can read, write, and delete
files in a shared store. Previously most of these tools defaulted to
approval_mode="never_require", so an agent could mutate the file store without
any host oversight. For human-in-the-loop scenarios — and to make the new
tool-approval middleware actually demonstrable with file access — the provider's
tools should require approval by default, while still offering an easy way to
auto-approve the safe, read-only operations so common workflows don't become
unusable.

Description & Review Guide

• What are the major changes?

  • FileAccessProvider now registers all six tools (save_file,
    read_file, delete_file, list_files, list_subdirectories,
    search_files) with approval_mode="always_require".
  • Adds two static auto-approval rules usable via
    ToolApprovalMiddleware/create_harness_agent(auto_approval_rules=...):
    • FileAccessProvider.read_only_tools_auto_approval_rule — auto-approves
      only the read-only tools (read, list files, list subdirectories, search),
      so save/delete still prompt.
    • FileAccessProvider.all_tools_auto_approval_rule — auto-approves every
      file-access tool.
  • Exposes the tool names as class constants (SAVE_FILE_TOOL_NAME,
    READ_FILE_TOOL_NAME, DELETE_FILE_TOOL_NAME, LIST_FILES_TOOL_NAME,
    LIST_SUBDIRECTORIES_TOOL_NAME, SEARCH_FILES_TOOL_NAME).
  • Adds a new harness_data_processing.py sample (with a bundled
    working/sales.csv) demonstrating the read-only auto-approval rule: reads
    run automatically while writes prompt for approval.
  • Updates python/packages/core/AGENTS.md and the file-access tests
    accordingly.

• What is the impact of these changes?

  • File-access tools now require host approval by default. Callers that want
    unattended operation opt in via the provided auto-approval rules. This is an
    additive tightening within the experimental harness surface.

• What do you want reviewers to focus on?

  • Whether always_require for all six tools (rather than only mutating tools)
    is the right default, and whether the two named auto-approval rules cover the
    expected use cases.

Related Issue

No related issue is linked for this change.

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.

[github-actions]

Python Test Coverage Report •

File	Stmts	Miss	Cover	Missing
packages/core/agent_framework/_harness
_file_access.py	507	36	92%	180, 312, 314, 326, 330, 625, 635, 651, 692, 719–720, 746, 750, 804–806, 828–831, 859, 952, 1180–1181, 1192–1193, 1204–1205, 1216–1217, 1231–1234, 1260–1261
TOTAL	39866	4497	88%

Python Unit Test Overview

Tests	Skipped	Failures	Errors	Time
7988	34 💤	0 ❌	0 🔥	2m 3s ⏱️

[github-actions]

Automated Code Review

Reviewers: 5 | Confidence: 93%

✓ Correctness

This PR is well-implemented and correct. All six file-access tools are changed from 'never_require' to 'always_require' approval mode, and two static auto-approval rules are provided for opt-in unattended operation. The Content.name attribute access in the rule functions is valid, the frozenset lookups are efficient, the removed ApprovalMode import is clean (no remaining references), and the sample demonstrates the intended usage pattern correctly. No correctness issues found.

✓ Security Reliability

This PR correctly tightens the security default for FileAccessProvider by requiring approval for all six file-access tools (previously only delete required it). The auto-approval rules are well-implemented using frozenset membership checks, and the middleware properly validates Content.name is not None before passing to rule callbacks. No security or reliability issues found — the change improves the security posture of the file access surface.

✓ Test Coverage

The test coverage for this PR is adequate. The three key behaviors (all tools requiring approval, read-only auto-approval rule, all-tools auto-approval rule) each have dedicated tests with both positive and negative assertions. The Content type is constructed correctly per its constructor signature. The ToolApprovalMiddleware integration path is covered by existing tests in test_harness_tool_approval.py. No significant test coverage gaps were found.

✓ Failure Modes

The PR is clean from a failure-mode perspective. The auto-approval rules are simple frozenset membership checks that safely return False for None names or unrecognized tools. The middleware's _function_call_from_request guard (line 298 of _tool_approval.py) already filters out Content objects with name=None before passing to rules. The removal of require_delete_approval is a breaking API signature change for direct callers, but this is within the experimental harness surface and is flaged as intentional. No silent failures, lost errors, partial writes, or race conditions identified.

✗ Design Approach

I found two design-level problems. First, the constructor change removes a previously supported opt-out knob entirely, so existing callers that pass require_delete_approval=False now fail at construction time; that is a source-level API break, not just a tighter default. Second, the new static auto-approval callbacks match only on tool name, so they can also auto-approve hosted tools with the same names even though the approval stack elsewhere treats hosted tools as a separate server-scoped boundary that should be passed through untouched.

Flagged Issues

• python/packages/core/agent_framework/_harness/_file_access.py:1060 drops the public require_delete_approval keyword entirely, so existing code that still calls FileAccessProvider(store, require_delete_approval=False) will now raise TypeError during construction.
• python/packages/core/agent_framework/_harness/_file_access.py:1084 and :1105 implement auto-approval by tool name only, which can approve a hosted tool that happens to use the same name even though _tools.py:1930-1934 says hosted approvals should be passed through untouched and _harness/_tool_approval.py:315-318 already scopes standing approvals by server_label.

---

Automated review by westey-m's agents

[github-actions]

Flagged issue

python/packages/core/agent_framework/_harness/_file_access.py:1060 drops the public require_delete_approval keyword entirely, so existing code that still calls FileAccessProvider(store, require_delete_approval=False) will now raise TypeError during construction.

---

Source: automated DevFlow PR review

[github-actions]

Flagged issue

python/packages/core/agent_framework/_harness/_file_access.py:1084 and :1105 implement auto-approval by tool name only, which can approve a hosted tool that happens to use the same name even though _tools.py:1930-1934 says hosted approvals should be passed through untouched and _harness/_tool_approval.py:315-318 already scopes standing approvals by server_label.

---

Source: automated DevFlow PR review

[Copilot]

Pull request overview

Tightens the Python harness FileAccessProvider tool approval defaults so file-access operations require host approval by default, while offering explicit auto-approval rules for read-only or all file-access tools. This makes tool-approval behavior demonstrable and safer for human-in-the-loop harness scenarios.

Changes:

• Registers all FileAccessProvider tools with approval_mode="always_require" and introduces two static auto-approval rule helpers (read_only_tools_auto_approval_rule, all_tools_auto_approval_rule).
• Exposes file-access tool names as class constants for reuse in docs/tests and rules.
• Adds a new harness sample (with bundled CSV data) and updates docs/tests to reflect the new approval defaults.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
python/samples/02-agents/harness/working/sales.csv	Adds seed CSV data for the new harness sample.
python/samples/02-agents/harness/harness_data_processing.py	New sample demonstrating read-only auto-approval with interactive approval for writes.
python/packages/core/tests/core/test_harness_file_access.py	Updates expectations for approval defaults and adds tests for the new auto-approval rules/constants.
python/packages/core/AGENTS.md	Documents new default approval behavior and the new auto-approval rules/constants.
python/packages/core/agent_framework/_harness/_file_access.py	Implements always-require approval for all file tools, adds constants, and adds the auto-approval rule helpers.

Comments suppressed due to low confidence (1)

python/packages/core/agent_framework/_harness/_file_access.py:1138

• With approval_mode="always_require" on the file-access tools, callers that use FileAccessProvider with the base Agent (without ToolApprovalMiddleware or manually injecting function_approval_response messages) will no longer have these tools executed during auto-invocation. This breaks existing samples like python/samples/02-agents/context_providers/file_access_data_processing/data_processing.py (it uses Agent(...) directly). Please update those samples to either install ToolApprovalMiddleware (optionally with FileAccessProvider.read_only_tools_auto_approval_rule) or switch them to create_harness_agent where tool approval is wired by default.

        @tool(name=FileAccessProvider.SAVE_FILE_TOOL_NAME, approval_mode="always_require")
        async def file_access_save_file(file_name: str, content: str, overwrite: bool = False) -> str:
            """Save a file with the given name and content. By default, does not overwrite an existing file unless overwrite is set to true."""  # noqa: E501
            try:
                normalized = _normalize_relative_path(file_name)

[github-actions]

Automated Code Review

Reviewers: 5 | Confidence: 94%

✓ Correctness

The changes are logically sound. All six tools are switched to approval_mode="always_require" consistently, the two static auto-approval rules correctly gate on both tool name membership and the absence of a server_label (preventing accidental auto-approval of same-named hosted tools), the removed require_delete_approval parameter has no remaining references, and Content.additional_properties is always initialized to a dict so the .get("server_label") call is safe. Tests accurately exercise both rules including the hosted-tool rejection path. No correctness issues found.

✓ Security Reliability

This PR tightens the default approval mode for all FileAccessProvider tools from 'never_require' to 'always_require', which is a security improvement. The auto-approval rules correctly reject hosted-tool calls (those carrying a server_label) to prevent name-collision bypasses. The implementation is clean: the AprovalMode import removal is safe (no remaining uses), additional_properties always defaults to {} so .get() calls are safe, and the static method signatures match the ToolApprovalRuleCallback type. No security or reliability issues found.

✓ Test Coverage

Test coverage for this PR is solid. The three key behavioral changes—(1) all six tools now require approval, (2) read-only auto-approval rule, and (3) all-tools auto-approval rule—each have dedicated tests with positive, negative, and security-boundary (server_label) assertions. The removed require_delete_approval parameter has no remaining references. The existing round-trip tests and middleware integration test provide adequate coverage for the end-to-end flow. No significant test coverage gaps found.

✓ Failure Modes

This PR cleanly tightens file-access tool approval defaults and adds well-structured auto-approval rules. No silent failure modes, swallowed exceptions, or operational hazards were found. The Content.additional_properties is always a dict (never None), so the _is_local_tool_call guard is safe. The removed require_delete_approval parameter has no remaining calers. The auto-approval rules integrate correctly with the ToolApprovalMiddleware._matches_auto_rule flow, which properly extracts the inner function_call content before passing it to rule callbacks. The resolved review comments (docstring and server_label scoping) have been addressed in the current diff.

✓ Design Approach

The design is mostly coherent: the provider now consistently routes file operations through the approval middleware, and the hosted-tool scoping fix is in place. I found one design-level compatibility problem, though: the constructor no longer accepts the previously supported require_delete_approval keyword, so existing callers now fail at import-time invocation even though the PR rationale says this is not a breaking change.

---

Automated review by westey-m's agents