PAN-2597#2601
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Codex app-server support as the default transport, including protocol schemas, process hosting, authenticated Unix-socket operations, readiness and delivery handling, approval flows, recovery resume behavior, TUI fallback support, and related documentation and tests. ChangesCodex app-server integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 6
🧹 Nitpick comments (2)
src/lib/overdeck/conversation-delivery.ts (1)
451-494: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate unix-socket-client logic vs.
codex.ts'spostAppServerInterrupt.Both this function and
postAppServerInterruptinsrc/lib/runtimes/codex.tsindependently implement "read token, check socket, POST JSON over unix socket with bridge-token header, parse response."codex.tsalso hardcodes'X-Overdeck-Bridge-Token'instead of importing the sharedBRIDGE_TOKEN_HEADERconstant used here — harmless at runtime (headers are case-insensitive) but a drift risk if the header name ever changes. Consider extracting a shared app-server HTTP client helper (e.g. alongside the manager/host modules) used by both call sites.🤖 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/lib/overdeck/conversation-delivery.ts` around lines 451 - 494, Extract the shared unix-socket app-server request flow from postCodexAppServerOp and codex.ts’s postAppServerInterrupt into a common helper, including socket/token validation, token loading, JSON POSTing, response-status handling, and parsing. Update both callers to use the helper and import the shared BRIDGE_TOKEN_HEADER constant instead of duplicating the header name.src/lib/agents/runtime-command.ts (1)
337-341: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
BRIDGE_TOKEN_HEADERhere instead of duplicating the header name. The casing difference is harmless, but reusing the shared export fromsrc/lib/bridge-token.tskeeps the bridge-token wire name centralized and avoids drift.🤖 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/lib/agents/runtime-command.ts` around lines 337 - 341, Update the headers construction in the runtime command request to use the shared BRIDGE_TOKEN_HEADER export from bridge-token.ts as the property key instead of the hardcoded X-Overdeck-Bridge-Token string, while preserving the existing token value.
🤖 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 `@docs/CODEX-APP-SERVER-PROTOCOL.md`:
- Line 31: Remove the absolute /home/eltmon/Projects/t3code filesystem
references from the documentation links at lines 31 and 37, replacing them with
repository-relative references to codexAppServerManager.ts while preserving the
intended line references.
In `@src/lib/__tests__/launcher-generator.test.ts`:
- Around line 950-969: The test in “codex tui escape hatch preserves the
previous conversation command byte-for-byte” constructs both scripts from
identical configurations, so it never exercises a distinct escape-hatch path.
Change one side to invoke the actual escape-hatch entry point or compare the
generated escape-hatch output against a captured prior-command baseline, while
preserving the byte-for-byte equality assertion.
In `@src/lib/agents/runtime-command.ts`:
- Around line 330-364: Update postAppServerStatus to enforce a per-request
timeout on the httpRequest, using the existing readiness timeout context, and
reject the promise when the request exceeds that limit. Ensure the timeout also
closes or aborts the underlying request so waitForCodexAppServerReady can resume
its deadline checks instead of remaining blocked.
In `@src/lib/overdeck/conversation-delivery.ts`:
- Around line 451-494: Update postCodexAppServerOp to configure a finite timeout
on the httpRequest, and destroy the request when the timeout expires so callers
receive a failure instead of remaining blocked. Preserve the existing response
parsing and error handling, and use the established timeout configuration if one
exists.
- Around line 162-173: Pass the approval request id from the rendered modal
through the codex-approval API into handleConversationCodexApproval, using the
existing toolUseId value from codexConversationPendingInput. Validate that the
submitted id matches the live pending approval before calling
postCodexAppServerOp, and reject stale or mismatched submissions instead of
applying the decision to another request.
In `@src/lib/runtimes/codex.ts`:
- Around line 81-112: The postAppServerInterrupt request can remain pending
indefinitely when the app-server is unresponsive. Add a timeout to the
httpRequest created in postAppServerInterrupt, and destroy the request/socket
when it fires so the Promise rejects and killAgent() continues through its
escalation steps.
---
Nitpick comments:
In `@src/lib/agents/runtime-command.ts`:
- Around line 337-341: Update the headers construction in the runtime command
request to use the shared BRIDGE_TOKEN_HEADER export from bridge-token.ts as the
property key instead of the hardcoded X-Overdeck-Bridge-Token string, while
preserving the existing token value.
In `@src/lib/overdeck/conversation-delivery.ts`:
- Around line 451-494: Extract the shared unix-socket app-server request flow
from postCodexAppServerOp and codex.ts’s postAppServerInterrupt into a common
helper, including socket/token validation, token loading, JSON POSTing,
response-status handling, and parsing. Update both callers to use the helper and
import the shared BRIDGE_TOKEN_HEADER constant instead of duplicating the header
name.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 847e0ebd-6a03-45b2-b6f2-15fc51261519
📒 Files selected for processing (32)
CLAUDE.mdconfiguration/harnesses.mdxdocs/AGENT-STATE-PLANES.mddocs/CODEX-APP-SERVER-PROTOCOL.mdpackages/contracts/src/harness-behavior.tssrc/dashboard/server/services/codex-conversation-parser.tssrc/lib/__tests__/launcher-generator.test.tssrc/lib/agents/__tests__/delivery.test.tssrc/lib/agents/__tests__/resume.test.tssrc/lib/agents/__tests__/runtime-command.test.tssrc/lib/agents/delivery.tssrc/lib/agents/recovery.tssrc/lib/agents/runtime-command.tssrc/lib/agents/spawn.tssrc/lib/codex/__fixtures__/app-server-schema.jsonsrc/lib/codex/__tests__/app-server-host.test.tssrc/lib/codex/__tests__/app-server-manager.test.tssrc/lib/codex/__tests__/fake-app-server.tssrc/lib/codex/app-server-host.tssrc/lib/codex/app-server-manager.tssrc/lib/config-yaml/defaults.tssrc/lib/config-yaml/merge.tssrc/lib/config-yaml/schema.tssrc/lib/cost-parsers/codex-parser.tssrc/lib/launcher-generator.tssrc/lib/overdeck/__tests__/conversation-delivery.test.tssrc/lib/overdeck/conversation-delivery.tssrc/lib/overdeck/conversation-runtime.tssrc/lib/runtimes/behavior.tssrc/lib/runtimes/codex.tssync-sources/hooks/record-cost-event.jstsdown.config.ts
| it('codex tui escape hatch preserves the previous conversation command byte-for-byte', () => { | ||
| const legacy = generateLauncherScriptSync({ | ||
| ...DEFAULT_CONFIG, | ||
| role: 'work', | ||
| harness: 'codex', | ||
| codexMode: 'tui', | ||
| spawnMode: 'conversation', | ||
| useSupervisor: true, | ||
| supervisorScriptPath: '/dist/pty-supervisor.js', | ||
| }); | ||
| const escapeHatch = generateLauncherScriptSync({ | ||
| ...DEFAULT_CONFIG, | ||
| role: 'work', | ||
| harness: 'codex', | ||
| codexMode: 'tui', | ||
| spawnMode: 'conversation', | ||
| useSupervisor: true, | ||
| supervisorScriptPath: '/dist/pty-supervisor.js', | ||
| }); | ||
| expect(escapeHatch).toBe(legacy); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Escape-hatch test compares two identical configs.
legacy and escapeHatch are built from byte-identical option objects, so expect(escapeHatch).toBe(legacy) is trivially true and does not validate the escape-hatch path against the prior TUI command. To be a meaningful regression guard, one side should represent the escape-hatch entry point (or the assertion should compare against a captured baseline string).
🤖 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/lib/__tests__/launcher-generator.test.ts` around lines 950 - 969, The
test in “codex tui escape hatch preserves the previous conversation command
byte-for-byte” constructs both scripts from identical configurations, so it
never exercises a distinct escape-hatch path. Change one side to invoke the
actual escape-hatch entry point or compare the generated escape-hatch output
against a captured prior-command baseline, while preserving the byte-for-byte
equality assertion.
Review CHANGES REQUESTED for PAN-2597Review Synthesis — PAN-2597 — 2026-07-13T00:18:00-04:00Verdict: CHANGES REQUESTED — Codex app-server launch/delivery cannot start a first turn because the selected model is never providedContext
Convoy Status
Blocking Findings[correctness, requirements] App-server first-turn delivery has no model —
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/runtimes/codex.ts (2)
640-644: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUnvalidated
pidinterpolated into a shell command.
pidisn't shell-quoted or validated as numeric before being embedded in thekill -TERMcommand; a multi-pane session could yield newline-separated PIDs that fragment the command.🛡️ Proposed fix
- const pid = stdout.trim() - if (pid) await execAsync(`kill -TERM -- -${pid} 2>/dev/null || kill -TERM ${pid} 2>/dev/null || true`) + const pid = stdout.trim().split('\n')[0]?.trim() + if (pid && /^\d+$/.test(pid)) { + await execAsync(`kill -TERM -- -${pid} 2>/dev/null || kill -TERM ${pid} 2>/dev/null || true`) + }🤖 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/lib/runtimes/codex.ts` around lines 640 - 644, Validate the trimmed output from the tmux pane lookup in the cleanup flow before using it in the kill command. Update the logic around pid so only a single numeric process ID is accepted, rejecting newline-separated or otherwise malformed values, and shell-quote the validated PID when interpolating it into execAsync; preserve the existing cleanup fallback behavior for valid PIDs.
616-630: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
killAgentskips the interrupt step for this runtime’scodex execagents.CodexRuntimeSync.spawnAgent()still launches plaincodex execin tmux and never seeds an app-server socket/token, butkillAgent()now routes step 1 through the globalcodexTransport()default. That makes the interrupt a no-op here and sends these agents straight to the SIGTERM ladder. Gate the app-server branch on per-agent socket/token presence, or keep this path on Ctrl-C.🤖 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/lib/runtimes/codex.ts` around lines 616 - 630, The interrupt logic in killAgent must not select the global codexTransport() app-server path for agents created by CodexRuntimeSync.spawnAgent(). Gate postAppServerInterrupt(agentId) on that agent’s app-server socket/token state, or otherwise retain the tmux C-c path when those credentials are absent, while preserving app-server interrupts for properly initialized agents.
🧹 Nitpick comments (1)
src/lib/runtimes/codex.ts (1)
133-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared config loader for Codex transport
src/lib/runtimes/codex.ts:133-169config-yamlalready mergescodex.transport; this file-by-file YAML reader duplicates path/precedence logic and can drift from the canonical config handling.🤖 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/lib/runtimes/codex.ts` around lines 133 - 169, Replace readCodexTransportFromConfig and its file-by-file YAML helpers with the shared config-yaml loader’s merged configuration, then read codex.transport from that canonical result while preserving the app-server fallback in codexTransport. Remove the duplicated projectConfigPaths and readCodexTransportFromYaml logic.
🤖 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.
Outside diff comments:
In `@src/lib/runtimes/codex.ts`:
- Around line 640-644: Validate the trimmed output from the tmux pane lookup in
the cleanup flow before using it in the kill command. Update the logic around
pid so only a single numeric process ID is accepted, rejecting newline-separated
or otherwise malformed values, and shell-quote the validated PID when
interpolating it into execAsync; preserve the existing cleanup fallback behavior
for valid PIDs.
- Around line 616-630: The interrupt logic in killAgent must not select the
global codexTransport() app-server path for agents created by
CodexRuntimeSync.spawnAgent(). Gate postAppServerInterrupt(agentId) on that
agent’s app-server socket/token state, or otherwise retain the tmux C-c path
when those credentials are absent, while preserving app-server interrupts for
properly initialized agents.
---
Nitpick comments:
In `@src/lib/runtimes/codex.ts`:
- Around line 133-169: Replace readCodexTransportFromConfig and its file-by-file
YAML helpers with the shared config-yaml loader’s merged configuration, then
read codex.transport from that canonical result while preserving the app-server
fallback in codexTransport. Remove the duplicated projectConfigPaths and
readCodexTransportFromYaml logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02a6f93f-6c89-41e9-ad7d-b4a3738b544f
📒 Files selected for processing (18)
docs/CODEX-APP-SERVER-PROTOCOL.mdscripts/circular-deps-baseline.txtscripts/lint-circular-deps.shsrc/dashboard/frontend/src/App/hooks/usePendingInputDialogs.tssrc/dashboard/server/routes/__tests__/conversations-supervisor.test.tssrc/lib/agents/__tests__/delivery.test.tssrc/lib/agents/delivery.tssrc/lib/agents/runtime-command.tssrc/lib/codex/__tests__/app-server-host.test.tssrc/lib/codex/app-server-host.tssrc/lib/config-yaml/roles.tssrc/lib/config-yaml/schema.tssrc/lib/launcher-generator.tssrc/lib/overdeck/__tests__/conversation-delivery.test.tssrc/lib/overdeck/conversation-delivery.tssrc/lib/runtimes/codex.tssrc/lib/runtimes/tmux-cli.tstests/playwright/conversation-supervisor-uat.test.ts
💤 Files with no reviewable changes (1)
- scripts/circular-deps-baseline.txt
🚧 Files skipped from review as they are similar to previous changes (7)
- src/lib/launcher-generator.ts
- docs/CODEX-APP-SERVER-PROTOCOL.md
- src/lib/agents/tests/delivery.test.ts
- src/lib/overdeck/tests/conversation-delivery.test.ts
- src/lib/codex/app-server-host.ts
- src/lib/agents/delivery.ts
- src/lib/overdeck/conversation-delivery.ts
This reverts commit c42c963.
# Conflicts: # src/dashboard/server/routes/__tests__/complete-planning.test.ts
# Conflicts: # src/lib/runtimes/codex.ts
Review CHANGES REQUESTED for PAN-2597Review Synthesis — PAN-2597 — 2026-07-14T22:18:00-04:00Verdict: CHANGES REQUESTED — security report is missing for the active review runContext
Convoy Status
Blocking Findings[security] Active-run security report is missingThe received security signal references run Non-blocking Findings[correctness] [demoted: previously reviewed] App-server threads ignore configured Codex permission mode —
|
1 participant
Issue: #2597
Acceptance Criteria
Implementation Tasks
Summary by CodeRabbit
New Features
Bug Fixes