From 6bd2fa998260d6779f84684fab0fd2355e323718 Mon Sep 17 00:00:00 2001 From: Simon Koch-Sultan Date: Thu, 29 Oct 2026 15:19:12 +0000 Subject: [PATCH] Fix codex_app_server provider against a real Codex app-server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects that only appear against a real `codex app-server` (the in-process fake server models none of them): 1. request() parked non-matching messages in _pending, which next_message() drains before reading the queue — so the first interleaved notification (a real server emits e.g. configWarning / account/updated before responses) was returned forever in a 100%-CPU busy loop and the actual response was never read. Stash non-matching messages locally during the request and restore them to the front of _pending, in order, when the response arrives. 2. thread/start sent sandbox as a {"mode": ..., "networkAccess": ...} map. ThreadStartParams.sandbox is a SandboxMode string enum in current app-server versions (verified against the server's generated JSON schema for Codex 0.160.1 and 0.164.0-alpha.9); the map form is rejected with -32600. Send "read-only". The existing test asserted the broken map shape and is corrected. 3. _ensure_authenticated() treated requiresOpenaiAuth as a logged-out signal. Current servers return requiresOpenaiAuth=true together with a valid ChatGPT account — the flag describes the auth mode. Only a missing account fails the boundary now (the account type check below is unchanged). Tests: the fake server gains a notify_first scenario (unsolicited notifications ahead of account/read and thread/start responses) and a chatgpt_requires_auth_flag account/read variant. New tests cover all three defects; each fails against the unfixed code (the notification test via a join-timeout assertion, so the wedged code fails instead of hanging the suite). --- openevolve/llm/codex_app_server.py | 53 ++++++++++++++++-------- tests/test_codex_app_server_llm.py | 66 +++++++++++++++++++++++++++++- 2 files changed, 101 insertions(+), 18 deletions(-) diff --git a/openevolve/llm/codex_app_server.py b/openevolve/llm/codex_app_server.py index b0800bdbc..74a20f1ee 100644 --- a/openevolve/llm/codex_app_server.py +++ b/openevolve/llm/codex_app_server.py @@ -125,20 +125,33 @@ def request(self, method: str, params: Optional[Dict[str, Any]], deadline: float self._next_id += 1 request_id = self._next_id self._send({"id": request_id, "method": method, "params": params or {}}) - while True: - message = self.next_message(deadline) - if message.get("id") != request_id: - # A notification (e.g. an early item/completed) or a late - # response: park it instead of dropping it. - self._pending.append(message) - continue - if "error" in message: - error = message["error"] or {} - raise RuntimeError( - f"codex app-server rejected {method}: " - f"{error.get('code')}: {error.get('message')}" - ) - return message.get("result") + # Non-matching messages must not be parked in self._pending here: + # next_message() drains _pending first, so a parked message would + # be returned again on the next iteration and the loop would spin + # forever without ever reading the actual response from the queue + # (a real codex app-server emits notifications before responses). + # Stash non-matching messages locally during the request and + # restore them to the front of _pending, in order, when the + # response arrives. + stash: List[Dict[str, Any]] = [] + try: + while True: + message = self.next_message(deadline) + if message.get("id") != request_id: + # A notification (e.g. an early item/completed) or a late + # response: stash it instead of dropping it. + stash.append(message) + continue + if "error" in message: + error = message["error"] or {} + raise RuntimeError( + f"codex app-server rejected {method}: " + f"{error.get('code')}: {error.get('message')}" + ) + return message.get("result") + finally: + for stashed in reversed(stash): + self._pending.appendleft(stashed) def notify(self, method: str, params: Optional[Dict[str, Any]] = None) -> None: self._send({"method": method, "params": params or {}}) @@ -284,7 +297,11 @@ def _run_app_server(self, prompt: str, timeout: int) -> str: thread_params: Dict[str, Any] = { "model": self.model, "approvalPolicy": "never", - "sandbox": {"mode": "read-only", "networkAccess": False}, + # ThreadStartParams.sandbox is a SandboxMode string enum + # ("read-only" | "workspace-write" | "danger-full-access"), + # not a {"mode": ..., "networkAccess": ...} map — current + # app-server versions reject the map form with -32600. + "sandbox": "read-only", } if self.cwd: thread_params["cwd"] = self.cwd @@ -307,7 +324,11 @@ def _run_app_server(self, prompt: str, timeout: int) -> str: def _ensure_authenticated(self, session: _AppServerSession, deadline: float) -> None: result = session.request("account/read", {}, deadline) or {} - if result.get("requiresOpenaiAuth") or result.get("account") is None: + # Current app-server versions return requiresOpenaiAuth=true + # together with a valid ChatGPT account — the flag describes the + # auth mode, not a missing login. The account presence/type check + # below is the real auth boundary. + if result.get("account") is None: raise CodexAuthError("Codex app-server reports the account is not authenticated.") account = result.get("account") or {} if account.get("type") != "chatgpt": diff --git a/tests/test_codex_app_server_llm.py b/tests/test_codex_app_server_llm.py index 9324c3aa5..fe94bd6c4 100644 --- a/tests/test_codex_app_server_llm.py +++ b/tests/test_codex_app_server_llm.py @@ -113,6 +113,10 @@ def _dispatch(self, method, request_id, params): elif method == "initialized": pass # notification, no reply elif method == "account/read": + if self.scenario == "notify_first": + # A real app-server pushes unsolicited notifications (config + # warnings, account updates) ahead of its responses. + self._notify("configWarning", {"message": "config warning"}) if self.scenario == "unauthenticated": self._reply(request_id, {"account": None, "requiresOpenaiAuth": True}) elif self.scenario == "apikey_account": @@ -120,12 +124,22 @@ def _dispatch(self, method, request_id, params): request_id, {"account": {"type": "apiKey"}, "requiresOpenaiAuth": False}, ) + elif self.scenario == "chatgpt_requires_auth_flag": + # Current app-server versions report requiresOpenaiAuth=True + # alongside a valid ChatGPT account: the flag describes the + # auth mode, not a missing login. + self._reply( + request_id, + {"account": AUTHENTICATED_ACCOUNT, "requiresOpenaiAuth": True}, + ) else: self._reply( request_id, {"account": AUTHENTICATED_ACCOUNT, "requiresOpenaiAuth": False}, ) elif method == "thread/start": + if self.scenario == "notify_first": + self._notify("account/updated", {"account": AUTHENTICATED_ACCOUNT}) self._reply( request_id, {"thread": {"id": "thread-123", "model": params.get("model")}}, @@ -267,6 +281,17 @@ def test_apikey_account_is_rejected_with_apikey_guidance(self): self.assertIn("OpenAI provider", str(outcome["error"])) self.assertNotIn("turn/start", [m for m, _ in fake.requests]) + def test_requires_openai_auth_flag_with_chatgpt_account_generates(self): + """requiresOpenaiAuth=True next to a valid ChatGPT account is logged in. + + The flag describes the auth mode of the app-server, so only a + missing (or non-ChatGPT) account may fail the auth boundary. + """ + outcome, fake, _ = run_generate(scenario="chatgpt_requires_auth_flag") + self.assertIsNone(outcome["error"]) + self.assertEqual(outcome["result"], "hello world") + self.assertIn("turn/start", [m for m, _ in fake.requests]) + def test_auth_error_is_not_retried(self): outcome, fake, _ = run_generate( scenario="unauthenticated", cfg_overrides={"retries": 3, "retry_delay": 0} @@ -287,8 +312,10 @@ def test_thread_is_non_interactive_and_read_only(self): params = next(p for m, p in fake.requests if m == "thread/start") self.assertEqual(params["model"], "gpt-5.3-codex") self.assertEqual(params["approvalPolicy"], "never") - self.assertEqual(params["sandbox"]["mode"], "read-only") - self.assertFalse(params["sandbox"]["networkAccess"]) + # ThreadStartParams.sandbox is a SandboxMode string enum, not a + # {"mode": ..., "networkAccess": ...} map; current app-server + # versions reject the map form with error -32600. + self.assertEqual(params["sandbox"], "read-only") def test_turn_input_and_effort(self): _, fake, _ = run_generate( @@ -324,6 +351,41 @@ def test_server_crash_mid_turn_raises(self): self.assertIsInstance(outcome["error"], RuntimeError) self.assertIn("exited unexpectedly", str(outcome["error"])) + def test_interleaved_notification_does_not_wedge_generate(self): + """A notification ahead of a response must not wedge request(). + + Regression test: request() parked non-matching messages in + _pending, which next_message() drains before reading the queue — + so the first interleaved notification was returned forever in a + busy loop and the actual response was never read. generate() runs + in a thread so the wedged version fails this test on the join + timeout instead of hanging the whole suite. + """ + fake = FakeAppServer("notify_first") + self.addCleanup(fake.terminate) + llm = CodexAppServerLLM(_make_cfg()) + outcome = {} + + def _execute(): + try: + outcome["result"] = asyncio.run(llm.generate("test prompt")) + outcome["error"] = None + except Exception as e: # noqa: BLE001 - tests assert on the type + outcome["error"] = e + + with patch( + "openevolve.llm.codex_app_server.subprocess.Popen", + return_value=fake, + ): + worker = threading.Thread(target=_execute, daemon=True) + worker.start() + worker.join(timeout=15) + self.assertFalse( + worker.is_alive(), "generate() wedged on an interleaved notification" + ) + self.assertIsNone(outcome.get("error")) + self.assertEqual(outcome.get("result"), "hello world") + def test_hung_server_hits_deadline(self): outcome, _, _ = run_generate(scenario="hang", cfg_overrides={"timeout": 1}) self.assertIsInstance(outcome["error"], asyncio.TimeoutError)