Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions python/packages/core/agent_framework/_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -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*.

Expand All @@ -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
Comment thread
giles17 marked this conversation as resolved.
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
Expand Down
41 changes: 40 additions & 1 deletion python/packages/core/tests/core/test_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import os
import tempfile
from typing import TypedDict
from typing import Any, Literal, TypedDict

import pytest

Expand Down Expand Up @@ -261,6 +261,45 @@ def test_str_accepted_for_secretstring(self) -> None:
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."""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import asyncio
import contextlib
import inspect
import json
import logging
import sys
import warnings
Expand Down Expand Up @@ -52,7 +53,12 @@
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,
Expand Down Expand Up @@ -338,6 +344,26 @@ async def normalized_handler(request: PermissionRequest, invocation: dict[str, s
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.

Expand All @@ -359,13 +385,18 @@ 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
model: str | None
timeout: float | None
log_level: str | None
base_directory: str | None
telemetry: dict[str, Any] | str | None


class GitHubCopilotOptions(TypedDict, total=False):
Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -574,6 +608,7 @@ def __init__(
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(
Expand All @@ -600,6 +635,7 @@ def __init__(
timeout=timeout,
log_level=log_level,
base_directory=base_directory,
telemetry=telemetry,
env_file_path=env_file_path,
env_file_encoding=env_file_encoding,
)
Expand Down Expand Up @@ -640,6 +676,9 @@ async def start(self) -> None:
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:
Expand All @@ -648,6 +687,8 @@ async def start(self) -> None:
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:
Expand Down Expand Up @@ -1434,7 +1475,15 @@ def _build_session_kwargs(
# 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import base64
import inspect
import json
import os
import unittest.mock
from collections.abc import Sequence
Expand Down Expand Up @@ -420,6 +421,76 @@ async def test_start_passes_base_directory_to_client(self) -> None:
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:
Expand Down Expand Up @@ -1927,10 +1998,28 @@ def runtime_hook(_input: Any, _context: Any) -> Any:

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
Expand Down
Loading