From ec5e91f03625768d424bc287e1dc45e1221907d2 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Mon, 8 Jun 2026 12:23:25 -0700 Subject: [PATCH] Address Python goal review feedback --- sdk/python/src/openai_codex/api.py | 4 +++ sdk/python/src/openai_codex/async_client.py | 24 +++++++++----- .../tests/test_app_server_goal_turns.py | 31 ++++++++++++++++++- 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/sdk/python/src/openai_codex/api.py b/sdk/python/src/openai_codex/api.py index 9f37bfd2f2..2971f8d1ac 100644 --- a/sdk/python/src/openai_codex/api.py +++ b/sdk/python/src/openai_codex/api.py @@ -78,6 +78,8 @@ from .generated.v2_all import ( ) from .models import InitializeResponse, JsonObject, Notification +_MAX_GOAL_OBJECTIVE_CHARS = 4_000 + def _normalize_goal_objective(objective: str) -> str: if not isinstance(objective, str): @@ -85,6 +87,8 @@ def _normalize_goal_objective(objective: str) -> str: objective = objective.strip() if not objective: raise ValueError("goal objective must not be empty") + if len(objective) > _MAX_GOAL_OBJECTIVE_CHARS: + raise ValueError(f"goal objective must be at most {_MAX_GOAL_OBJECTIVE_CHARS} characters") return objective diff --git a/sdk/python/src/openai_codex/async_client.py b/sdk/python/src/openai_codex/async_client.py index 10f4def5f5..e7028727af 100644 --- a/sdk/python/src/openai_codex/async_client.py +++ b/sdk/python/src/openai_codex/async_client.py @@ -53,6 +53,7 @@ class AsyncCodexClient: def __init__(self, config: CodexConfig | None = None) -> None: """Create the wrapped sync client that owns the transport process.""" self._sync = CodexClient(config=config) + self._background_tasks: set[asyncio.Task[None]] = set() async def __aenter__(self) -> "AsyncCodexClient": """Start the Codex process when entering an async context.""" @@ -90,6 +91,9 @@ class AsyncCodexClient: async def close(self) -> None: """Close the wrapped sync client in a worker thread.""" await self._call_sync(self._sync.close) + if self._background_tasks: + await asyncio.gather(*tuple(self._background_tasks), return_exceptions=True) + self._background_tasks.clear() async def initialize(self) -> InitializeResponse: """Initialize the Codex session.""" @@ -247,17 +251,23 @@ class AsyncCodexClient: try: return await asyncio.shield(operation) except asyncio.CancelledError: - try: - state, _ = await operation - except BaseException: - pass - else: + + async def cleanup_cancelled_start() -> None: + try: + state, _ = await operation + except BaseException: + return try: await self.pause_goal(thread_id) except Exception: pass - state.finish() - self.unregister_goal_operation(state) + finally: + state.finish() + self.unregister_goal_operation(state) + + cleanup = asyncio.create_task(cleanup_cancelled_start()) + self._background_tasks.add(cleanup) + cleanup.add_done_callback(self._background_tasks.discard) raise async def turn_start( diff --git a/sdk/python/tests/test_app_server_goal_turns.py b/sdk/python/tests/test_app_server_goal_turns.py index 71d9538eb4..9cc028c74a 100644 --- a/sdk/python/tests/test_app_server_goal_turns.py +++ b/sdk/python/tests/test_app_server_goal_turns.py @@ -576,6 +576,19 @@ def test_failed_goal_starts_release_routing_without_model_requests(tmp_path) -> with pytest.raises(TypeError) as type_error: thread.start_goal(123) # type: ignore[arg-type] + existing = codex._client.thread_goal_set( + thread.id, + objective="Keep this valid stored goal", + status=ThreadGoalStatus.paused, + ).goal + with pytest.raises(ValueError) as long_error: + thread.start_goal("x" * 4_001) + persisted = codex._client.request( + "thread/goal/get", + {"threadId": thread.id}, + response_model=ThreadGoalGetResponse, + ).goal + ephemeral = codex.thread_start(ephemeral=True) with pytest.raises(InvalidRequestError) as ephemeral_error: ephemeral.start_goal("Persist this goal") @@ -585,7 +598,17 @@ def test_failed_goal_starts_release_routing_without_model_requests(tmp_path) -> registered_goals = dict(codex._client._router._goal_operations) assert { - "errors": [str(empty_error.value), str(type_error.value), ephemeral_error.value.message], + "errors": [ + str(empty_error.value), + str(type_error.value), + str(long_error.value), + ephemeral_error.value.message, + ], + "stored_goal": ( + existing.objective, + persisted.objective if persisted else None, + persisted.status if persisted else None, + ), "follow_up": (follow_up.status, follow_up.final_response), "request_count": len(requests), "registered_goals": registered_goals, @@ -593,8 +616,14 @@ def test_failed_goal_starts_release_routing_without_model_requests(tmp_path) -> "errors": [ "goal objective must not be empty", "goal objective must be a string", + "goal objective must be at most 4000 characters", f"thread must be persisted before starting a goal: {ephemeral.id}", ], + "stored_goal": ( + "Keep this valid stored goal", + "Keep this valid stored goal", + ThreadGoalStatus.paused, + ), "follow_up": (TurnStatus.completed, "Ordinary turn complete."), "request_count": 1, "registered_goals": {},