mirror of
https://github.com/microsoft/agent-framework.git
synced 2026-06-16 21:04:09 +08:00
Address review on Foundry Toolbox MCP samples
Reviewed feedback addressed: - Drop the branch-pinned `git+https://...@feature/...` entries from `04_foundry_toolbox/requirements.txt`; restore the simple comment + `mcp` runtime dep. The git pins were only useful while iterating on the PR and shouldn't ship. (eavanvalkenburg) - Fix the `/toolsets/` typo in both `04_foundry_toolbox/README.md` and `06_files/README.md`. Verified empirically against the research_toolbox in the test workspace: the toolbox MCP gateway lives at `/toolboxes/{name}/mcp?api-version=v1` and requires the `Foundry-Features: Toolboxes=V1Preview` header. `/toolsets/{name}/mcp` returns 403 with `preview_feature_required: Toolsets=V1Preview` (a different opt-in feature). - Wrap `httpx.AsyncClient(...)` in `async with ... as http_client:` in both samples so the connection pool is cleaned up. (Copilot reviewer) - Make the `TOOLBOX_NAME` env var consistent in both samples. Previously the tool name silently fell back to `"toolbox"` when `TOOLBOX_NAME` was unset, but `resolve_toolbox_endpoint()` still required `TOOLBOX_NAME` and would raise `KeyError`. The samples now resolve the endpoint once and derive the tool name from the resolved URL when `TOOLBOX_NAME` isn't set, so the local tool name always matches the upstream toolbox identity regardless of which env var the user set. (Copilot reviewer) - Rename `_responses.is_consent_error` to `consent_url_from_error`: the helper returns `str | None` (the consent URL), not a bool, so the new name matches behavior. Update the test class accordingly. (eavanvalkenburg) - Tighten `_handle_inner_agent`'s lazy-entry catch from `Exception` to `AgentFrameworkException`, the type the MCP layer actually wraps consent errors in via `MCPStreamableHTTPTool.__aenter__` → `ToolExecutionException(inner_exception=mcp_error)`. Network failures, cancellations, and other non-framework exceptions now propagate normally instead of being briefly caught and re-raised. The test helper `_make_consent_error` is updated to use `ToolExecutionException` so it matches the real-world wrapping. (eavanvalkenburg) - Clarify the `github_pat` description in `agent.manifest.yaml` to note it's only needed when the PAT-based connection (`github-mcp-pat-conn`) is chosen; users selecting the OAuth2 connection (`github-mcp-oauth-conn`) can leave it empty. (Copilot reviewer) Validation: ran both samples end-to-end against a real Foundry toolbox (`research_toolbox`) -- the samples connect successfully and the agent lists the toolbox's MCP tools (`api_specs___fetch_azure_rest_api_docs`, etc.). `uv run poe test -P foundry_hosting` passes (119 tests), pyright + mypy clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
committed by
eavanvalkenburg
Unverified
parent
483bfe386b
commit
7f51de9937
@@ -38,7 +38,7 @@ from agent_framework_foundry_hosting._responses import (
|
||||
InMemoryFunctionApprovalStorage, # pyright: ignore[reportPrivateUsage]
|
||||
_item_to_message, # pyright: ignore[reportPrivateUsage]
|
||||
_output_item_to_message, # pyright: ignore[reportPrivateUsage]
|
||||
is_consent_error,
|
||||
consent_url_from_error,
|
||||
)
|
||||
|
||||
|
||||
@@ -2896,30 +2896,39 @@ class TestCheckpointContextPathValidation:
|
||||
|
||||
|
||||
def _make_consent_error(url: str = "https://consent.example.com/auth") -> Exception:
|
||||
"""Build an exception wrapping a Foundry MCP gateway consent error."""
|
||||
"""Build an exception wrapping a Foundry MCP gateway consent error.
|
||||
|
||||
Mirrors the real-world wrapping produced by ``MCPStreamableHTTPTool.__aenter__``,
|
||||
which catches connection-time ``McpError``s and re-raises them as a
|
||||
``ToolExecutionException`` (an ``AgentFrameworkException`` subclass) with the
|
||||
original error attached via ``inner_exception``. ``consent_url_from_error``
|
||||
then finds the wrapped ``McpError`` in ``exc.args``.
|
||||
"""
|
||||
from agent_framework.exceptions import ToolExecutionException
|
||||
|
||||
inner = McpError(ErrorData(code=CONSENT_ERROR_CODE, message=url))
|
||||
return Exception("MCP consent required", inner)
|
||||
return ToolExecutionException("MCP consent required", inner_exception=inner)
|
||||
|
||||
|
||||
class TestIsConsentError:
|
||||
class TestConsentUrlFromError:
|
||||
def test_returns_consent_url_when_inner_arg_is_consent_mcp_error(self) -> None:
|
||||
exc = _make_consent_error("https://example.com/consent")
|
||||
assert is_consent_error(exc) == "https://example.com/consent"
|
||||
assert consent_url_from_error(exc) == "https://example.com/consent"
|
||||
|
||||
def test_returns_none_when_no_mcp_error_in_args(self) -> None:
|
||||
assert is_consent_error(Exception("boom")) is None
|
||||
assert consent_url_from_error(Exception("boom")) is None
|
||||
|
||||
def test_returns_none_when_mcp_error_has_different_code(self) -> None:
|
||||
inner = McpError(ErrorData(code=-32000, message="some other error"))
|
||||
exc = Exception("wrapped", inner)
|
||||
assert is_consent_error(exc) is None
|
||||
assert consent_url_from_error(exc) is None
|
||||
|
||||
def test_returns_none_for_bare_mcp_error_without_wrapping(self) -> None:
|
||||
# `args` of a bare McpError holds the message string, not an McpError
|
||||
# instance, so it does not match the wrapping pattern produced by the
|
||||
# MCP client when it bubbles consent errors up.
|
||||
bare = McpError(ErrorData(code=CONSENT_ERROR_CODE, message="https://x"))
|
||||
assert is_consent_error(bare) is None
|
||||
assert consent_url_from_error(bare) is None
|
||||
|
||||
|
||||
class TestAgentLifecycle:
|
||||
|
||||
Reference in New Issue
Block a user