From a2802480211a6b28f3c00c0ca9bbb2838503eba4 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Sat, 16 May 2026 19:47:51 +0300 Subject: [PATCH] [codex] Split Python SDK helper logic (#22939) ## Summary - Move approval-mode mapping into `sdk/python/src/openai_codex/_approval_mode.py`. - Move initialize metadata parsing and normalization into `sdk/python/src/openai_codex/_initialize_metadata.py`. - Keep the public `ApprovalMode` export stable and retarget direct metadata helper coverage. ## Integration coverage - Add an app-server harness smoke that exercises sync and async SDK initialization plus thread creation. ## Validation - Local tests were not run per repo guidance. CI should validate this branch once the PR is online. --- sdk/python/src/openai_codex/_approval_mode.py | 51 ++++++++ .../src/openai_codex/_initialize_metadata.py | 54 +++++++++ sdk/python/src/openai_codex/api.py | 110 ++---------------- sdk/python/tests/test_app_server_lifecycle.py | 55 +++++++++ .../tests/test_public_api_signatures.py | 5 +- 5 files changed, 173 insertions(+), 102 deletions(-) create mode 100644 sdk/python/src/openai_codex/_approval_mode.py create mode 100644 sdk/python/src/openai_codex/_initialize_metadata.py diff --git a/sdk/python/src/openai_codex/_approval_mode.py b/sdk/python/src/openai_codex/_approval_mode.py new file mode 100644 index 0000000000..bbb57030c0 --- /dev/null +++ b/sdk/python/src/openai_codex/_approval_mode.py @@ -0,0 +1,51 @@ +from __future__ import annotations + +from enum import Enum +from typing import NoReturn + +from .generated.v2_all import ( + ApprovalsReviewer, + AskForApproval, + AskForApprovalValue, +) + + +class ApprovalMode(str, Enum): + """High-level approval behavior for escalated permission requests.""" + + deny_all = "deny_all" + auto_review = "auto_review" + + +def _approval_mode_settings( + approval_mode: ApprovalMode, +) -> tuple[AskForApproval, ApprovalsReviewer | None]: + """Map the public approval mode to generated app-server start params.""" + if not isinstance(approval_mode, ApprovalMode): + supported = ", ".join(mode.value for mode in ApprovalMode) + raise ValueError(f"approval_mode must be one of: {supported}") + + match approval_mode: + case ApprovalMode.auto_review: + return ( + AskForApproval(root=AskForApprovalValue.on_request), + ApprovalsReviewer.auto_review, + ) + case ApprovalMode.deny_all: + return AskForApproval(root=AskForApprovalValue.never), None + case _: + return _assert_never_approval_mode(approval_mode) + + +def _assert_never_approval_mode(approval_mode: NoReturn) -> NoReturn: + """Make approval mode mapping exhaustive for static type checkers.""" + raise AssertionError(f"Unhandled approval mode: {approval_mode!r}") + + +def _approval_mode_override_settings( + approval_mode: ApprovalMode | None, +) -> tuple[AskForApproval | None, ApprovalsReviewer | None]: + """Map an optional public approval mode to app-server override params.""" + if approval_mode is None: + return None, None + return _approval_mode_settings(approval_mode) diff --git a/sdk/python/src/openai_codex/_initialize_metadata.py b/sdk/python/src/openai_codex/_initialize_metadata.py new file mode 100644 index 0000000000..48e5922c46 --- /dev/null +++ b/sdk/python/src/openai_codex/_initialize_metadata.py @@ -0,0 +1,54 @@ +from __future__ import annotations + +from .models import InitializeResponse, ServerInfo + + +def _split_user_agent(user_agent: str) -> tuple[str | None, str | None]: + raw = user_agent.strip() + if not raw: + return None, None + if "/" in raw: + name, version = raw.split("/", 1) + return (name or None), (version or None) + parts = raw.split(maxsplit=1) + if len(parts) == 2: + return parts[0], parts[1] + return raw, None + + +def validate_initialize_metadata(payload: InitializeResponse) -> InitializeResponse: + user_agent = (payload.userAgent or "").strip() + server = payload.serverInfo + + server_name: str | None = None + server_version: str | None = None + + if server is not None: + server_name = (server.name or "").strip() or None + server_version = (server.version or "").strip() or None + + if (server_name is None or server_version is None) and user_agent: + parsed_name, parsed_version = _split_user_agent(user_agent) + if server_name is None: + server_name = parsed_name + if server_version is None: + server_version = parsed_version + + normalized_server_name = (server_name or "").strip() + normalized_server_version = (server_version or "").strip() + if not user_agent or not normalized_server_name or not normalized_server_version: + raise RuntimeError( + "initialize response missing required metadata " + f"(user_agent={user_agent!r}, server_name={normalized_server_name!r}, server_version={normalized_server_version!r})" + ) + + if server is None: + payload.serverInfo = ServerInfo( + name=normalized_server_name, + version=normalized_server_version, + ) + else: + server.name = normalized_server_name + server.version = normalized_server_version + + return payload diff --git a/sdk/python/src/openai_codex/api.py b/sdk/python/src/openai_codex/api.py index c894447124..0ea99f9b3d 100644 --- a/sdk/python/src/openai_codex/api.py +++ b/sdk/python/src/openai_codex/api.py @@ -2,9 +2,14 @@ from __future__ import annotations import asyncio from dataclasses import dataclass -from enum import Enum -from typing import AsyncIterator, Iterator, NoReturn +from typing import AsyncIterator, Iterator +from ._approval_mode import ( + ApprovalMode as ApprovalMode, + _approval_mode_override_settings, + _approval_mode_settings, +) +from ._initialize_metadata import validate_initialize_metadata from ._inputs import ( ImageInput as ImageInput, Input, @@ -25,9 +30,6 @@ from ._run import ( from .async_client import AsyncAppServerClient from .client import AppServerClient, AppServerConfig from .generated.v2_all import ( - ApprovalsReviewer, - AskForApproval, - AskForApprovalValue, ModelListResponse, Personality, ReasoningEffort, @@ -55,61 +57,7 @@ from .generated.v2_all import ( TurnStartParams, TurnSteerResponse, ) -from .models import InitializeResponse, JsonObject, Notification, ServerInfo - - -def _split_user_agent(user_agent: str) -> tuple[str | None, str | None]: - raw = user_agent.strip() - if not raw: - return None, None - if "/" in raw: - name, version = raw.split("/", 1) - return (name or None), (version or None) - parts = raw.split(maxsplit=1) - if len(parts) == 2: - return parts[0], parts[1] - return raw, None - - -class ApprovalMode(str, Enum): - """High-level approval behavior for escalated permission requests.""" - - deny_all = "deny_all" - auto_review = "auto_review" - - -def _approval_mode_settings( - approval_mode: ApprovalMode, -) -> tuple[AskForApproval, ApprovalsReviewer | None]: - """Map the public approval mode to generated app-server start params.""" - if not isinstance(approval_mode, ApprovalMode): - supported = ", ".join(mode.value for mode in ApprovalMode) - raise ValueError(f"approval_mode must be one of: {supported}") - - match approval_mode: - case ApprovalMode.auto_review: - return ( - AskForApproval(root=AskForApprovalValue.on_request), - ApprovalsReviewer.auto_review, - ) - case ApprovalMode.deny_all: - return AskForApproval(root=AskForApprovalValue.never), None - case _: - return _assert_never_approval_mode(approval_mode) - - -def _assert_never_approval_mode(approval_mode: NoReturn) -> NoReturn: - """Make approval mode mapping exhaustive for static type checkers.""" - raise AssertionError(f"Unhandled approval mode: {approval_mode!r}") - - -def _approval_mode_override_settings( - approval_mode: ApprovalMode | None, -) -> tuple[AskForApproval | None, ApprovalsReviewer | None]: - """Map an optional public approval mode to app-server override params.""" - if approval_mode is None: - return None, None - return _approval_mode_settings(approval_mode) +from .models import InitializeResponse, JsonObject, Notification class Codex: @@ -119,7 +67,7 @@ class Codex: self._client = AppServerClient(config=config) try: self._client.start() - self._init = self._validate_initialize(self._client.initialize()) + self._init = validate_initialize_metadata(self._client.initialize()) except Exception: self._client.close() raise @@ -130,44 +78,6 @@ class Codex: def __exit__(self, _exc_type, _exc, _tb) -> None: self.close() - @staticmethod - def _validate_initialize(payload: InitializeResponse) -> InitializeResponse: - user_agent = (payload.userAgent or "").strip() - server = payload.serverInfo - - server_name: str | None = None - server_version: str | None = None - - if server is not None: - server_name = (server.name or "").strip() or None - server_version = (server.version or "").strip() or None - - if (server_name is None or server_version is None) and user_agent: - parsed_name, parsed_version = _split_user_agent(user_agent) - if server_name is None: - server_name = parsed_name - if server_version is None: - server_version = parsed_version - - normalized_server_name = (server_name or "").strip() - normalized_server_version = (server_version or "").strip() - if not user_agent or not normalized_server_name or not normalized_server_version: - raise RuntimeError( - "initialize response missing required metadata " - f"(user_agent={user_agent!r}, server_name={normalized_server_name!r}, server_version={normalized_server_version!r})" - ) - - if server is None: - payload.serverInfo = ServerInfo( - name=normalized_server_name, - version=normalized_server_version, - ) - else: - server.name = normalized_server_name - server.version = normalized_server_version - - return payload - @property def metadata(self) -> InitializeResponse: return self._init @@ -354,7 +264,7 @@ class AsyncCodex: try: await self._client.start() payload = await self._client.initialize() - self._init = Codex._validate_initialize(payload) + self._init = validate_initialize_metadata(payload) self._initialized = True except Exception: await self._client.close() diff --git a/sdk/python/tests/test_app_server_lifecycle.py b/sdk/python/tests/test_app_server_lifecycle.py index 0baddccc57..77ba269450 100644 --- a/sdk/python/tests/test_app_server_lifecycle.py +++ b/sdk/python/tests/test_app_server_lifecycle.py @@ -39,6 +39,61 @@ def test_thread_set_name_and_read(tmp_path) -> None: } +def test_sync_and_async_initialization_round_trip_metadata(tmp_path) -> None: + """Public clients should initialize and start threads through app-server.""" + + async def async_scenario(harness: AppServerHarness) -> dict[str, object]: + async with AsyncCodex(config=harness.app_server_config()) as codex: + thread = await codex.thread_start() + server = codex.metadata.serverInfo + return { + "thread_id": thread.id, + "user_agent": codex.metadata.userAgent, + "server_name": None if server is None else server.name, + "server_version": None if server is None else server.version, + } + + with AppServerHarness(tmp_path) as harness: + with Codex(config=harness.app_server_config()) as codex: + thread = codex.thread_start() + server = codex.metadata.serverInfo + sync_summary = { + "thread_id": thread.id, + "user_agent": codex.metadata.userAgent, + "server_name": None if server is None else server.name, + "server_version": None if server is None else server.version, + } + async_summary = asyncio.run(async_scenario(harness)) + + assert { + "sync": { + "thread_id_present": bool(sync_summary["thread_id"]), + "user_agent_present": bool(sync_summary["user_agent"]), + "server_name_present": bool(sync_summary["server_name"]), + "server_version_present": bool(sync_summary["server_version"]), + }, + "async": { + "thread_id_present": bool(async_summary["thread_id"]), + "user_agent_present": bool(async_summary["user_agent"]), + "server_name_present": bool(async_summary["server_name"]), + "server_version_present": bool(async_summary["server_version"]), + }, + } == { + "sync": { + "thread_id_present": True, + "user_agent_present": True, + "server_name_present": True, + "server_version_present": True, + }, + "async": { + "thread_id_present": True, + "user_agent_present": True, + "server_name_present": True, + "server_version_present": True, + }, + } + + def test_thread_list_filters_archived_threads(tmp_path) -> None: """Thread listing should reflect archive state through app-server.""" with AppServerHarness(tmp_path) as harness: diff --git a/sdk/python/tests/test_public_api_signatures.py b/sdk/python/tests/test_public_api_signatures.py index f38c2b8c94..f13fb35c08 100644 --- a/sdk/python/tests/test_public_api_signatures.py +++ b/sdk/python/tests/test_public_api_signatures.py @@ -18,6 +18,7 @@ from openai_codex import ( RunResult, Thread, ) +from openai_codex._initialize_metadata import validate_initialize_metadata from openai_codex.types import InitializeResponse EXPECTED_ROOT_EXPORTS = [ @@ -444,7 +445,7 @@ def test_lifecycle_methods_are_codex_scoped() -> None: def test_initialize_metadata_parses_user_agent_shape() -> None: """Initialize metadata should accept the legacy user-agent-only payload shape.""" payload = InitializeResponse.model_validate({"userAgent": "codex-cli/1.2.3"}) - parsed = Codex._validate_initialize(payload) + parsed = validate_initialize_metadata(payload) assert parsed is payload assert parsed.userAgent == "codex-cli/1.2.3" assert parsed.serverInfo is not None @@ -455,7 +456,7 @@ def test_initialize_metadata_parses_user_agent_shape() -> None: def test_initialize_metadata_requires_non_empty_information() -> None: """Initialize metadata should fail when the runtime gives no identity signal.""" try: - Codex._validate_initialize(InitializeResponse.model_validate({})) + validate_initialize_metadata(InitializeResponse.model_validate({})) except RuntimeError as exc: assert "missing required metadata" in str(exc) else: