From 548044e40afb95140cc1a42b751e184b15b1fbd8 Mon Sep 17 00:00:00 2001 From: tmatup <51425734+tmatup@users.noreply.github.com> Date: Wed, 21 Oct 2026 20:36:04 +0000 Subject: [PATCH] fix(codex): crash a turn codex ended as failed instead of grading it Codex delivers a normal turn/completed when it gives up on the provider, with turn.status == failed and a TurnError (for example rateLimitExceeded after its own stream retries ran out). The harness never read that status, so the turn booked as COMPLETED and the row was graded FAILURE on work the provider had cut off. In adhoc-2026-10-21_16-16-27 (codex, gpt-5.6-luna) 27 rows (11 v1, 16 v2) ended this way, after 0 to 39 commands, and read as skill failures in the v1-vs-v2 comparison. The turn now crashes with the provider's message, the same way Pi and OpenCode already treat a terminal provider error. categorize_error routes the "rate limit exceeded" text to AGENT_RATE_LIMIT and its retry budget; when retries run out the row lands as ERROR. The partial turn keeps its commands and provider_errors. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013fePq8hgqbtNvf2pNvA3WC --- .claude/notes/agents.md | 7 +- src/coder_eval/agents/codex_agent.py | 46 ++++++++-- tests/test_codex_agent.py | 122 +++++++++++++++++++++++++++ 3 files changed, 165 insertions(+), 10 deletions(-) diff --git a/.claude/notes/agents.md b/.claude/notes/agents.md index e11d1268f..c4d48490d 100644 --- a/.claude/notes/agents.md +++ b/.claude/notes/agents.md @@ -267,10 +267,15 @@ matter how much the run actually billed. So the CLI harnesses crash rather than arm keys on a step the CLI reported as FINISHED, its own claim that a generation completed, rather than on `usage.is_empty()` alone, which would also condemn a stream cut before any step could finish. -- **A terminal provider error** (Pi, OpenCode) — `pi -p` exits 0 after exhausting its +- **A terminal provider error** (Pi, OpenCode, Codex) — `pi -p` exits 0 after exhausting its internal retries, so without the crash the turn books as a clean COMPLETED (`FinalStatus.FAILURE`, category "failed"), silently depressing the measured pass rate. Crashing routes it to `FinalStatus.ERROR`, which is excluded from outcomes. + Codex reports the same thing differently: its `turn/completed` arrives normally but + carries `turn.status == failed` and a `TurnError` (for example `rateLimitExceeded` + after its own stream retries ran out). The harness reads that status and crashes, with + the provider's message, so `categorize_error` routes a rate limit to + `AGENT_RATE_LIMIT` and its retry budget like any other agent crash. - **A CLI that closed its stream but would not exit** within the grace period. Every arm is gated on `stopped_early` / `max_turns_exhausted`, because an intentional cut diff --git a/src/coder_eval/agents/codex_agent.py b/src/coder_eval/agents/codex_agent.py index 86fc8171c..6f1d89969 100644 --- a/src/coder_eval/agents/codex_agent.py +++ b/src/coder_eval/agents/codex_agent.py @@ -259,6 +259,23 @@ def _status_value(status: Any) -> str: return str(value).lower() if value is not None else "" +def _failed_turn_reason(turn: Any) -> str | None: + """The crash message for a Turn codex ended as ``failed``, or None for any other status. + + Codex ends a turn ``failed`` when it gives up on the provider, e.g. a + ``rateLimitExceeded`` it will not retry. The message carries the provider's + own text, so ``categorize_error`` routes it (``rate limit`` -> + ``AGENT_RATE_LIMIT``) like any other agent crash. + """ + if turn is None or _status_value(getattr(turn, "status", None)) != "failed": + return None + error = getattr(turn, "error", None) + message = str(getattr(error, "message", "") or "").strip() or "no error message" + kind, _ = _codex_error_kind(error) + suffix = f" (codexErrorInfo={kind})" if kind else "" + return f"Codex turn failed: {message}{suffix}" + + def _record_command_text(argv: Any) -> str: """The command a CommandExecution record ran, without the shell wrapper Codex adds. @@ -384,24 +401,29 @@ def _message_uncached_input(m: AssistantMessage) -> int: _ZSH_PROFILE_NAMES = frozenset({".zshenv", ".zprofile", ".zshrc"}) -def _provider_error_from(payload: Any) -> ProviderError: - """Map a codex ``ErrorNotification`` payload onto a ``ProviderError``. +def _codex_error_kind(error: Any) -> tuple[str | None, int | None]: + """``(kind, http_status)`` of a codex ``TurnError``'s ``codexErrorInfo``. ``codexErrorInfo`` is either a bare category (enum) or a one-key object whose key is the category and whose value may carry ``httpStatusCode``. """ - error = getattr(payload, "error", None) - kind: str | None = None - http_status: int | None = None - info = getattr(getattr(error, "codex_error_info", None), "root", None) + info: Any = getattr(getattr(error, "codex_error_info", None), "root", None) if isinstance(info, Enum): - kind = str(info.value) - elif hasattr(info, "model_dump"): + return str(info.value), None + if hasattr(info, "model_dump"): dumped = info.model_dump(by_alias=True, mode="json") if len(dumped) == 1: kind, body = next(iter(dumped.items())) if isinstance(body, dict) and isinstance(body.get("httpStatusCode"), int): - http_status = body["httpStatusCode"] + return kind, body["httpStatusCode"] + return kind, None + return None, None + + +def _provider_error_from(payload: Any) -> ProviderError: + """Map a codex ``ErrorNotification`` payload onto a ``ProviderError``.""" + error = getattr(payload, "error", None) + kind, http_status = _codex_error_kind(error) return ProviderError( at=datetime.now(), message=str(getattr(error, "message", "") or ""), @@ -1199,6 +1221,12 @@ def _on_turn_timeout() -> None: # like every other timeout path. assert timeout is not None self._finalize_and_raise_timeout(state.finalize, timeout) + failed_reason = None if state.ended_cleanly else _failed_turn_reason(state.result_turn) + if failed_reason is not None: + # Codex gave up on the turn (a provider error it would not retry), + # so the agent never finished: crash, never score it. + # Rationale: .claude/notes/agents.md § Why a clean exit can still be a crash + self._finalize_and_raise_crash(state.finalize, truncate_crash_message(failed_reason)) except (AgentCrashError, TurnTimeoutError): # Already funneled through finalize by the inner handlers. raise diff --git a/tests/test_codex_agent.py b/tests/test_codex_agent.py index 3aec31dae..6d87f47a3 100644 --- a/tests/test_codex_agent.py +++ b/tests/test_codex_agent.py @@ -2985,3 +2985,125 @@ def test_clean_turn_has_no_provider_errors(self): st, collector = self._state() st.finalize(AgentEndStatus.COMPLETED) assert collector.build_turn_record().provider_errors == [] + + +# The provider message codex reported in adhoc-2026-10-21_16-16-27 +# (v2/skill-case-diagnose-formal-arg-slot-ids): five retried stream disconnects, +# then a rate limit it would not retry, then a `failed` turn/completed. +_RATE_LIMIT_MESSAGE = ( + "rate limit exceeded: Your requests to gpt-5.6-luna for gpt-5.6-luna in eastus2 have exceeded token rate limit." +) + + +def _error_notification(error: dict, *, will_retry: bool) -> SimpleNamespace: + from openai_codex.generated.v2_all import ErrorNotification + + payload = ErrorNotification.model_validate( + {"error": error, "threadId": "t1", "turnId": "u1", "willRetry": will_retry} + ) + return SimpleNamespace(method="error", payload=payload) + + +def _turn_failed(error: dict | None) -> SimpleNamespace: + turn = Turn.model_validate( + { + "id": "turn_1", + "status": "failed", + "durationMs": 21847, + "error": error, + "items": [], + "itemsView": "notLoaded", + } + ) + return SimpleNamespace(method="turn/completed", payload=TurnCompletedNotification(thread_id="th_1", turn=turn)) + + +def _rate_limited_turn(*, commands: int) -> list[SimpleNamespace]: + notes: list[SimpleNamespace] = [] + for i in range(commands): + root = SimpleNamespace( + type="commandExecution", + id=f"cmd_{i}", + command=f"uip maestro case validate {i}", + exit_code=0, + aggregated_output="ok\n", + duration_ms=5, + ) + notes += [_item_notification("item/started", root), _item_notification("item/completed", root)] + notes.append(_delta("I'm using the Maestro Case skill to repair the plan.")) + for attempt in range(1, 6): + notes.append( + _error_notification( + { + "message": f"Reconnecting... {attempt}/5", + "codexErrorInfo": {"responseStreamDisconnected": {"httpStatusCode": None}}, + "additionalDetails": _RATE_LIMIT_MESSAGE, + }, + will_retry=True, + ) + ) + notes.append( + _error_notification({"message": _RATE_LIMIT_MESSAGE, "codexErrorInfo": "rateLimitExceeded"}, will_retry=False) + ) + notes.append(_turn_failed({"message": _RATE_LIMIT_MESSAGE, "codexErrorInfo": "rateLimitExceeded"})) + return notes + + +class TestFailedTurnIsACrash: + """A `turn/completed` whose turn is `failed` is a crash, never a COMPLETED turn. + + Before this, codex's terminal rate limit booked as COMPLETED and the row was + graded FAILURE: 27 of 114 rows in adhoc-2026-10-21_16-16-27 (codex, + gpt-5.6-luna) were scored on a turn the provider cut off. + """ + + @pytest.mark.parametrize("commands", [0, 4]) + async def test_rate_limited_turn_raises_crash_and_keeps_the_partial(self, commands): + agent = _started_agent(parse_agent_config(type=AgentKind.CODEX), _rate_limited_turn(commands=commands)) + + with pytest.raises(AgentCrashError) as excinfo: + await agent.communicate("repair the case") + + assert _RATE_LIMIT_MESSAGE in str(excinfo.value) + assert "codexErrorInfo=rateLimitExceeded" in str(excinfo.value) + assert agent.get_state() == AgentState.ERROR + + partial = agent.pending_turn + assert partial is not None + assert partial.crashed is True + assert partial.crash_reason is not None and "rate limit exceeded" in partial.crash_reason + # The work done before the cut and the provider evidence both survive. + assert len(partial.commands) == commands + assert [(e.kind, e.will_retry) for e in partial.provider_errors] == [ + *[("responseStreamDisconnected", True)] * 5, + ("rateLimitExceeded", False), + ] + + async def test_rate_limit_crash_is_categorized_as_a_rate_limit(self): + from coder_eval.errors.categories import ErrorCategory + from coder_eval.errors.categorization import categorize_error + + agent = _started_agent(parse_agent_config(type=AgentKind.CODEX), _rate_limited_turn(commands=0)) + with pytest.raises(AgentCrashError) as excinfo: + await agent.communicate("repair the case") + + assert categorize_error(excinfo.value, {"component": "agent"}) is ErrorCategory.AGENT_RATE_LIMIT + + async def test_failed_turn_without_error_details_still_crashes(self): + agent = _started_agent(parse_agent_config(type=AgentKind.CODEX), [_delta("hi"), _turn_failed(None)]) + + with pytest.raises(AgentCrashError, match="no error message"): + await agent.communicate("go") + + async def test_completed_turn_with_a_retried_provider_error_is_not_a_crash(self): + notes = [ + _error_notification({"message": "Reconnecting... 1/5"}, will_retry=True), + _delta("done"), + _turn_completed(), + ] + agent = _started_agent(parse_agent_config(type=AgentKind.CODEX), notes) + + record = await agent.communicate("go") + + assert record.crashed is False + assert agent.pending_turn is None