From 2054d62702e65d547c1ea337b7d6d803a06c2707 Mon Sep 17 00:00:00 2001 From: Ruiming Zhao Date: Thu, 20 Aug 2026 08:33:46 +0000 Subject: [PATCH] Python: fix(github-copilot): forward telemetry config to client (#7625) * fix(github-copilot): forward telemetry config to client * Python: fix telemetry settings typing for github_copilot `load_settings` does not coerce dict-typed fields, so GITHUB_COPILOT_TELEMETRY and .env values reach the agent as plain strings. Declaring `GitHubCopilotSettings.telemetry` as `dict[str, Any]` therefore misstated the runtime contract and failed the test typing checks where a string is assigned. Widen the annotation to `dict[str, Any] | str | None` and fix the union arm resolution in `_check_override_type`: parameterized generics are not `type` instances, so they were dropped from the allowed set and a valid dict override was rejected at runtime. Arms without a runtime class, such as `Literal`, now skip validation instead of narrowing it incorrectly. Also drive the telemetry string tests through the documented environment variable path rather than mutating `_settings` directly, and cover the valid-JSON-but-not-an-object case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 657d2953-4112-4a01-889b-c0c6863630c1 * Python: resolve settings override types through generic origins Python 3.10 reports parameterized generics such as `dict[str, Any]` as instances of `type`, so the union arm resolution kept the alias and `isinstance` raised `TypeError: isinstance() argument 2 cannot be a parameterized generic` on that interpreter. Resolve every annotation through `get_origin` first via a shared `_runtime_class` helper, which also removes the same latent failure for a non-union parameterized generic field, and return `None` for annotations such as `Literal[...]` that have no runtime class so validation is skipped rather than narrowed incorrectly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 657d2953-4112-4a01-889b-c0c6863630c1 --------- Co-authored-by: Giles Odigwe Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 657d2953-4112-4a01-889b-c0c6863630c1 --- .../core/agent_framework/_settings.py | 34 ++++++- .../packages/core/tests/core/test_settings.py | 41 +++++++- .../agent_framework_github_copilot/_agent.py | 53 ++++++++++- .../tests/test_github_copilot_agent.py | 93 ++++++++++++++++++- 4 files changed, 212 insertions(+), 9 deletions(-) diff --git a/python/packages/core/agent_framework/_settings.py b/python/packages/core/agent_framework/_settings.py index c2d0b3590..60b8e3135 100644 --- a/python/packages/core/agent_framework/_settings.py +++ b/python/packages/core/agent_framework/_settings.py @@ -115,6 +115,19 @@ def _coerce_value(value: str, target_type: type) -> Any: return value +def _runtime_class(annotation: Any) -> type | None: + """Return the class ``isinstance`` can test *annotation* against, or ``None``. + + Parameterized generics such as ``dict[str, Any]`` cannot be passed to ``isinstance`` + and are instances of ``type`` on Python 3.10 but not on later versions, so the origin + is always preferred. Annotations without a runtime class, such as ``Literal[...]``, + return ``None`` so callers can skip validation instead of guessing. + """ + origin = get_origin(annotation) + candidate = annotation if origin is None else origin + return candidate if isinstance(candidate, type) else None + + def _check_override_type(value: Any, field_type: type, field_name: str) -> None: """Validate that *value* is compatible with *field_type*. @@ -135,14 +148,27 @@ def _check_override_type(value: Any, field_type: type, field_name: str) -> None: allowed: tuple[type, ...] if origin is Union or origin is type(int | str): - allowed = tuple(a for a in args if isinstance(a, type) and a is not type(None)) # If any arm is a Callable, allow anything callable if any(get_origin(a) is Callable or a is Callable for a in args): return - elif isinstance(field_type, type): - allowed = (field_type,) + resolved: list[type] = [] + for arm in args: + if arm is type(None): + continue + # ``isinstance`` rejects parameterized generics, and on Python 3.10 they are + # themselves instances of ``type``, so resolve through the origin first. + runtime_type = _runtime_class(arm) + if runtime_type is None: + # An arm such as ``Literal[...]`` has no runtime class to test against; + # checking the remaining arms would reject values the annotation allows. + return + resolved.append(runtime_type) + allowed = tuple(resolved) else: - return # complex / unknown annotation — skip check + field_class = _runtime_class(field_type) + if field_class is None: + return # complex / unknown annotation — skip check + allowed = (field_class,) if not allowed: return diff --git a/python/packages/core/tests/core/test_settings.py b/python/packages/core/tests/core/test_settings.py index 1d3c5a5d7..58d21a397 100644 --- a/python/packages/core/tests/core/test_settings.py +++ b/python/packages/core/tests/core/test_settings.py @@ -4,7 +4,7 @@ import os import tempfile -from typing import TypedDict +from typing import Any, Literal, TypedDict import pytest @@ -261,6 +261,45 @@ class TestOverrideTypeValidation: assert isinstance(settings["api_key"], SecretString) assert settings["api_key"] == "plain-string" + def test_parameterized_generic_union_arm_accepted(self) -> None: + """A ``dict`` override is valid for ``dict[str, Any] | str | None``.""" + + class GenericUnionSettings(TypedDict, total=False): + config: dict[str, Any] | str | None + + settings = load_settings(GenericUnionSettings, env_prefix="TEST_", config={"key": "value"}) + + assert settings["config"] == {"key": "value"} + + def test_parameterized_generic_union_arm_rejects_unrelated_type(self) -> None: + class GenericUnionSettings(TypedDict, total=False): + config: dict[str, Any] | str | None + + with pytest.raises(ValueError, match="Invalid type for setting 'config'"): + load_settings(GenericUnionSettings, env_prefix="TEST_", config=1.5) + + def test_bare_parameterized_generic_field(self) -> None: + """A non-union ``dict[str, Any]`` is validated against its origin, not the alias.""" + + class GenericSettings(TypedDict, total=False): + config: dict[str, Any] + + settings = load_settings(GenericSettings, env_prefix="TEST_", config={"key": "value"}) + assert settings["config"] == {"key": "value"} + + with pytest.raises(ValueError, match="Invalid type for setting 'config'"): + load_settings(GenericSettings, env_prefix="TEST_", config=1.5) + + def test_union_with_literal_arm_skips_check(self) -> None: + """``Literal`` arms have no runtime class, so validation is skipped rather than wrong.""" + + class LiteralUnionSettings(TypedDict, total=False): + mode: Literal["all"] | list[str] | None + + settings = load_settings(LiteralUnionSettings, env_prefix="TEST_", mode=["a", "b"]) + + assert settings["mode"] == ["a", "b"] + class TestMutuallyExclusive: """Test mutually exclusive field validation via tuple entries in required_fields.""" diff --git a/python/packages/github_copilot/agent_framework_github_copilot/_agent.py b/python/packages/github_copilot/agent_framework_github_copilot/_agent.py index 726a345bf..2f7b963cb 100644 --- a/python/packages/github_copilot/agent_framework_github_copilot/_agent.py +++ b/python/packages/github_copilot/agent_framework_github_copilot/_agent.py @@ -5,6 +5,7 @@ from __future__ import annotations import asyncio import contextlib import inspect +import json import logging import sys import warnings @@ -52,7 +53,12 @@ else: from typing_extensions import TypeVar # pragma: no cover try: - from copilot import CopilotClient, CopilotSession, RuntimeConnection + from copilot import ( + CopilotClient, + CopilotSession, + RuntimeConnection, + TelemetryConfig, + ) from copilot.generated.rpc import ( PermissionDecisionApproveForSession, PermissionDecisionApproveForSessionApproval, @@ -338,6 +344,26 @@ def _with_normalized_permission_decisions(handler: PermissionHandlerType) -> Asy return normalized_handler +def _parse_telemetry_config(raw: str) -> TelemetryConfig | None: + # GITHUB_COPILOT_TELEMETRY and matching .env values are read as plain strings while the + # Copilot SDK expects a mapping, so parse here before the value reaches CopilotClient. + # Malformed values are logged and ignored so a bad telemetry setting cannot prevent the + # agent from starting. + try: + parsed = json.loads(raw) + except json.JSONDecodeError: + logger.warning( + "Ignoring malformed GITHUB_COPILOT_TELEMETRY value; expected a JSON object with TelemetryConfig keys." + ) + return None + if not isinstance(parsed, dict): + logger.warning( + "Ignoring invalid GITHUB_COPILOT_TELEMETRY value; expected a JSON object with TelemetryConfig keys." + ) + return None + return cast(TelemetryConfig, parsed) + + class GitHubCopilotSettings(TypedDict, total=False): """GitHub Copilot model settings. @@ -359,6 +385,10 @@ class GitHubCopilotSettings(TypedDict, total=False): GITHUB_COPILOT_BASE_DIRECTORY. Defaults to ~/.copilot when not set. Only applicable when the SDK spawns the CLI process (ignored when connecting to an external server via a pre-configured client). + telemetry: OpenTelemetry configuration for the Copilot CLI process. This is + passed to the SDK client when it is created by the agent. Values coming + from GITHUB_COPILOT_TELEMETRY or a .env file arrive as a JSON string and + are parsed into a mapping before they reach the SDK. """ cli_path: str | None @@ -366,6 +396,7 @@ class GitHubCopilotSettings(TypedDict, total=False): timeout: float | None log_level: str | None base_directory: str | None + telemetry: dict[str, Any] | str | None class GitHubCopilotOptions(TypedDict, total=False): @@ -437,6 +468,9 @@ class GitHubCopilotOptions(TypedDict, total=False): base_directory: str """Directory where the CLI stores session state, configuration, and other persistent data.""" + telemetry: TelemetryConfig + """OpenTelemetry configuration for the Copilot CLI process.""" + on_pre_tool_use: PreToolUseHandler """Pre-tool-use hook handler for the Copilot SDK. @@ -574,6 +608,7 @@ class RawGitHubCopilotAgent(BaseAgent, Generic[OptionsT]): on_pre_tool_use: PreToolUseHandler | None = opts.pop("on_pre_tool_use", None) on_function_approval: FunctionApprovalCallback | None = opts.pop("on_function_approval", None) base_directory = opts.pop("base_directory", None) + telemetry = opts.pop("telemetry", None) if on_function_approval is not None and on_pre_tool_use is not None: raise ValueError( @@ -600,6 +635,7 @@ class RawGitHubCopilotAgent(BaseAgent, Generic[OptionsT]): timeout=timeout, log_level=log_level, base_directory=base_directory, + telemetry=telemetry, env_file_path=env_file_path, env_file_encoding=env_file_encoding, ) @@ -640,6 +676,9 @@ class RawGitHubCopilotAgent(BaseAgent, Generic[OptionsT]): cli_path = self._settings.get("cli_path") or None log_level = self._settings.get("log_level") or None base_directory = self._settings.get("base_directory") or None + telemetry = self._settings.get("telemetry") or None + if isinstance(telemetry, str): + telemetry = _parse_telemetry_config(telemetry) client_kwargs: dict[str, Any] = {} if cli_path: @@ -648,6 +687,8 @@ class RawGitHubCopilotAgent(BaseAgent, Generic[OptionsT]): client_kwargs["log_level"] = log_level if base_directory: client_kwargs["base_directory"] = base_directory + if telemetry: + client_kwargs["telemetry"] = telemetry self._client = CopilotClient(**client_kwargs) try: @@ -1434,7 +1475,15 @@ class RawGitHubCopilotAgent(BaseAgent, Generic[OptionsT]): # Strip agent-internal and client-level keys that are consumed here or in the # run methods (and settings) but are NOT valid create_session parameters, so # they don't leak through the passthrough layer and raise TypeError. - for key in ("on_pre_tool_use", "on_function_approval", "timeout", "cli_path", "log_level", "base_directory"): + for key in ( + "on_pre_tool_use", + "on_function_approval", + "timeout", + "cli_path", + "log_level", + "base_directory", + "telemetry", + ): kwargs.pop(key, None) return kwargs diff --git a/python/packages/github_copilot/tests/test_github_copilot_agent.py b/python/packages/github_copilot/tests/test_github_copilot_agent.py index 8d65c2ca8..6e17db2f7 100644 --- a/python/packages/github_copilot/tests/test_github_copilot_agent.py +++ b/python/packages/github_copilot/tests/test_github_copilot_agent.py @@ -4,6 +4,7 @@ import base64 import inspect +import json import os import unittest.mock from collections.abc import Sequence @@ -420,6 +421,76 @@ class TestGitHubCopilotAgentLifecycle: kwargs = MockClient.call_args.kwargs assert kwargs["base_directory"] == "/custom/copilot/home" + async def test_start_passes_telemetry_to_client(self) -> None: + """Test that telemetry settings are passed to the Copilot client.""" + telemetry = { + "exporter_type": "otlp-http", + "otlp_endpoint": "http://localhost:4318", + "otlp_protocol": "http/json", + "capture_content": True, + } + with patch("agent_framework_github_copilot._agent.CopilotClient") as MockClient: + mock_client = MagicMock() + mock_client.start = AsyncMock() + MockClient.return_value = mock_client + + agent = GitHubCopilotAgent( + default_options=copilot_options(cast(GitHubCopilotOptions, {"telemetry": telemetry})) + ) + await agent.start() + + assert MockClient.call_args.kwargs["telemetry"] == telemetry + + async def test_start_parses_json_telemetry_string(self) -> None: + """JSON strings from env/.env settings are parsed before reaching the client.""" + telemetry = { + "exporter_type": "otlp-http", + "otlp_endpoint": "http://localhost:4318", + "capture_content": True, + } + with ( + patch("agent_framework_github_copilot._agent.CopilotClient") as MockClient, + patch.dict("os.environ", {"GITHUB_COPILOT_TELEMETRY": json.dumps(telemetry)}), + ): + mock_client = MagicMock() + mock_client.start = AsyncMock() + MockClient.return_value = mock_client + + agent = GitHubCopilotAgent() + await agent.start() + + assert MockClient.call_args.kwargs["telemetry"] == telemetry + + async def test_start_ignores_malformed_telemetry_string(self) -> None: + """A malformed telemetry JSON value is dropped instead of breaking startup.""" + with ( + patch("agent_framework_github_copilot._agent.CopilotClient") as MockClient, + patch.dict("os.environ", {"GITHUB_COPILOT_TELEMETRY": "{not json"}), + ): + mock_client = MagicMock() + mock_client.start = AsyncMock() + MockClient.return_value = mock_client + + agent = GitHubCopilotAgent() + await agent.start() + + assert "telemetry" not in MockClient.call_args.kwargs + + async def test_start_ignores_non_object_telemetry_string(self) -> None: + """Valid JSON that is not an object cannot be a TelemetryConfig and is dropped.""" + with ( + patch("agent_framework_github_copilot._agent.CopilotClient") as MockClient, + patch.dict("os.environ", {"GITHUB_COPILOT_TELEMETRY": "[1, 2]"}), + ): + mock_client = MagicMock() + mock_client.start = AsyncMock() + MockClient.return_value = mock_client + + agent = GitHubCopilotAgent() + await agent.start() + + assert "telemetry" not in MockClient.call_args.kwargs + async def test_start_base_directory_not_set_when_unspecified(self) -> None: """Test that base_directory is not included in client kwargs when not specified.""" with patch("agent_framework_github_copilot._agent.CopilotClient") as MockClient: @@ -1927,10 +1998,28 @@ class TestGitHubCopilotAgentOptionsPassthrough: agent = GitHubCopilotAgent(client=mock_client) # timeout and on_pre_tool_use are consumed by the agent, not create_session. - await agent.run("hello", options=cast(Any, {"timeout": 30, "on_pre_tool_use": runtime_hook})) + await agent.run( + "hello", + options=cast( + Any, + { + "timeout": 30, + "on_pre_tool_use": runtime_hook, + "telemetry": {"exporter_type": "file", "file_path": "/tmp/copilot.jsonl"}, + }, + ), + ) config = mock_client.create_session.call_args.kwargs - for leaked in ("timeout", "on_pre_tool_use", "on_function_approval", "cli_path", "log_level", "base_directory"): + for leaked in ( + "timeout", + "on_pre_tool_use", + "on_function_approval", + "cli_path", + "log_level", + "base_directory", + "telemetry", + ): assert leaked not in config # on_pre_tool_use is still honored via the hooks parameter. assert config["hooks"]["on_pre_tool_use"] is runtime_hook