Skip to content

feat(agent): Default Codex to the native app-server harness#2723

Draft
charlesvien wants to merge 6 commits into
mainfrom
feat/codex-app-server-adapter
Draft

feat(agent): Default Codex to the native app-server harness#2723
charlesvien wants to merge 6 commits into
mainfrom
feat/codex-app-server-adapter

Conversation

@charlesvien

@charlesvien charlesvien commented Jun 17, 2026

Copy link
Copy Markdown
Member

Problem

Codex ran through the Zed codex-acp ACP adapter, an extra translation layer that weakens native Codex capabilities (thread resume/fork, typed item and tool events, detailed usage, structured outputs, approvals).

Changes

  1. Bundle the native codex CLI binary (download-binaries + vite copy)
  2. Add CodexAppServerAgent speaking Codex's native app-server JSON-RPC under an unchanged ACP surface
  3. Add the app-server JSON-RPC client, ACP event mapping, and spawn helper
  4. Default createAcpConnection to the native agent, with POSTHOG_CODEX_USE_ACP=1 to fall back to codex-acp

How did you test this?

Manually

Automatic notifications

  • Publish to changelog?
  • Alert Sales and Marketing teams?

charlesvien commented Jun 17, 2026

Copy link
Copy Markdown
Member Author

@github-actions

github-actions Bot commented Jun 17, 2026

Copy link
Copy Markdown

React Doctor found no issues in the changed files. 🎉

Reviewed by React Doctor for commit da46d82.

@greptile-apps

greptile-apps Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Comments Outside Diff (3)

  1. packages/agent/src/adapters/codex-app-server/spawn.ts, line 1176-1184 (link)

    P2 apiBaseUrl is not escaped in the config arg

    developerInstructions is carefully escaped (backslash, newlines, quotes) before being injected into the -c flag value, but apiBaseUrl and the other string literals (e.g. "posthog", "responses") are interpolated verbatim. If apiBaseUrl ever contains a " character (e.g. from a misconfigured env var), the produced config line will be malformed and silently misbehave. Consider applying the same escaping logic, or at minimum adding an assertion that the URL contains no double-quotes.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: packages/agent/src/adapters/codex-app-server/spawn.ts
    Line: 1176-1184
    
    Comment:
    **`apiBaseUrl` is not escaped in the config arg**
    
    `developerInstructions` is carefully escaped (backslash, newlines, quotes) before being injected into the `-c` flag value, but `apiBaseUrl` and the other string literals (e.g. `"posthog"`, `"responses"`) are interpolated verbatim. If `apiBaseUrl` ever contains a `"` character (e.g. from a misconfigured env var), the produced config line will be malformed and silently misbehave. Consider applying the same escaping logic, or at minimum adding an assertion that the URL contains no double-quotes.
    
    How can I resolve this? If you propose a fix, please make it concise.
  2. packages/agent/src/adapters/codex-app-server/codex-app-server-agent.ts, line 821-831 (link)

    P2 pendingTurnResolve not cleared when turn/start request rejects

    pendingTurnResolve is assigned before await this.rpc.request(TURN_START, ...). If that request rejects (e.g. the server returns a JSON-RPC error), prompt() throws but pendingTurnResolve is left pointing at the abandoned completion promise's resolver. A subsequent prompt() call overwrites it correctly, but if the server had partially accepted the first turn and later emits turn/completed, handleNotification would call the stale resolver on the new call's promise, prematurely resolving it. Clearing pendingTurnResolve in a catch block around the request keeps the invariant that the field is only set while a live turn is in flight.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: packages/agent/src/adapters/codex-app-server/codex-app-server-agent.ts
    Line: 821-831
    
    Comment:
    **`pendingTurnResolve` not cleared when `turn/start` request rejects**
    
    `pendingTurnResolve` is assigned before `await this.rpc.request(TURN_START, ...)`. If that request rejects (e.g. the server returns a JSON-RPC error), `prompt()` throws but `pendingTurnResolve` is left pointing at the abandoned completion promise's resolver. A subsequent `prompt()` call overwrites it correctly, but if the server had partially accepted the first turn and later emits `turn/completed`, `handleNotification` would call the stale resolver on the *new* call's promise, prematurely resolving it. Clearing `pendingTurnResolve` in a `catch` block around the request keeps the invariant that the field is only set while a live turn is in flight.
    
    How can I resolve this? If you propose a fix, please make it concise.
  3. packages/agent/src/adapters/codex-app-server/codex-app-server-agent.test.ts, line 567 (link)

    P2 Test mocks use as unknown as to bypass required interface properties

    init, NewSessionRequest, and PromptRequest stubs are cast via as unknown as InterfaceType rather than supplying all required fields. This sidesteps TypeScript's property checks and can mask breakage when the real interface gains a required field that the agent code reads. The same pattern appears across the three describe blocks. Prefer building the minimal complete objects the agent actually reads, which makes the test contract explicit and keeps it safe under future interface changes.

    Rule Used: When creating mock objects for tests, include all ... (source)

    Learned From
    PostHog/posthog#32521

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: packages/agent/src/adapters/codex-app-server/codex-app-server-agent.test.ts
    Line: 567
    
    Comment:
    **Test mocks use `as unknown as` to bypass required interface properties**
    
    `init`, `NewSessionRequest`, and `PromptRequest` stubs are cast via `as unknown as InterfaceType` rather than supplying all required fields. This sidesteps TypeScript's property checks and can mask breakage when the real interface gains a required field that the agent code reads. The same pattern appears across the three `describe` blocks. Prefer building the minimal complete objects the agent actually reads, which makes the test contract explicit and keeps it safe under future interface changes.
    
    **Rule Used:** When creating mock objects for tests, include all ... ([source](https://app.greptile.com/posthog-org-19734/-/custom-context?memory=693b8339-f300-4cf5-bacb-3f1630384594))
    
    **Learned From**
    [PostHog/posthog#32521](https://github.com/PostHog/posthog/pull/32521)
    
    How can I resolve this? If you propose a fix, please make it concise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
packages/agent/src/adapters/codex-app-server/spawn.ts:1176-1184
**`apiBaseUrl` is not escaped in the config arg**

`developerInstructions` is carefully escaped (backslash, newlines, quotes) before being injected into the `-c` flag value, but `apiBaseUrl` and the other string literals (e.g. `"posthog"`, `"responses"`) are interpolated verbatim. If `apiBaseUrl` ever contains a `"` character (e.g. from a misconfigured env var), the produced config line will be malformed and silently misbehave. Consider applying the same escaping logic, or at minimum adding an assertion that the URL contains no double-quotes.

### Issue 2 of 3
packages/agent/src/adapters/codex-app-server/codex-app-server-agent.ts:821-831
**`pendingTurnResolve` not cleared when `turn/start` request rejects**

`pendingTurnResolve` is assigned before `await this.rpc.request(TURN_START, ...)`. If that request rejects (e.g. the server returns a JSON-RPC error), `prompt()` throws but `pendingTurnResolve` is left pointing at the abandoned completion promise's resolver. A subsequent `prompt()` call overwrites it correctly, but if the server had partially accepted the first turn and later emits `turn/completed`, `handleNotification` would call the stale resolver on the *new* call's promise, prematurely resolving it. Clearing `pendingTurnResolve` in a `catch` block around the request keeps the invariant that the field is only set while a live turn is in flight.

### Issue 3 of 3
packages/agent/src/adapters/codex-app-server/codex-app-server-agent.test.ts:567
**Test mocks use `as unknown as` to bypass required interface properties**

`init`, `NewSessionRequest`, and `PromptRequest` stubs are cast via `as unknown as InterfaceType` rather than supplying all required fields. This sidesteps TypeScript's property checks and can mask breakage when the real interface gains a required field that the agent code reads. The same pattern appears across the three `describe` blocks. Prefer building the minimal complete objects the agent actually reads, which makes the test contract explicit and keeps it safe under future interface changes.

Reviews (1): Last reviewed commit: "default codex to native app-server harne..." | Re-trigger Greptile

@charlesvien charlesvien force-pushed the feat/codex-app-server-adapter branch from f544874 to 2ee0938 Compare June 17, 2026 05:29
@charlesvien charlesvien changed the title add codex app-server json-rpc client feat(agent): Default Codex to the native app-server harness Jun 17, 2026
@charlesvien charlesvien marked this pull request as ready for review June 17, 2026 05:45
@greptile-apps

greptile-apps Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
packages/agent/src/adapters/codex-app-server/app-server-client.ts:104-106
**Unexpected process exit leaves pending promises hanging forever**

When the codex process crashes, the stdout stream closes and `readLoop` detects `done` and exits silently. At that point `AppServerClient.pending` still holds any in-flight RPC requests, and more critically `CodexAppServerAgent.pendingTurnResolve` is still set (waiting for `turn/completed` that will never arrive). Neither is ever rejected, so `prompt()` hangs indefinitely.

`close()` already does the right thing — rejects all `pending` calls and clears state — but it is never triggered by a transport EOF. The loop should call `void this.close()` (or a dedicated `onClose` callback) when `done` is `true` and `!this.closed`.

Reviews (2): Last reviewed commit: "default codex to native app-server harne..." | Re-trigger Greptile

Comment thread packages/agent/src/adapters/codex-app-server/app-server-client.ts
@charlesvien charlesvien force-pushed the fix/cloud-claude-reasoning-effort branch from 688d6b1 to ad5cb12 Compare June 17, 2026 06:19
@charlesvien charlesvien force-pushed the feat/codex-app-server-adapter branch from 2ee0938 to 7abe5de Compare June 17, 2026 06:19
@charlesvien charlesvien marked this pull request as draft June 17, 2026 06:26
@charlesvien charlesvien added the Stamphog This will request an autostamp by stamphog on small changes label Jun 17, 2026
@charlesvien charlesvien changed the base branch from fix/cloud-claude-reasoning-effort to graphite-base/2723 June 17, 2026 18:26
@charlesvien charlesvien force-pushed the feat/codex-app-server-adapter branch from 7abe5de to 2028e4b Compare June 17, 2026 18:38
@graphite-app graphite-app Bot changed the base branch from graphite-base/2723 to main June 17, 2026 18:39
@charlesvien charlesvien force-pushed the feat/codex-app-server-adapter branch from 2028e4b to f30123d Compare June 17, 2026 18:39
@charlesvien charlesvien force-pushed the feat/codex-app-server-adapter branch from f30123d to da46d82 Compare June 17, 2026 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Stamphog This will request an autostamp by stamphog on small changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant