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 <gilesodigwe@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 657d2953-4112-4a01-889b-c0c6863630c1
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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."""
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user