Python: Add second approval-required tool (set_stop_loss) to concurrent_builder_tool_approval sample

[moonbox3]

Motivation and Context

The concurrent_builder_tool_approval sample only demonstrated a single tool requiring approval (execute_trade), which didn't fully showcase how concurrent workflows handle approval requests for multiple different tools simultaneously. Adding a second approval-required tool makes the sample more realistic and informative.

Fixes #4874

Description

A new set_stop_loss tool with approval_mode="always_require" is added to the sample alongside the existing execute_trade tool. Both agents' instructions and tool lists are updated to include the new tool, and the task prompt now requests stop-loss orders. The expected output comments are updated to reflect the additional approval requests, demonstrating that concurrent workflows can independently handle multiple approval-requiring tools across parallel agents.

Contribution Checklist

• The code builds clean without any errors or warnings
• The PR follows the Contribution Guidelines
• All unit tests pass, and I have added new tests where possible
• Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.

---

Note: PR autogenerated by moonbox3's agent

[moonbox3]

Automated Code Review

Reviewers: 4 | Confidence: 91%

✓ Correctness

This PR adds a new set_stop_loss tool with approval_mode="always_require" to the concurrent builder tool approval sample. The new tool follows the same patterns as the existing execute_trade tool, uses valid Annotated[float, ...] parameter typing, and is correctly wired into both agents' tool lists. The instructions, prompts, and sample output documentation are all updated consistently. No logical, API usage, or correctness issues found.

✓ Security Reliability

This is a straightforward sample file change that adds a new set_stop_loss mock tool function with approval_mode="always_require" to demonstrate multiple tools requiring approval in a concurrent workflow. The new tool follows the same patterns as the existing execute_trade tool. All functions are mocks returning formatted strings with no real side effects. The approval flow in process_event_stream handles the new tool generically via the existing function_approval_request type check. No security or reliability issues found.

✓ Test Coverage

This PR modifies only a sample file (not library code) to add a second approval-required tool (set_stop_loss) to demonstrate handling approval requests for different tools in concurrent workflows. The new set_stop_loss tool function is trivial (string formatting), and the underlying framework behaviors it exercises—``@tool(approval_mode="always_require") and `ConcurrentBuilder` concurrent approval handling—are already covered by existing unit tests in `test_concurrent.py`, `test_tools.py`, `test_function_invocation_logic.py`, and others. No new library-level behavior is introduced, so no new tests are needed.

✓ Design Approach

This PR adds a second approval-required tool (set_stop_loss) to the concurrent workflow sample to better demonstrate multiple approval-required tools working in parallel. The change is purely additive and well-scoped: the new tool is correctly decorated with approval_mode="always_require", added to both agents, and the documentation/comments are updated consistently. There is one pre-existing inconsistency (not introduced here) worth noting: the sample output comment shows "Approval requested for tool: ..." and "Arguments: ..." lines, but process_event_stream in this file does not actually print those messages (it only prints "Simulating human approval for: .."). The PR updates the sample output comment to add more of these phantom lines without fixing the underlying gap between documented output and actual code behavior, which could mislead developers trying to understand the sample.

Suggestions

• The sample output comment (lines 200–228) shows "Approval requested for tool:" and "Arguments: ..." lines that process_event_stream does not actually print — only "Simulating human approval for:" is emitted. The sequential sample (sequential_builder_tool_approval.py) correctly prints [APPROVAL REQUIRED], tool name, and arguments before the simulated approval. Consider aligning process_event_stream in this file to also print the tool name and arguments when collecting approval requests, so the sample output comment matches the real output.

---

Automated review by moonbox3's agents

[Copilot]

Pull request overview

Enhances the Python concurrent_builder_tool_approval workflow sample to demonstrate concurrent approval handling across multiple distinct approval-required tools, making the scenario closer to real trading/risk-management flows.

Changes:

• Added a new approval-required tool, set_stop_loss, alongside execute_trade.
• Updated both agents’ instructions and tool lists to include stop-loss usage.
• Updated the task prompt and the sample’s expected output narrative to reflect stop-loss approval requests.

[moonbox3]

Automated Code Review

Reviewers: 4 | Confidence: 92%

✓ Correctness

This is a straightforward sample code enhancement that adds informative print statements when tool approval requests are received during concurrent workflow processing. The access pattern (event.data.function_call.name and event.data.function_call.arguments) is correct: when Content.type == "function_approval_request", the function_call attribute is always a Content object of type function_call which has name and arguments attributes. The # type: ignore comments are appropriate since the type checker can't narrow function_call from Content | None based on the runtime type check. The existing code at lines 138-139 already uses the same access pattern without issue.

✓ Security Reliability

