communicate: add Codex app-server adapter#805
Open
willwashburn wants to merge 4 commits into
Open
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/sdk/src/communicate/index.ts">
<violation number="1" location="packages/sdk/src/communicate/index.ts:33">
P2: This overload now allows `onRelay('name', relay)` even though runtime expects the second argument to be framework options for string-first calls, which can lead to mis-detection/errors.</violation>
</file>
<file name="packages/sdk/src/communicate/adapters/codex-jsonrpc.ts">
<violation number="1" location="packages/sdk/src/communicate/adapters/codex-jsonrpc.ts:126">
P1: Handle child process `'error'` in addition to `'exit'`; spawn failures can otherwise surface as unhandled errors and crash the process.</violation>
</file>
<file name="packages/sdk/src/communicate/adapters/codex.ts">
<violation number="1" location="packages/sdk/src/communicate/adapters/codex.ts:369">
P2: Only clear `activeTurnId` after a successful `turn/interrupt` request; clearing it before awaiting the RPC can desynchronize local state when the request fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
- spawnCodexAppServer: attach 'error' listener so spawn failures
(ENOENT, etc.) surface through onExit instead of crashing the process.
- onRelay overloads: split the public string-first and object-first
signatures so onRelay('name', relay) no longer type-checks; the
runtime always expected framework options as the second argument when
the first argument is a name.
- CodexRelayHandle.interrupt: only clear activeTurnId after the
turn/interrupt RPC resolves, so a failed request leaves local state
in sync with the server.
Adds regression tests for spawn error propagation and interrupt failure
preserving activeTurnId.
Contributor
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughImplements a Codex communicate adapter that spawns a codex app-server over stdio JSON-RPC: adds transport/client, adapter lifecycle with version and MCP handling, relay inbox steering/fallbacks, fork/share semantics, comprehensive tests, entrypoint wiring, package export, and README docs. ChangesCodex Communicate Adapter via JSON-RPC App-Server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/sdk/src/__tests__/communicate/adapters/codex.test.ts (1)
278-306: ⚡ Quick winAdd a regression test for
turn/completedpayloads that useparams.turnId.Current coverage only exercises
params.turn.id; aturnId-only case would protect turn-state tracking across notification shape variants.Suggested test shape
+test('Codex turn/completed with turnId does not clear a different active turn', async () => { + const { onRelay } = await loadCodexAdapterModule(); + const client = new FakeCodexClient(['relaycast']); + const handle = onRelay('CodexTester', { framework: 'codex', clientFactory: () => client }, new FakeRelay()); + await handle.ready; + + await client.emit({ + method: 'turn/started', + params: { threadId: 'thread-1', turnId: 'turn-live' }, + }); + + await client.emit({ + method: 'turn/completed', + params: { threadId: 'thread-1', turnId: 'turn-other' }, + }); + + await handle.interrupt(); + const interrupt = lastRequest(client, 'turn/interrupt'); + assert.equal(interrupt.params.turnId, 'turn-live'); +});🤖 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 `@packages/sdk/src/__tests__/communicate/adapters/codex.test.ts` around lines 278 - 306, Add a regression case to the existing "Codex turn/completed queues relay inbox for the next turn/start input" test to exercise the notification shape that uses params.turnId instead of params.turn.id: in the test that uses loadCodexAdapterModule(), FakeCodexClient, and onRelay('CodexTester', ...), emit a second turn/completed event where the payload includes params.turnId = 'turn-finished-2' (and no params.turn object), then call handle.send('Continue implementation.') and assert via lastRequest(client, 'turn/start') that the request used the same params.threadId and that the constructed input text contains the Relay message and the continuation text; this ensures the adapter's turn-completed handling accepts params.turnId as well as params.turn.id.packages/sdk/README.md (1)
114-122: 💤 Low valueConsider showing cleanup for completeness.
The example demonstrates initialization and sending a message but doesn't show the cleanup step. Adding
await codex.close()at the end would make the example more complete for users who might run this in a script.📝 Suggested addition
await codex.ready; await codex.send('Review the current branch and report risks.'); +await codex.close();🤖 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 `@packages/sdk/README.md` around lines 114 - 122, Add a cleanup step to the example by calling the client close method after use: after awaiting codex.ready and codex.send(...), also await codex.close() so the Relay/onRelay connection is cleanly torn down; update the README example to include this final await codex.close() call referencing the Relay, onRelay and codex symbols.packages/sdk/src/__tests__/communicate/adapters/codex-jsonrpc.test.ts (1)
212-227: ⚡ Quick winAdd a late-subscriber
onExitregression test.Please add a case where the child exits before
transport.onExit(...)is registered, then assert the callback still resolves. This protects the lifecycle contract from regressing.🤖 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 `@packages/sdk/src/__tests__/communicate/adapters/codex-jsonrpc.test.ts` around lines 212 - 227, Add a regression test variant under spawnCodexAppServer that simulates the child exiting before the caller registers onExit: create a transport via spawnCodexAppServer (use a command that immediately exits or mock the child to close right away), wait until the child has already exited, then register transport.onExit(...) and assert the callback still resolves with the expected exit values; target the existing test harness and symbols spawnCodexAppServer and transport.onExit so the test verifies late-subscriber behavior without changing production code.
🤖 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 `@packages/sdk/src/communicate/adapters/codex-jsonrpc.ts`:
- Around line 133-145: The onExit handler may miss already-terminated children;
after attaching listeners (child.once('exit', settle) and child.once('error',
...)) check the child's current termination state and immediately call settle if
needed by inspecting child.exitCode and child.signalCode (and still honoring
lastSpawnError): if exitCode is a number call settle(exitCode, null), if
signalCode is a string call settle(null, signalCode), and if lastSpawnError is
set call settle(null, null) to ensure the callback is always invoked; update the
onExit implementation (references: onExit, child, settle, lastSpawnError) to
perform these immediate checks right after listener registration.
In `@packages/sdk/src/communicate/adapters/codex.ts`:
- Around line 579-582: The branch handling notification.method ===
'turn/completed' clears this.activeTurnId unconditionally when params.turn?.id
is missing, but some payloads send params.turnId instead; update the condition
to check for either params.turn?.id or params.turnId and only clear
this.activeTurnId when the completed turn id matches the current activeTurnId
(i.e., compare this.activeTurnId to params.turn?.id || params.turnId) so
steer/interrupt still target an active turn when appropriate; locate the logic
in the codex adapter around notification.method === 'turn/completed' and modify
the if-check that references params.turn?.id and this.activeTurnId accordingly.
---
Nitpick comments:
In `@packages/sdk/README.md`:
- Around line 114-122: Add a cleanup step to the example by calling the client
close method after use: after awaiting codex.ready and codex.send(...), also
await codex.close() so the Relay/onRelay connection is cleanly torn down; update
the README example to include this final await codex.close() call referencing
the Relay, onRelay and codex symbols.
In `@packages/sdk/src/__tests__/communicate/adapters/codex-jsonrpc.test.ts`:
- Around line 212-227: Add a regression test variant under spawnCodexAppServer
that simulates the child exiting before the caller registers onExit: create a
transport via spawnCodexAppServer (use a command that immediately exits or mock
the child to close right away), wait until the child has already exited, then
register transport.onExit(...) and assert the callback still resolves with the
expected exit values; target the existing test harness and symbols
spawnCodexAppServer and transport.onExit so the test verifies late-subscriber
behavior without changing production code.
In `@packages/sdk/src/__tests__/communicate/adapters/codex.test.ts`:
- Around line 278-306: Add a regression case to the existing "Codex
turn/completed queues relay inbox for the next turn/start input" test to
exercise the notification shape that uses params.turnId instead of
params.turn.id: in the test that uses loadCodexAdapterModule(), FakeCodexClient,
and onRelay('CodexTester', ...), emit a second turn/completed event where the
payload includes params.turnId = 'turn-finished-2' (and no params.turn object),
then call handle.send('Continue implementation.') and assert via
lastRequest(client, 'turn/start') that the request used the same params.threadId
and that the constructed input text contains the Relay message and the
continuation text; this ensures the adapter's turn-completed handling accepts
params.turnId as well as params.turn.id.
🪄 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: ff515a49-1bf6-481f-a1c0-031e0c348448
📒 Files selected for processing (8)
packages/sdk/README.mdpackages/sdk/package.jsonpackages/sdk/src/__tests__/communicate/adapters/codex-jsonrpc.test.tspackages/sdk/src/__tests__/communicate/adapters/codex.test.tspackages/sdk/src/communicate/adapters/codex-jsonrpc.tspackages/sdk/src/communicate/adapters/codex.tspackages/sdk/src/communicate/adapters/index.tspackages/sdk/src/communicate/index.ts
- spawnCodexAppServer.onExit: handle the race where the child has already terminated before the listener is registered by checking child.exitCode / child.signalCode and firing the callback synchronously. 'exit'/'error' events are not replayed after they have been emitted, so a late subscriber would otherwise hang. - CodexRelayHandle turn/completed: accept params.turnId in addition to params.turn?.id, matching the existing turn/started handling. Without this, payloads that send only turnId would clear activeTurnId unconditionally and break a subsequent steer/interrupt. Adds regression tests for both the late onExit subscription and the turn/completed payload shapes (matching turnId clears, non-matching turnId is ignored).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
codex app-serverstdio JSON-RPC.ready,send,steer,interrupt,fork,close), relay inbox injection onitem/completed/turn/completed, relaycast MCP registration, and a minimum Codex version probe.onCodexRelay, add theframework: 'codex'discriminator inonRelay(), add the package subpath export, and document the PTY-vs-app-server tradeoff.Fixes #803
Testing
npm --prefix packages/sdk run checknpx tsc -p packages/sdk/tsconfig.build.json --noEmitnpx tsc -p packages/sdk/tsconfig.build.jsonnpx vitest run src/__tests__/communicate/adapters/codex.test.ts src/__tests__/communicate/adapters/codex-jsonrpc.test.ts --config vitest.config.tsnpx vitest run src/__tests__/communicate --config vitest.config.tscodex-cli 0.125.0Note:
npm --prefix packages/sdk run buildis currently blocked before SDK compilation in the existing prebuild chain:packages/github-primitivecannot resolve@agent-relay/workflow-typesbecause the workspace package is not linked innode_modules. The direct SDK build-config compile and declaration emit pass.