Add Codex AppServer Pi provider#92
Merged
glittercowboy merged 4 commits intomainfrom Apr 29, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds a new Pi extension provider "codex-appserver" that runs a codex app-server child process over newline-delimited JSON-RPC, threads a configurable provider value (defaulting to "claude-cli") from task context through the Pi executor, and includes tests and a smoke test; also bumps a Go dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PI as PI Executor
participant Codex as Codex App-Server
participant Tool as Tool Handler
Client->>PI: start turn (model, provider, tools)
PI->>Codex: spawn process, open JSON-RPC (stdin/stdout)
PI->>Codex: send turn/request (with model, tools)
Codex->>PI: emit text_start
PI->>Client: stream text_start
Codex->>PI: emit text_delta
PI->>Client: stream text_delta
Codex->>PI: emit tool_call_start
PI->>Tool: emit toolcall_start and run tool (ask_user_questions)
Tool-->>PI: return tool result
PI->>Codex: deliver tool result
Codex->>PI: emit tool_call_end / toolUse
PI->>Client: stream toolUse
Codex->>PI: emit token_usage
PI->>Client: stream usage
Codex->>PI: emit done
PI->>Client: finalize stream
Client->>PI: abort
PI->>Codex: send turn/interrupt
Codex->>PI: acknowledge interrupt
PI->>Client: close stream
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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)
Review rate limit: 4/10 reviews remaining, refill in 31 minutes and 35 seconds. 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: 4
🧹 Nitpick comments (1)
internal/session/actor.go (1)
619-622: Prefer a single source of truth for provider defaulting.
"claude-cli"fallback is now duplicated here and ininternal/pi/executor.go. Consider centralizing defaulting in one layer to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/actor.go` around lines 619 - 622, Centralize the `"claude-cli"` default by adding a shared helper ProviderOrDefault(provider string) that does strings.TrimSpace and returns "claude-cli" when empty, then replace the local logic in actor.go (the provider := strings.TrimSpace(tc.Provider) / if provider == "" block) with provider = ProviderOrDefault(tc.Provider) and update internal/pi/executor.go to call the same ProviderOrDefault so there is a single source of truth for provider defaulting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/pi/extension/codex-appserver-provider.ts`:
- Around line 274-319: The initPromise IIFE can stay rejected forever if
codexRequest("initialize") or codexRequest("thread/start") throws, so wrap the
async initialization block in a try/catch that performs the same cleanup done in
the codex.on("close") handler (set initialized = false; initPromise = null;
codex = null; rl = null; threadId = null; activeTurnId = null; pendingBridge =
null; abort/kill the codex process if started; end any activeRun stream with an
error) and then rethrow the error so callers see the failure; this ensures
initPromise is cleared on error and subsequent calls to the function will retry
initialization.
- Around line 349-353: streamCodexAppServer currently overwrites the single
global activeRun, risking cross-routing when streams overlap; add a guard at the
start of streamCodexAppServer to detect an existing activeRun (created by
createAssistantMessageEventStream/codexOutputForModel) and either reject/throw
(or return an error stream) instead of proceeding, then ensure activeRun is
cleared on stream termination (on end/error/close handlers) so subsequent calls
can set activeRun safely; update the function to check activeRun before
assigning, and add cleanup setting activeRun = null in the stream's completion
and error handlers to prevent state corruption.
- Around line 282-297: The Codex process close handler currently leaves pending
RPC promises/timers alive; update the codex.on("close", ...) block to actively
reject and cleanup all pending RPCs: iterate the pendingBridge (and any other
pending requests collection), for each pending request call its reject callback
with a clear Error (e.g. "Codex process exited") and clear any associated
timeout timers, then remove entries from pendingBridge; also set pendingBridge =
null (and any other pending maps) after cleanup so no further resolution
happens; keep the existing activeRun shutdown logic but ensure pending RPCs are
rejected before nulling pendingBridge/initPromise.
In `@internal/pi/extension/codex-appserver-smoke.mjs`:
- Around line 57-61: The smoke test currently checks only camelCase event fields
(event.toolName) in the conditional branches that set flags.sawToolStart and
flags.sawToolEnd; update the conditions in the event handlers (the two if blocks
referencing event.type === "tool_execution_start" / "tool_execution_end") to
accept either camelCase or snake_case by checking both event.toolName ||
event.tool_name (or equivalent) so the flags are set when the payload uses
either key form.
---
Nitpick comments:
In `@internal/session/actor.go`:
- Around line 619-622: Centralize the `"claude-cli"` default by adding a shared
helper ProviderOrDefault(provider string) that does strings.TrimSpace and
returns "claude-cli" when empty, then replace the local logic in actor.go (the
provider := strings.TrimSpace(tc.Provider) / if provider == "" block) with
provider = ProviderOrDefault(tc.Provider) and update internal/pi/executor.go to
call the same ProviderOrDefault so there is a single source of truth for
provider defaulting.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b48550b-f8bd-4b36-ba9f-83792eecb6be
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
go.modinternal/pi/executor.gointernal/pi/extension/codex-appserver-provider.test.mjsinternal/pi/extension/codex-appserver-provider.tsinternal/pi/extension/codex-appserver-smoke.mjsinternal/pi/extension/index.tsinternal/session/actor.gointernal/session/actor_test.go
glittercowboy
force-pushed
the
codex/codex-appserver-provider
branch
from
d96c248 to
48c8cca
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/session/actor.go (1)
619-619: Consider centralizing provider normalization in one layer.Line 619 normalizes with
pi.ProviderOrDefault(...), andpi.NewExecutor(...)also normalizes. Keeping normalization only in the executor reduces drift risk.♻️ Minimal refactor
- provider := pi.ProviderOrDefault(tc.Provider) @@ - Provider: provider, + Provider: tc.Provider,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/actor.go` at line 619, The provider normalization is duplicated: remove the call to pi.ProviderOrDefault at the call site and rely on pi.NewExecutor to normalize consistently; specifically, stop assigning provider := pi.ProviderOrDefault(tc.Provider) and instead pass the raw tc.Provider (or tc.Provider value) into NewExecutor (or any place that expects normalization), so only pi.NewExecutor performs normalization and you avoid drift between pi.ProviderOrDefault and pi.NewExecutor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/session/actor.go`:
- Line 619: The provider normalization is duplicated: remove the call to
pi.ProviderOrDefault at the call site and rely on pi.NewExecutor to normalize
consistently; specifically, stop assigning provider :=
pi.ProviderOrDefault(tc.Provider) and instead pass the raw tc.Provider (or
tc.Provider value) into NewExecutor (or any place that expects normalization),
so only pi.NewExecutor performs normalization and you avoid drift between
pi.ProviderOrDefault and pi.NewExecutor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d19c889-d378-4de7-b107-614d5ec26737
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
go.modinternal/pi/executor.gointernal/pi/extension/codex-appserver-provider.test.mjsinternal/pi/extension/codex-appserver-provider.tsinternal/pi/extension/codex-appserver-smoke.mjsinternal/pi/extension/index.tsinternal/session/actor.gointernal/session/actor_test.go
✅ Files skipped from review due to trivial changes (2)
- go.mod
- internal/pi/extension/codex-appserver-provider.test.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/pi/extension/index.ts
- internal/pi/extension/codex-appserver-smoke.mjs
- internal/pi/extension/codex-appserver-provider.ts
glittercowboy
force-pushed
the
codex/codex-appserver-provider
branch
from
48c8cca to
c6a2ced
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/session/actor_test.go (1)
256-287: Add a whitespace-only provider case.
ProviderOrDefaulttrims whitespace, but this test only covers the empty-string path. A" "case would lock in the defaulting behavior and catch regressions where whitespace is forwarded literally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/actor_test.go` around lines 256 - 287, Add a whitespace-only provider test case to ensure ProviderOrDefault treats strings like " " as empty and falls back to the default; update TestActorPiExecutorDefaultsEmptyProviderToClaudeCli (or add a new test) to set the provider to a whitespace string (e.g., t.Setenv("FAKE_PI_PROVIDER", " ") or equivalent used by testPiOptions), run NewActor and actor.runPiExecutor as in the existing test, then assert the produced args (readNulArgsFile / argIndex checks) still yield "--provider" value "claude-cli"; this verifies ProviderOrDefault trimming behavior and prevents forwarding literal whitespace provider values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/session/actor_test.go`:
- Around line 256-287: Add a whitespace-only provider test case to ensure
ProviderOrDefault treats strings like " " as empty and falls back to the
default; update TestActorPiExecutorDefaultsEmptyProviderToClaudeCli (or add a
new test) to set the provider to a whitespace string (e.g.,
t.Setenv("FAKE_PI_PROVIDER", " ") or equivalent used by testPiOptions), run
NewActor and actor.runPiExecutor as in the existing test, then assert the
produced args (readNulArgsFile / argIndex checks) still yield "--provider" value
"claude-cli"; this verifies ProviderOrDefault trimming behavior and prevents
forwarding literal whitespace provider values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f0fe0948-3cd7-4f39-aff4-0329eedec1ec
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
go.modinternal/pi/executor.gointernal/pi/extension/codex-appserver-provider.test.mjsinternal/pi/extension/codex-appserver-provider.tsinternal/pi/extension/codex-appserver-smoke.mjsinternal/pi/extension/index.tsinternal/session/actor.gointernal/session/actor_test.go
✅ Files skipped from review due to trivial changes (2)
- go.mod
- internal/pi/extension/codex-appserver-provider.test.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/pi/extension/index.ts
- internal/pi/extension/codex-appserver-smoke.mjs
- internal/session/actor.go
- internal/pi/extension/codex-appserver-provider.ts
This was referenced Apr 29, 2026
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
1 participant
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.
Adds codex-appserver as a Pi provider. Preserves Claude SDK persistent session/cache prompt behavior from 9524290 and routes Codex dynamic tool calls through Pi-owned tool execution.
Dependencies:
Summary by CodeRabbit
New Features
Bug Fixes
Tests