The change adds two print statements to a sample script that display the tool name and arguments from a function approval request as it arrives in the event stream. This is purely a sample/demo enhancement for better user experience during the approval flow. The # type: ignore comments are consistent with the existing pattern at lines 139 (already present in the file). The function_call attribute on Content is a well-defined field (line 508/561 in _types.py), and the guard event.data.type == "function_approval_request" ensures the field is populated before access. No security, reliability, or correctness issues are present.

✓ Test Coverage

This PR adds three lines of print output to a sample script (concurrent_builder_tool_approval.py) to display tool name and arguments when an approval request is received. The change is cosmetic console output in a sample file, not in library/package code. No behavioral logic is introduced or modified — only additional print statements inside an existing conditional branch. Sample files in this repo do not have unit tests, and adding tests for print-statement-only changes in samples would provide negligible value. The sample output docstring at the bottom of the file has already been updated in a prior version to reflect this output, so the code and documentation are consistent.

✓ Design Approach

The diff adds two print statements inside the event-stream processor to surface tool name and arguments at the moment an approval request is received, rather than only at the later 'simulating approval' step. This is a sample file, and the output matches what the existing docstring already documents. The approach is correct and consistent with the surrounding code. The # type: ignore comments are consistent with the existing pattern at line 139 where request.function_call.name is accessed the same way. Since from_function_approval_request always sets function_call as a non-optional parameter, the runtime risk is low. No fundamental design issues exist.

Suggestions

• The function_call field is typed as Content | None, so accessing .name and .arguments without a None-guard could raise AttributeError if the content were constructed via a path other than from_function_approval_request. Adding if event.data.function_call is not None: around lines 130-131 (and removing the # type: ignore comments) would be safer, more idiomatic, and consistent with what the type system expects.

---

Automated review by moonbox3's agents

[markwallace-microsoft]

Python Test Coverage Report •

File	Stmts	Miss	Cover	Missing
TOTAL	28982	3476	88%

report-only-changed-files is enabled. No files were changed during this commit :)

Python Unit Test Overview

Tests	Skipped	Failures	Errors	Time
5753	30 💤	0 ❌	0 🔥	1m 35s ⏱️

[moonbox3]

Automated Code Review

Reviewers: 4 | Confidence: 89%

✓ Correctness

Clean, correct change. The added request.function_call is not None guard narows the type from Content | None to Content, which properly eliminates the need for the # type: ignore comments. The factory method from_function_approval_request always sets function_call to a non-None value, so the guard won't change runtime behavior in practice, but it's more defensive and type-safe. No correctness issues found.

✓ Security Reliability

The diff adds a defensive None check on request.function_call before accessing its attributes, replacing # type: ignore suppressions with a proper runtime guard. This is a sound reliability improvement that prevents AttributeError if a malformed function_approval_request arrives with a None function_call. The underlying to_function_approval_response method in the core library also passes self.function_call through with # type: ignore, meaning the None would propagate silently without this guard. However, when function_call is None, the sample now silently skips the approval request without producing a response entry, which could cause the workflow to hang waiting for a response that never comes. This is a minor concern for sample code but worth noting.

✓ Test Coverage

This PR adds a None-guard (and request.function_call is not None) to two sample files before accessing function_call attributes, and removes the now-unnecessary # type: ignore comments. The underlying Content.function_call field is typed as Content | None, so this change is correct for type safety. A corresponding unit test (test_function_approval_request_function_call_none_guard in test_types.py) already exists that validates the None-guard pattern, covering both the None case and the normal case with attributes. Since these are sample files (not library code), the existing library-level tests adequately cover the underlying behavior. No new tests are needed for this change.

✓ Design Approach

The PR removes # type: ignore suppressors by adding an is not None guard before accessing request.function_call. This is a valid fix: Content.__init__ types function_call as Optional[Content], so the type-checker correctly flags unguarded access. The concurrent sample already uses this exact pattern, confirming this is the intended idiom. The from_function_approval_request factory does require a non-None function_call, so this state should never occur via the public API. The only design concern is that when type == 'function_approval_request' but function_call is None (e.g., from a deserialization edge case), both samples silently skip the request, leaving it unanswered in responses — which could hang the workflow. A warning log in an else branch would surface this as a framework bug rather than a silent no-op. That said, since this is sample/demo code and not framework logic, this is a minor concern rather than a blocking issue.

Suggestions

• When type == 'function_approval_request' but function_call is None, both samples silently skip the request without adding a response entry, which could hang the workflow. Consider adding an else branch with a warning log (e.g., print(f'WARNING: function_approval_request {request_id} has no function_call, skipping')) so this edge case is detectable rather than a silent no-op.
• Longer-term, the root cause is that Content uses a monolithic class with a single function_call: Content | None field for all type variants (the recurring # type: ignore in to_function_approval_response at line 1238 of _types.py exposes the same gap). A discriminated union or per-type dataclasses would eliminate this class of type suppressions entirely.

---

Automated review by moonbox3's agents

[eavanvalkenburg]

@moonbox3 you have the approvals, but might need a more carefull rebase before merging...