fix: add a safe Codex compatibility relay - #937
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a local FastAPI compatibility relay between OpenAI Chat Completions clients and the Codex Responses streaming API, including request translation, SSE handling, credential access, authentication, CLI startup, tests, and documentation. ChangesCodex compatibility relay
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ChatClient
participant FastAPI
participant CodexRelay
participant CodexResponses
ChatClient->>FastAPI: POST /v1/chat/completions
FastAPI->>CodexRelay: Translate request
CodexRelay->>CodexResponses: Open streaming request
CodexResponses-->>CodexRelay: Return SSE events
CodexRelay-->>FastAPI: Translate response
FastAPI-->>ChatClient: JSON or SSE response
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/codex_relay.py (4)
33-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused OAuth constants.
CODEX_OAUTH_TOKEN_URLandCODEX_OAUTH_CLIENT_IDare never referenced, and they imply a refresh flow that this module explicitly delegates to Hermes. Dropping them also clears the Ruff S105 false positive on Line 33.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codex_relay.py` around lines 33 - 34, Remove the unused CODEX_OAUTH_TOKEN_URL and CODEX_OAUTH_CLIENT_ID constants from the module, leaving the existing Hermes-delegated OAuth flow unchanged.Source: Linters/SAST tools
135-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten the signature to match actual behavior.
Every path returns a dict or raises, so
| Noneis dead. Also, theisinstance(tool, dict)guard only makes sense because callers pass unvalidated list elements — annotate the parameter asAny.♻️ Suggested signature
-def _response_tool(tool: dict[str, Any]) -> dict[str, Any] | None: +def _response_tool(tool: Any) -> dict[str, Any]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codex_relay.py` around lines 135 - 137, Update the _response_tool function signature to accept Any and return dict[str, Any], removing the unused None union while preserving the existing validation and error behavior.Source: Coding guidelines
744-747: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename this local to avoid shadowing the upstream
response.
responsehere shadows thestream_chatparameter (Line 703) that the outer loop and thefinally: await response.aclose()rely on. Behavior is correct today, but any future read of the httpx response insidetranslatewould silently resolve to this dict.♻️ Proposed rename
- response, usage = _terminal_response(event, require_usage=include_usage) + terminal_payload, usage = _terminal_response(event, require_usage=include_usage) terminal = True - details = response.get("incomplete_details") + details = terminal_payload.get("incomplete_details")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codex_relay.py` around lines 744 - 747, Rename the local `response` returned by `_terminal_response` in the `translate` flow to a distinct name, and update its subsequent `get("incomplete_details")` access. Preserve the outer `stream_chat` response binding so `finally: await response.aclose()` continues to reference the HTTP response.
654-666: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsolidate the three SSE parsers.
_sse_events,complete.event_payload, andstream_chat.event_payloadimplement the same event framing (comment skip,event:/data:accumulation,[DONE], non-object rejection, trailing flush) three times. Extracting one async line-to-event generator that bothcompleteandstream_chatconsume would remove the duplicate loops at Lines 672-687 and 760-775 as well, and keep terminal-event semantics in one place.Also applies to: 711-723
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codex_relay.py` around lines 654 - 666, Consolidate the duplicated SSE framing logic from _sse_events, complete.event_payload, and stream_chat.event_payload into one shared async line-to-event generator. Have both complete and stream_chat consume this generator, preserving comment skipping, event/data accumulation, [DONE] handling, non-object rejection, trailing flush, and event-name type assignment; remove the redundant parsing loops and local event_payload implementations.tests/llm/test_codex_relay.py (1)
203-217: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the function-local
asyncioimports.
asynciois already imported at module scope (Line 3), so the local re-imports on Lines 207 and 215 (andbase64on Line 489) are redundant.♻️ Proposed cleanup
store = AuthStore(path) with pytest.raises(RelayError, match="credential source"): - import asyncio asyncio.run(store.token()) @@ store = AuthStore(path) - import asyncio assert asyncio.run(store.token()) == "synthetic"As per coding guidelines: "follow isort conventions with absolute imports preferred".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/llm/test_codex_relay.py` around lines 203 - 217, Remove the redundant function-local asyncio imports from test_auth_store_is_read_only_and_reports_missing_source and test_auth_store_reads_only_canonical_provider_token, reusing the existing module-level import. Also remove the redundant local base64 import near the referenced token-handling test, preserving the module-level import and current behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex_relay.py`:
- Around line 579-599: Reformat src/codex_relay.py so all lines comply with the
88-character Black/Ruff limit, including the cited long expressions. Add
Google-style docstrings with appropriate Args, Returns, and Raises sections to
CodexRelay, its methods close, _send, open_upstream, complete, stream_chat, and
the public create_app and main functions.
- Around line 819-823: Update the authorization check in the inbound_key
authentication block to compare encoded byte values with hmac.compare_digest, so
non-ASCII bearer candidates return the existing 401 response instead of raising
TypeError. Also ensure an empty inbound_key is treated as unconfigured or
rejected during create_app setup, preventing every request from being rejected.
---
Nitpick comments:
In `@src/codex_relay.py`:
- Around line 33-34: Remove the unused CODEX_OAUTH_TOKEN_URL and
CODEX_OAUTH_CLIENT_ID constants from the module, leaving the existing
Hermes-delegated OAuth flow unchanged.
- Around line 135-137: Update the _response_tool function signature to accept
Any and return dict[str, Any], removing the unused None union while preserving
the existing validation and error behavior.
- Around line 744-747: Rename the local `response` returned by
`_terminal_response` in the `translate` flow to a distinct name, and update its
subsequent `get("incomplete_details")` access. Preserve the outer `stream_chat`
response binding so `finally: await response.aclose()` continues to reference
the HTTP response.
- Around line 654-666: Consolidate the duplicated SSE framing logic from
_sse_events, complete.event_payload, and stream_chat.event_payload into one
shared async line-to-event generator. Have both complete and stream_chat consume
this generator, preserving comment skipping, event/data accumulation, [DONE]
handling, non-object rejection, trailing flush, and event-name type assignment;
remove the redundant parsing loops and local event_payload implementations.
In `@tests/llm/test_codex_relay.py`:
- Around line 203-217: Remove the redundant function-local asyncio imports from
test_auth_store_is_read_only_and_reports_missing_source and
test_auth_store_reads_only_canonical_provider_token, reusing the existing
module-level import. Also remove the redundant local base64 import near the
referenced token-handling test, preserving the module-level import and current
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56a76695-04f7-48bb-87fa-c7ef552f4be1
📒 Files selected for processing (3)
docs/codex-relay.mdsrc/codex_relay.pytests/llm/test_codex_relay.py
| class CodexRelay: | ||
| def __init__( | ||
| self, | ||
| *, | ||
| client: httpx.AsyncClient | None = None, | ||
| access_token: str | None = None, | ||
| token_provider: TokenProvider | None = None, | ||
| auth_path: Path = DEFAULT_AUTH_PATH, | ||
| upstream_url: str = CODEX_RESPONSES_URL, | ||
| ) -> None: | ||
| self.client = client or httpx.AsyncClient(timeout=httpx.Timeout(1800.0, connect=30.0)) | ||
| self._owns_client = client is None | ||
| self.access_token = access_token | ||
| self.token_provider = token_provider | ||
| self.auth = AuthStore(auth_path) | ||
| self.upstream_url = upstream_url | ||
|
|
||
| async def close(self) -> None: | ||
| if self._owns_client: | ||
| await self.client.aclose() | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Line length and docstring conventions.
CodexRelay and its methods (close, _send, open_upstream, complete, stream_chat), plus create_app and main, carry no docstrings, and roughly twenty lines in this module exceed 88 characters (e.g., Lines 32, 197, 211, 302, 321, 420, 735, 742, 752, 799, 868). Running Black/Ruff format and adding Google-style docstrings with Args/Returns/Raises on the public surface would bring this file in line with the rest of src/.
As per coding guidelines: "keep lines within 88 characters to match Black-compatible formatting" and "use Google-style docstrings".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/codex_relay.py` around lines 579 - 599, Reformat src/codex_relay.py so
all lines comply with the 88-character Black/Ruff limit, including the cited
long expressions. Add Google-style docstrings with appropriate Args, Returns,
and Raises sections to CodexRelay, its methods close, _send, open_upstream,
complete, stream_chat, and the public create_app and main functions.
Source: Coding guidelines
|
Thanks for the review. I addressed the actionable stability and cleanup points in commit
The focused relay suite now passes 51 tests and the adjacent LLM suite passes 291 tests. I left the broader SSE-parser consolidation and blanket docstring request out of this PR because they are unrelated refactors; the existing parser behavior is covered by the passing regression suite. |
1 participant
Honcho normally talks to language models using the OpenAI chat format, while Codex uses a slightly different format. This pull request adds a small bridge between those two formats, so Honcho can use Codex without changing the way the rest of Honcho talks to models.
It also makes that bridge safer: it does not write or refresh credentials itself, requires a key when exposed beyond the local machine, handles streaming correctly, and avoids passing private error details back to callers.
What this changes
Existing Honcho deployments are unchanged unless this optional relay is started and configured.
Why
Honcho's current OpenAI backend speaks the Chat Completions protocol. Codex exposes a Responses endpoint. Without a compatibility layer, the two sides cannot be used together reliably, especially for streaming and usage reporting.
This PR keeps the integration deliberately small and explicit rather than changing Honcho's general LLM abstraction.
Related Honcho work
These existing PRs are relevant context and should be considered alongside this one:
Verification
All checks were run from the locked project environment:
uv run --frozen pytest tests/llm/test_codex_relay.py -q -n 0— 49 passeduv run --frozen pytest tests/llm -q— 289 passedI also ran two real smoke checks against the branch in a temporary local process:
[DONE]receivedNo production service was restarted or changed.
Known limitation
The full repository test command remains limited by the local PostgreSQL fixture: the environment cannot authenticate/connect to
postgres@localhost:5432. The relay-focused and complete LLM suites are green.Scope
The change is limited to:
docs/codex-relay.mdsrc/codex_relay.pytests/llm/test_codex_relay.pyI would appreciate feedback on whether the supported request boundary and the credential-ownership model are the right fit for Honcho upstream.
Summary by CodeRabbit
New Features
Documentation
Tests