feat(app-server): run tui and exec through in-process app-server#19
Open
feat(app-server): run tui and exec through in-process app-server#19
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:
📝 WalkthroughWalkthroughIntroduces an in-process app-server client/runtime, expands the protocol (MCP elicitation, plugin list/install, command exec streaming/controls, macOS permissions, web search config), adds command execution manager, plugin marketplace/listing, artifacts runtime management, realtime startup context, tracing/signal handling, and extensive tests and schema/type updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Client App
participant IPC as InProcessAppServerClient
participant RT as InProcess Runtime
participant MP as MessageProcessor
participant DB as State DB
Caller->>IPC: start(args)
IPC->>RT: bootstrap channels
RT->>MP: Initialize(request)
MP->>DB: load state/config
DB-->>MP: state/config
MP-->>RT: Initialized(notification)
RT-->>IPC: ready
Caller->>IPC: request/notify(...)
IPC->>RT: enqueue command
RT->>MP: process (typed path)
MP-->>RT: ServerNotification/ServerRequest
RT-->>IPC: InProcessServerEvent(...)
IPC-->>Caller: next_event()
Note over IPC,RT: shutdown() drains and joins
sequenceDiagram
autonumber
actor Client
participant S as App Server
participant CEM as CommandExecManager
participant OS as OS Process
Client->>S: command/exec (params)
S->>CEM: start(process params)
CEM->>OS: spawn (pty/pipe)
par Stream stdout/stderr
CEM-->>S: command/exec/outputDelta (base64)
S-->>Client: notification(outputDelta)
and Control
Client->>S: command/exec/write/resize/terminate
S->>CEM: write/resize/terminate
CEM->>OS: apply control
end
OS-->>CEM: exit code + buffers
CEM-->>S: CommandExecResponse
S-->>Client: response(exitCode, stdout, stderr)
sequenceDiagram
autonumber
actor User
participant S as App Server
participant MCP as Apps MCP Server
actor CLI as Client Surface
User->>S: Start turn calls MCP tool
S->>MCP: call_tool(...)
MCP-->>S: Elicitation(form/url)
S-->>CLI: ServerRequest mcpServer/elicitation/request
CLI-->>S: Resolve/Reject (action + content/meta)
S->>MCP: create_elicitation_result(action, content)
MCP-->>S: tool result
S-->>CLI: turn/items, completed
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (10)
codex-rs/app-server/src/thread_state.rs (1)
45-55: Consider using doc comments for enum variants.The
ThreadListenerCommandenum and its variants use regular comments (//) while other structs in this file (e.g.,TurnSummary,ThreadState) use doc comments (///). Using doc comments would provide consistent documentation style and enable rustdoc to generate documentation for these variants.✏️ Suggested change
-// ThreadListenerCommand is used to perform operations in the context of the thread listener, for serialization purposes. +/// Commands executed in the thread listener context for serialization purposes. pub(crate) enum ThreadListenerCommand { - // SendThreadResumeResponse is used to resume an already running thread by sending the thread's history to the client and atomically subscribing for new updates. + /// Resume an already running thread by sending history to the client and atomically subscribing for updates. SendThreadResumeResponse(Box<PendingThreadResumeRequest>), - // ResolveServerRequest is used to notify the client that the request has been resolved. - // It is executed in the thread listener's context to ensure that the resolved notification is ordered with regard to the request itself. + /// Notify the client that a server request has been resolved. + /// + /// Executed in the listener's context to ensure ordering with respect to the request. ResolveServerRequest { request_id: RequestId, completion_tx: oneshot::Sender<()>, }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/thread_state.rs` around lines 45 - 55, The enum ThreadListenerCommand and its variants (SendThreadResumeResponse, ResolveServerRequest) are using line comments; change them to doc comments so rustdoc picks them up and documentation style matches other types like TurnSummary and ThreadState—replace the leading // with /// on the enum and each variant, and include brief descriptions for Box<PendingThreadResumeRequest>, RequestId and the completion_tx oneshot::Sender<()> field to explain their roles.codex-rs/README.md (1)
56-58: Minor wording polish: use “in-process” for consistency.Line 56 currently reads as “in process”; hyphenating improves readability and matches the term used elsewhere in the PR context.
Suggested doc tweak
-Both `codex exec` and the interactive TUI run through `codex-app-server` in -process (via `codex-app-server-client`) for a shared conversational runtime. +Both `codex exec` and the interactive TUI run through `codex-app-server` in-process +(via `codex-app-server-client`) for a shared conversational runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/README.md` around lines 56 - 58, Update the README sentence that mentions how codex exec and the interactive TUI run through codex-app-server: change the phrase "in process" to the hyphenated form "in-process" to match project terminology and improve consistency; the sentence references "codex exec", "interactive TUI", "codex-app-server", and "codex-app-server-client".codex-rs/app-server-protocol/schema/json/v2/HistoryEntryReadResponse.json (1)
26-40: Consider makingentrya required property.The
entryproperty is defined but not listed in arequiredarray. This means a valid response could be an empty object{}rather than{ "entry": null }. For API clarity and explicit null handling, consider adding:"properties": { "entry": { ... } }, + "required": [ + "entry" + ], "title": "HistoryEntryReadResponse",This ensures consumers always receive the
entryfield (either with a value or explicitlynull), making client-side handling more predictable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/v2/HistoryEntryReadResponse.json` around lines 26 - 40, HistoryEntryReadResponse's "entry" property is optional right now; make it required so callers always receive an "entry" field (which can be a HistoryEntry or null). Update the JSON schema for HistoryEntryReadResponse by adding a "required": ["entry"] array alongside the existing "properties" so the schema enforces presence of the "entry" property; target the HistoryEntryReadResponse definition (the "entry" property and surrounding object) when applying the change.codex-rs/app-server-protocol/schema/json/v2/McpServerStartupUpdatedNotification.json (1)
36-53: Minor inconsistency:failedvariant is missingtitleproperty.The other
McpServerStartupStatusvariants (starting,ready,cancelled) include atitleproperty, but thefailedvariant does not. For consistency and better tooling support (e.g., code generators that use titles for type names):"required": [ "error", "state" ], + "title": "FailedMcpServerStartupStatus", "type": "object"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/v2/McpServerStartupUpdatedNotification.json` around lines 36 - 53, The "failed" variant in the McpServerStartupUpdatedNotification JSON schema is missing a title property; update the object schema for the failed state (the object under properties with required ["error","state"] that corresponds to the "failed" enum) to include a "title" property consistent with the other McpServerStartupStatus variants (e.g., "title": "failed") so code generators/tooling treat it like the other variants.codex-rs/app-server/src/app_server_tracing.rs (1)
74-80: Use request-specific RPC method labels for in-process spans.Setting both
otel.nameandrpc.methodto"in_process"collapses all typed requests into one trace bucket. Consider recording the concrete request method (variant-level mapping) to preserve actionable observability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/app_server_tracing.rs` around lines 74 - 80, The span currently hardcodes otel.name and rpc.method to "in_process", which collapses all requests; change the info_span! call that creates span (the span variable via info_span!) to use the concrete request method string instead: extract or compute a method name from the incoming request (e.g., add/call a small helper like request_method(req) or match on the request enum/variant to return a &str) and pass that value into otel.name and rpc.method fields of info_span! so each span is labeled by the real RPC method.codex-rs/tui/src/chatwidget.rs (1)
7698-7700: Consolidate op sending throughsubmit_opor wrapper to ensure consistent logging and state setup.The
op_sender()method at line 7698 allows direct channel access, and ops sent through this path can bypass the logging (line 7705) and review task state setup (lines 7706-7708) insubmit_op. While the codebase handles non-active thread ops insubmit_op_to_threadby callinglog_outbound_opexplicitly,Op::Shutdownis sent directly without logging inrequest_shutdown_for_inactive_threads(app.rs:894), and the Review task state setup is skipped for all non-active thread ops. Consider either makingop_senderprivate and routing all ops through controlled submission points, or wrapping the returned sender to enforce consistent side effects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/tui/src/chatwidget.rs` around lines 7698 - 7700, op_sender() exposes a raw UnboundedSender<Op> allowing ops (e.g., Op::Shutdown) to bypass submit_op's logging and review task state setup; make op_sender private or replace it with a controlled wrapper that routes all sends through submit_op (or a new submit_op_wrapper) so that log_outbound_op and the review-task state setup in submit_op/submit_op_to_thread always run; update callers that currently call op_sender().send(...) (including request_shutdown_for_inactive_threads) to use submit_op/submit_op_wrapper and ensure submit_op_to_thread still explicitly calls log_outbound_op for non-active threads.codex-rs/app-server/src/bespoke_event_handling.rs (1)
243-262: Add branch tests for newly introduced event handlers.I don’t see direct tests in this file for the new warning/background/elicitation/undo/MCP-startup/shutdown-complete branches. A small set of focused tests here would reduce regression risk.
Also applies to: 641-678, 1612-1707
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/bespoke_event_handling.rs` around lines 243 - 262, Add focused tests that cover the new event branches (EventMsg::Warning, EventMsg::BackgroundEvent and the other newly added branches like elicitation/undo/MCP-startup/shutdown-complete) by exercising the code paths in bespoke_event_handling.rs: construct the appropriate EventMsg variants and assert that outgoing.send_server_notification is awaited and receives the expected ServerNotification variants (e.g., ServerNotification::Warning with WarningNotification, ServerNotification::BackgroundEvent with BackgroundEventNotification). Use the existing test harness/mocks for outgoing (or implement a simple mock/spy for send_server_notification) to capture sent notifications, and add similar tests for the other branches mentioned (lines ~641-678 and ~1612-1707) to ensure each branch serializes the correct notification type and message. Ensure each test is small, deterministic, and asserts the notification payload matches the event.message.codex-rs/app-server-protocol/src/protocol/common.rs (1)
1596-1691: Consider adding serialization tests for the remaining new notifications.Great coverage overall; adding tests for
BackgroundEvent,ThreadUndoStarted,ThreadUndoCompleted,McpServerStartupCompleted, andSkillsUpdatedwould close the remaining gaps.Also applies to: 911-947
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/src/protocol/common.rs` around lines 1596 - 1691, Add unit tests mirroring the existing serialize_* tests for the missing notification variants: BackgroundEvent, ThreadUndoStarted, ThreadUndoCompleted, McpServerStartupCompleted, and SkillsUpdated by creating ServerNotification::BackgroundEvent(...), ServerNotification::ThreadUndoStarted(...), ServerNotification::ThreadUndoCompleted(...), ServerNotification::McpServerStartupCompleted(...), and ServerNotification::SkillsUpdated(...) instances (using the v2 types like v2::BackgroundEventNotification, v2::ThreadUndoStartedNotification, etc.), then assert their serde_json::to_value outputs equal the expected json!() structures just like the existing serialize_warning_notification and serialize_stream_error_notification tests; place them alongside the other serialize_* tests and follow the same pattern of constructing the notification, building the expected JSON object with correct field names (e.g., "method" and "params"), and using assert_eq!(json!(...), serde_json::to_value(¬ification)?).codex-rs/app-server/src/codex_message_processor.rs (1)
2739-3367: Consider extracting a shared helper for the new thread-op handlers.
thread_undo,thread_shutdown,memory_drop,memory_update,thread_shell_command_run, andhistory_addrepeat the same load/submit/respond/error pattern. A common helper would reduce duplication and future drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/codex_message_processor.rs` around lines 2739 - 3367, Many handlers (thread_undo, thread_shutdown, memory_drop, memory_update, thread_shell_command_run, history_add) duplicate the pattern of calling load_thread, sending load errors, submitting an Op on the thread, and sending a response or internal error—extract a shared async helper (e.g., with_thread_submit) that takes (&self, request_id: ConnectionRequestId, thread_id: String, F) where F is an async closure that receives the loaded thread and returns Result<ResponsePayload, E> (or directly invokes thread.submit(Op::...)). Implement the helper to call self.load_thread(&thread_id).await and on Err call self.outgoing.send_error, on Ok run the closure, map Ok to self.outgoing.send_response(request_id, payload).await and map Err to self.send_internal_error(request_id, format!(...)). Update the listed functions (thread_undo, thread_shutdown, memory_drop, memory_update, thread_shell_command_run, history_add) to call with_thread_submit and pass a closure that performs the thread.submit(Op::...) and returns the appropriate response payload (or turn_id for shell command) to eliminate duplication while preserving existing send_invalid_request_error/send_internal_error behavior.codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json (1)
13811-13851:ThreadContextSnapshotshould mirror the override surface for client reconciliation.Line 15535 allows setting
personality,serviceTier, andsummary, but Line 13811 snapshot payload does not expose them back. That makes state reconciliation lossy.♻️ Proposed schema alignment
"ThreadContextSnapshot": { "properties": { "approvalPolicy": { "$ref": "#/definitions/v2/AskForApproval" }, "cwd": { "type": "string" }, "model": { "type": "string" }, "modelProvider": { "type": "string" }, + "personality": { + "anyOf": [ + { "$ref": "#/definitions/v2/Personality" }, + { "type": "null" } + ] + }, "reasoningEffort": { "anyOf": [ { "$ref": "#/definitions/v2/ReasoningEffort" }, { "type": "null" } ] }, "sandbox": { "$ref": "#/definitions/v2/SandboxPolicy" }, + "serviceTier": { + "anyOf": [ + { "$ref": "#/definitions/v2/ServiceTier" }, + { "type": "null" } + ] + }, + "summary": { + "anyOf": [ + { "$ref": "#/definitions/v2/ReasoningSummary" }, + { "type": "null" } + ] + }, "threadId": { "type": "string" } } }Also applies to: 15535-15654
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json` around lines 13811 - 13851, ThreadContextSnapshot currently omits the client override fields used elsewhere (personality, serviceTier, summary), causing lossy reconciliation; add "personality", "serviceTier", and "summary" to the ThreadContextSnapshot "properties" object and model them with the same schema references/types used by the override surface (use the same v2 definitions referenced at the override location), and keep them optional (not in "required") so the snapshot mirrors the override surface for client reconciliation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codex-rs/app-server-client/src/lib.rs`:
- Around line 469-491: The shutdown method can early-return on command_result
(the `??` after mapping) before performing worker cleanup; change the flow so
you send `ClientCommand::Shutdown { response_tx }`, await the `response_rx` with
`timeout(SHUTDOWN_TIMEOUT, response_rx)`, capture the command_result (or its
mapped IoError) into a local variable instead of using `??` to return
immediately, then always run the worker cleanup logic that awaits or aborts
`worker_handle` (the `timeout(SHUTDOWN_TIMEOUT, &mut worker_handle).await`
branch and abort/await sequence) and only after that return the captured error
(if any). Ensure references to `command_tx`, `response_rx`, `timeout`,
`SHUTDOWN_TIMEOUT`, and `worker_handle` are used so cleanup always runs even
when the shutdown command errors.
In `@codex-rs/app-server-protocol/schema/typescript/v2/HistoryEntryReadParams.ts`:
- Line 5: HistoryEntryReadParams and the HistoryEntry u64 fields are generating
TypeScript bigint types which break JSON serialization; update the Rust
definitions for the affected fields (e.g., the log_id field in the
HistoryEntryReadParams struct and the corresponding u64 in HistoryEntry) to
include the #[ts(type = "number")] annotation so they generate number types in
TypeScript consistent with other fields like
SessionConfiguredEvent.history_log_id, or alternatively change the generated
TypeScript types to string and add conversion at the serialization
boundary—prefer adding #[ts(type = "number")] on the Rust u64 fields to match
project conventions.
In `@codex-rs/app-server-protocol/schema/typescript/v2/ThreadForkResponse.ts`:
- Line 11: The generated TypeScript ThreadForkResponse forces the 64-bit Rust
field history_log_id into a JS number (historyLogId) losing precision; edit the
Rust declaration in protocol v2.rs where history_log_id is annotated with
#[ts(type = "number")] and remove that override so ts-rs will emit bigint for
u64, matching how SessionConfiguredNotification is handled, then regenerate the
TypeScript types.
In `@codex-rs/app-server-protocol/src/protocol/v2.rs`:
- Around line 2497-2504: The HistoryEntry struct's ts field is u64 but missing
the ts-rs override, causing TypeScript to emit bigint; add the ts-rs attribute
to force a number: annotate the ts field in struct HistoryEntry with #[ts(type =
"number")] (matching other u64 fields in this file) so the generated
HistoryEntry.ts uses number for ts and aligns with JSON transport.
In `@codex-rs/app-server/src/codex_message_processor.rs`:
- Around line 3380-3408: The code calls load_thread(&thread_id) but then
performs a global lookup via codex_core::lookup_global_history_entry(log_id,
offset, ...) allowing cross-thread reads; change to use a thread-scoped lookup
or validate the looked-up entry belongs to the requested thread: either call a
thread-aware API (e.g., lookup_thread_history_entry or similar) passing
thread_id along with log_id/offset, or after
codex_core::lookup_global_history_entry verify the returned entry's thread
identifier matches thread_id and return an error (use self.send_internal_error
or self.outgoing.send_error) if it does not; update references around
load_thread, lookup_global_history_entry, and the HistoryEntryReadResponse
construction to ensure only entries from the requested thread are returned.
- Around line 7634-7637: The struct literal for PendingThreadResumeRequest is
currently hard-coding synthetic metadata (history_log_id, history_entry_count,
forked_from_thread_id, network_proxy) which can overwrite true client state;
instead, populate these fields from the real thread/state when constructing
PendingThreadResumeRequest (reuse the same population logic used by the
non-running resume path or pull values from the existing Thread/ThreadMetadata),
and remove the zero/None defaults so running-thread resume responses carry
actual metadata rather than placeholders.
In `@codex-rs/app-server/src/in_process.rs`:
- Around line 205-210: The send on self.client_tx inside shutdown() (the
InProcessClientMessage::Shutdown { done_tx } send) is awaited without a timeout,
which can hang indefinitely if the channel is full; wrap the send in a bounded
wait (e.g. tokio::time::timeout with a short Duration) or use try_send with
fallback, and handle the timeout/error by logging or returning so shutdown() can
proceed to the existing abort/timeout logic. Concretely: in shutdown() replace
the direct .await of self.client_tx.send(...) with a timeout-wrapped send (or
try_send with retry/spawn), detect elapsed or send failure, and ensure you still
signal failure (log/propagate) and continue so the later abort logic runs.
In `@codex-rs/app-server/src/message_processor.rs`:
- Around line 319-329: The handlers process_notification (JSONRPCNotification)
and process_client_notification (ClientNotification) currently log entire
payloads; change them to avoid emitting full protocol objects by logging only
safe, redacted summary fields (e.g., method/type, id, and a masked or
length-only params summary) or a short constant message; update the tracing
calls in process_notification and process_client_notification to extract and log
non-sensitive metadata instead of the full Debug of notification objects, and
ensure any other similar handlers (the ones mentioned around the other
occurrences of full-object logging) follow the same redaction pattern.
In `@codex-rs/exec/src/lib.rs`:
- Around line 750-755: The current spawned task awaits tokio::signal::ctrl_c()
only once and sends to interrupt_tx a single time, so subsequent Ctrl+C presses
are ignored; change the tokio::spawn block that references
interrupt_tx/interrupt_rx and tokio::signal::ctrl_c() to repeatedly await ctrl_c
in a loop and send a notification for every signal (and exit the loop if send
fails because the receiver was dropped), ensuring multiple interrupts are
delivered to interrupt_rx for later turns.
- Around line 147-173: The SessionConfiguredEvent construction in
session_configured_from_start_response currently hardcodes history_log_id and
history_entry_count to 0; replace those zeros by forwarding the values from the
incoming ThreadStartResponse (use response.history_log_id and
response.history_entry_count). Do the same for the sibling converter
(session_configured_from_resume_response) which maps ThreadResumeResponse ->
SessionConfiguredEvent, ensuring both history_log_id and history_entry_count are
copied from the respective response struct fields rather than hardcoded.
In `@codex-rs/tui/src/app_event_sender.rs`:
- Line 21: The matches! call is moving `event` (AppEvent is not Copy) which
breaks later uses; change the pattern to match a reference instead (use `&event`
in the matches! invocation) so it borrows rather than moves, keeping subsequent
uses of `event` valid and preserving the existing patterns `AppEvent::CodexOp(_)
| AppEvent::AgentCommand(_)`.
In `@codex-rs/tui/src/app.rs`:
- Around line 1423-1425: reset_thread_event_state() currently clears
thread_attach_retry_state but does not cancel previously spawned retry tasks, so
those tasks can still enqueue stale AppEvent::ThreadCreated after a reset;
update reset_thread_event_state to first cancel/abort any outstanding retry
tasks (e.g., by storing and signalling a CancellationToken/AbortHandle or
keeping JoinHandles and aborting them) referenced from thread_attach_retry_state
before calling clear(), and harden the retry task callback that enqueues
AppEvent::ThreadCreated to verify the current attach context/state (e.g., check
active_thread_id or a unique attach attempt id) before emitting the event; apply
the same cancellation/validation change to the other reset site around the code
referenced at 3399-3403.
- Around line 3373-3375: The duplicate-scheduling guard currently compares
now.saturating_duration_since(state.last_scheduled_at) against a fixed
THREAD_ATTACH_RETRY_DELAY, which allows later backoff retries (500ms/750ms/...)
to overlap; update both checks (the one using state.last_scheduled_at around the
shown block and the similar one at the other occurrence) to compare against a
dynamic delay derived from the thread's current retry/backoff state instead of
the fixed constant—e.g., compute a delay = THREAD_ATTACH_RETRY_DELAY *
(state.attach_attempts.saturating_add(1)) or use an existing per-thread
next_retry_delay field (state.next_retry_delay) and replace the fixed
THREAD_ATTACH_RETRY_DELAY in the condition with that computed/field value so
retries are de-duplicated according to the actual backoff schedule.
In `@codex-rs/tui/src/chatwidget/agent.rs`:
- Around line 603-605: When handling EventMsg::TurnComplete(_) and
EventMsg::TurnAborted(_) (the match arm that currently sets current_turn_id =
None), also clear the pending_server_requests map so stale approval entries
don't leak into subsequent turns; update the same matching logic that touches
current_turn_id (and the analogous spot near the other TurnComplete/TurnAborted
handler) to call pending_server_requests.clear() or remove only entries
associated with the finished turn id if you prefer more targeted cleanup,
ensuring both current_turn_id and pending_server_requests are synchronized when
a turn ends.
- Around line 992-1017: The code currently removes the pending ID from
pending_server_requests.exec_approvals before awaiting
client.resolve_server_request, which loses the mapping if the async call fails;
instead, look up or clone the request_id without removing it (e.g., use
get(&id).cloned()), call client.resolve_server_request(request_id,
result).await, and only call pending_server_requests.exec_approvals.remove(&id)
after the resolve succeeds; keep the same send_warning_event/send_error_event
flows and ensure errors do not drop the pending entry so users can retry.
---
Nitpick comments:
In
`@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json`:
- Around line 13811-13851: ThreadContextSnapshot currently omits the client
override fields used elsewhere (personality, serviceTier, summary), causing
lossy reconciliation; add "personality", "serviceTier", and "summary" to the
ThreadContextSnapshot "properties" object and model them with the same schema
references/types used by the override surface (use the same v2 definitions
referenced at the override location), and keep them optional (not in "required")
so the snapshot mirrors the override surface for client reconciliation.
In `@codex-rs/app-server-protocol/schema/json/v2/HistoryEntryReadResponse.json`:
- Around line 26-40: HistoryEntryReadResponse's "entry" property is optional
right now; make it required so callers always receive an "entry" field (which
can be a HistoryEntry or null). Update the JSON schema for
HistoryEntryReadResponse by adding a "required": ["entry"] array alongside the
existing "properties" so the schema enforces presence of the "entry" property;
target the HistoryEntryReadResponse definition (the "entry" property and
surrounding object) when applying the change.
In
`@codex-rs/app-server-protocol/schema/json/v2/McpServerStartupUpdatedNotification.json`:
- Around line 36-53: The "failed" variant in the
McpServerStartupUpdatedNotification JSON schema is missing a title property;
update the object schema for the failed state (the object under properties with
required ["error","state"] that corresponds to the "failed" enum) to include a
"title" property consistent with the other McpServerStartupStatus variants
(e.g., "title": "failed") so code generators/tooling treat it like the other
variants.
In `@codex-rs/app-server-protocol/src/protocol/common.rs`:
- Around line 1596-1691: Add unit tests mirroring the existing serialize_* tests
for the missing notification variants: BackgroundEvent, ThreadUndoStarted,
ThreadUndoCompleted, McpServerStartupCompleted, and SkillsUpdated by creating
ServerNotification::BackgroundEvent(...),
ServerNotification::ThreadUndoStarted(...),
ServerNotification::ThreadUndoCompleted(...),
ServerNotification::McpServerStartupCompleted(...), and
ServerNotification::SkillsUpdated(...) instances (using the v2 types like
v2::BackgroundEventNotification, v2::ThreadUndoStartedNotification, etc.), then
assert their serde_json::to_value outputs equal the expected json!() structures
just like the existing serialize_warning_notification and
serialize_stream_error_notification tests; place them alongside the other
serialize_* tests and follow the same pattern of constructing the notification,
building the expected JSON object with correct field names (e.g., "method" and
"params"), and using assert_eq!(json!(...),
serde_json::to_value(¬ification)?).
In `@codex-rs/app-server/src/app_server_tracing.rs`:
- Around line 74-80: The span currently hardcodes otel.name and rpc.method to
"in_process", which collapses all requests; change the info_span! call that
creates span (the span variable via info_span!) to use the concrete request
method string instead: extract or compute a method name from the incoming
request (e.g., add/call a small helper like request_method(req) or match on the
request enum/variant to return a &str) and pass that value into otel.name and
rpc.method fields of info_span! so each span is labeled by the real RPC method.
In `@codex-rs/app-server/src/bespoke_event_handling.rs`:
- Around line 243-262: Add focused tests that cover the new event branches
(EventMsg::Warning, EventMsg::BackgroundEvent and the other newly added branches
like elicitation/undo/MCP-startup/shutdown-complete) by exercising the code
paths in bespoke_event_handling.rs: construct the appropriate EventMsg variants
and assert that outgoing.send_server_notification is awaited and receives the
expected ServerNotification variants (e.g., ServerNotification::Warning with
WarningNotification, ServerNotification::BackgroundEvent with
BackgroundEventNotification). Use the existing test harness/mocks for outgoing
(or implement a simple mock/spy for send_server_notification) to capture sent
notifications, and add similar tests for the other branches mentioned (lines
~641-678 and ~1612-1707) to ensure each branch serializes the correct
notification type and message. Ensure each test is small, deterministic, and
asserts the notification payload matches the event.message.
In `@codex-rs/app-server/src/codex_message_processor.rs`:
- Around line 2739-3367: Many handlers (thread_undo, thread_shutdown,
memory_drop, memory_update, thread_shell_command_run, history_add) duplicate the
pattern of calling load_thread, sending load errors, submitting an Op on the
thread, and sending a response or internal error—extract a shared async helper
(e.g., with_thread_submit) that takes (&self, request_id: ConnectionRequestId,
thread_id: String, F) where F is an async closure that receives the loaded
thread and returns Result<ResponsePayload, E> (or directly invokes
thread.submit(Op::...)). Implement the helper to call
self.load_thread(&thread_id).await and on Err call self.outgoing.send_error, on
Ok run the closure, map Ok to self.outgoing.send_response(request_id,
payload).await and map Err to self.send_internal_error(request_id,
format!(...)). Update the listed functions (thread_undo, thread_shutdown,
memory_drop, memory_update, thread_shell_command_run, history_add) to call
with_thread_submit and pass a closure that performs the thread.submit(Op::...)
and returns the appropriate response payload (or turn_id for shell command) to
eliminate duplication while preserving existing
send_invalid_request_error/send_internal_error behavior.
In `@codex-rs/app-server/src/thread_state.rs`:
- Around line 45-55: The enum ThreadListenerCommand and its variants
(SendThreadResumeResponse, ResolveServerRequest) are using line comments; change
them to doc comments so rustdoc picks them up and documentation style matches
other types like TurnSummary and ThreadState—replace the leading // with /// on
the enum and each variant, and include brief descriptions for
Box<PendingThreadResumeRequest>, RequestId and the completion_tx
oneshot::Sender<()> field to explain their roles.
In `@codex-rs/README.md`:
- Around line 56-58: Update the README sentence that mentions how codex exec and
the interactive TUI run through codex-app-server: change the phrase "in process"
to the hyphenated form "in-process" to match project terminology and improve
consistency; the sentence references "codex exec", "interactive TUI",
"codex-app-server", and "codex-app-server-client".
In `@codex-rs/tui/src/chatwidget.rs`:
- Around line 7698-7700: op_sender() exposes a raw UnboundedSender<Op> allowing
ops (e.g., Op::Shutdown) to bypass submit_op's logging and review task state
setup; make op_sender private or replace it with a controlled wrapper that
routes all sends through submit_op (or a new submit_op_wrapper) so that
log_outbound_op and the review-task state setup in submit_op/submit_op_to_thread
always run; update callers that currently call op_sender().send(...) (including
request_shutdown_for_inactive_threads) to use submit_op/submit_op_wrapper and
ensure submit_op_to_thread still explicitly calls log_outbound_op for non-active
threads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8f305ca-3e0d-4768-9417-ccca81d22d8a
⛔ Files ignored due to path filters (1)
codex-rs/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (118)
codex-rs/Cargo.tomlcodex-rs/README.mdcodex-rs/app-server-client/Cargo.tomlcodex-rs/app-server-client/README.mdcodex-rs/app-server-client/src/lib.rscodex-rs/app-server-protocol/schema/json/ClientRequest.jsoncodex-rs/app-server-protocol/schema/json/McpElicitationRequestParams.jsoncodex-rs/app-server-protocol/schema/json/McpElicitationRequestResponse.jsoncodex-rs/app-server-protocol/schema/json/ServerNotification.jsoncodex-rs/app-server-protocol/schema/json/ServerRequest.jsoncodex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.jsoncodex-rs/app-server-protocol/schema/json/v2/BackgroundEventNotification.jsoncodex-rs/app-server-protocol/schema/json/v2/HistoryAddParams.jsoncodex-rs/app-server-protocol/schema/json/v2/HistoryAddResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/HistoryEntryReadParams.jsoncodex-rs/app-server-protocol/schema/json/v2/HistoryEntryReadResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/McpServerStartupCompletedNotification.jsoncodex-rs/app-server-protocol/schema/json/v2/McpServerStartupUpdatedNotification.jsoncodex-rs/app-server-protocol/schema/json/v2/McpToolsListParams.jsoncodex-rs/app-server-protocol/schema/json/v2/McpToolsListResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/MemoryDropParams.jsoncodex-rs/app-server-protocol/schema/json/v2/MemoryDropResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/MemoryUpdateParams.jsoncodex-rs/app-server-protocol/schema/json/v2/MemoryUpdateResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/PromptCustomListParams.jsoncodex-rs/app-server-protocol/schema/json/v2/PromptCustomListResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/SkillsUpdatedNotification.jsoncodex-rs/app-server-protocol/schema/json/v2/StreamErrorNotification.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadConfigReloadParams.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadConfigReloadResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadShellCommandRunParams.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadShellCommandRunResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadShutdownCompletedNotification.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadShutdownParams.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadShutdownResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadTurnContextSetParams.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadTurnContextSetResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadUndoCompletedNotification.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadUndoParams.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadUndoResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadUndoStartedNotification.jsoncodex-rs/app-server-protocol/schema/json/v2/WarningNotification.jsoncodex-rs/app-server-protocol/schema/typescript/ClientRequest.tscodex-rs/app-server-protocol/schema/typescript/ServerNotification.tscodex-rs/app-server-protocol/schema/typescript/ServerRequest.tscodex-rs/app-server-protocol/schema/typescript/v2/BackgroundEventNotification.tscodex-rs/app-server-protocol/schema/typescript/v2/HistoryAddParams.tscodex-rs/app-server-protocol/schema/typescript/v2/HistoryAddResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/HistoryEntry.tscodex-rs/app-server-protocol/schema/typescript/v2/HistoryEntryReadParams.tscodex-rs/app-server-protocol/schema/typescript/v2/HistoryEntryReadResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/McpElicitationDecision.tscodex-rs/app-server-protocol/schema/typescript/v2/McpElicitationRequestParams.tscodex-rs/app-server-protocol/schema/typescript/v2/McpElicitationRequestResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/McpServerStartupCompletedNotification.tscodex-rs/app-server-protocol/schema/typescript/v2/McpServerStartupFailure.tscodex-rs/app-server-protocol/schema/typescript/v2/McpServerStartupStatus.tscodex-rs/app-server-protocol/schema/typescript/v2/McpServerStartupUpdatedNotification.tscodex-rs/app-server-protocol/schema/typescript/v2/McpToolsListParams.tscodex-rs/app-server-protocol/schema/typescript/v2/McpToolsListResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/MemoryDropParams.tscodex-rs/app-server-protocol/schema/typescript/v2/MemoryDropResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/MemoryUpdateParams.tscodex-rs/app-server-protocol/schema/typescript/v2/MemoryUpdateResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/PromptCustomListParams.tscodex-rs/app-server-protocol/schema/typescript/v2/PromptCustomListResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/SessionNetworkProxyRuntime.tscodex-rs/app-server-protocol/schema/typescript/v2/SkillsUpdatedNotification.tscodex-rs/app-server-protocol/schema/typescript/v2/StreamErrorNotification.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadConfigReloadParams.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadConfigReloadResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadContextSnapshot.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadForkResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadResumeResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadShellCommandRunParams.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadShellCommandRunResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadShutdownCompletedNotification.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadShutdownParams.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadShutdownResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadStartResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadTurnContextSetParams.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadTurnContextSetResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadUndoCompletedNotification.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadUndoParams.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadUndoResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/ThreadUndoStartedNotification.tscodex-rs/app-server-protocol/schema/typescript/v2/WarningNotification.tscodex-rs/app-server-protocol/schema/typescript/v2/index.tscodex-rs/app-server-protocol/src/protocol/common.rscodex-rs/app-server-protocol/src/protocol/v2.rscodex-rs/app-server/README.mdcodex-rs/app-server/src/app_server_tracing.rscodex-rs/app-server/src/bespoke_event_handling.rscodex-rs/app-server/src/codex_message_processor.rscodex-rs/app-server/src/in_process.rscodex-rs/app-server/src/lib.rscodex-rs/app-server/src/message_processor.rscodex-rs/app-server/src/outgoing_message.rscodex-rs/app-server/src/thread_state.rscodex-rs/app-server/src/transport.rscodex-rs/core/src/lib.rscodex-rs/exec/Cargo.tomlcodex-rs/exec/src/lib.rscodex-rs/tui/Cargo.tomlcodex-rs/tui/src/agent_command.rscodex-rs/tui/src/app.rscodex-rs/tui/src/app_event.rscodex-rs/tui/src/app_event_sender.rscodex-rs/tui/src/bottom_pane/approval_overlay.rscodex-rs/tui/src/bottom_pane/mod.rscodex-rs/tui/src/chatwidget.rscodex-rs/tui/src/chatwidget/agent.rscodex-rs/tui/src/chatwidget/tests.rscodex-rs/tui/src/lib.rscodex-rs/tui/src/status_indicator_widget.rs
|
|
||
| // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. | ||
|
|
||
| export type HistoryEntryReadParams = { threadId: string, logId: bigint, offset: number, }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Step 1: Confirm bigint in generated params type ==="
if [ -f "codex-rs/app-server-protocol/schema/typescript/v2/HistoryEntryReadParams.ts" ]; then
cat codex-rs/app-server-protocol/schema/typescript/v2/HistoryEntryReadParams.ts
else
echo "File not found at specified path, searching..."
find . -name "HistoryEntryReadParams.ts" 2>/dev/null | head -5
fiRepository: fcoury/codex
Length of output: 355
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Step 2: Locate JSON serialization call sites ==="
rg -nP 'JSON\.stringify' --type typescript --type javascript codex-rs/app-server-protocol -A 2 -B 2 | head -100Repository: fcoury/codex
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Step 3: Search for BigInt handling mechanisms ==="
rg -nP 'BigInt|replacer|reviver' --type typescript --type javascript codex-rs/app-server-protocol -B 2 -A 2 | head -100Repository: fcoury/codex
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Step 4: Find where HistoryEntryReadParams is used ==="
rg -nP 'HistoryEntryReadParams' --type typescript --type javascript codex-rs/app-server-protocol -B 2 -A 3 | head -150Repository: fcoury/codex
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Step 2: Locate JSON serialization call sites ==="
rg -nP 'JSON\.stringify' codex-rs/app-server-protocol -A 2 -B 2 | head -120Repository: fcoury/codex
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Step 3: Search for BigInt handling ==="
rg -i 'BigInt|replacer|reviver' codex-rs/app-server-protocol -B 2 -A 2 | head -120Repository: fcoury/codex
Length of output: 7363
🏁 Script executed:
#!/bin/bash
#!/bin/bash
set -euo pipefail
echo "=== Step 4: Find usages of HistoryEntryReadParams ==="
rg 'HistoryEntryReadParams' codex-rs/app-server-protocol -B 2 -A 5 | head -150Repository: fcoury/codex
Length of output: 15712
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Step 5: Check for WebSocket or stdio transport mechanisms ==="
rg -nP 'send|transport|serialize' codex-rs/app-server-protocol -A 1 -B 1 --max-count=50Repository: fcoury/codex
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find TypeScript/JavaScript client code that uses HistoryEntryReadParams
echo "=== Finding TypeScript client code using HistoryEntryReadParams ==="
find . -name "*.ts" -o -name "*.js" | xargs grep -l "HistoryEntryReadParams" 2>/dev/null | grep -v schema | grep -v node_modules | head -20Repository: fcoury/codex
Length of output: 123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how requests are actually constructed and sent
echo "=== Check for request construction/transmission code ==="
rg -i "history.*entry.*read|sendRequest|fetch|JSON.stringify" --max-count=30 --type-list | head -5
find . -name "*.ts" -o -name "*.js" | xargs grep -l "history/entry/read" 2>/dev/null | grep -v schema | head -10Repository: fcoury/codex
Length of output: 519
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at the Rust source definition more closely
echo "=== Examining Rust source definition of HistoryEntryReadParams ==="
cat codex-rs/app-server-protocol/src/protocol/v2.rs | grep -A 10 "pub struct HistoryEntryReadParams"Repository: fcoury/codex
Length of output: 426
🏁 Script executed:
#!/bin/bash
set -euso pipefail
# Check ts-rs configuration and how it handles u64
echo "=== Check how ts-rs handles u64 and if there's custom handling ==="
rg "ts-rs|u64.*ts|bigint" codex-rs/app-server-protocol/src/protocol/v2.rs | head -30Repository: fcoury/codex
Length of output: 353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Looking at JSON schema for HistoryEntryReadParams ==="
cat codex-rs/app-server-protocol/schema/json/v2/HistoryEntryReadParams.jsonRepository: fcoury/codex
Length of output: 549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if bigint is ever used in generated TS files ==="
grep -r "bigint" codex-rs/app-server-protocol/schema/typescript/v2/ --include="*.ts" | head -20Repository: fcoury/codex
Length of output: 1633
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Look for any serialization adapter or custom handlers ==="
find codex-rs -name "*.rs" | xargs grep -l "serialize.*u64\|u64.*serialize" 2>/dev/null | head -5Repository: fcoury/codex
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check README or documentation about wire format ==="
if [ -f "README.md" ]; then head -100 README.md; fi
find . -name "*PROTOCOL*" -o -name "*protocol*readme*" 2>/dev/null | head -5Repository: fcoury/codex
Length of output: 2736
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Critical: Check if JSON schema is actually used and if uint64 is compatible ==="
cat codex-rs/app-server-protocol/schema/json/v2/HistoryEntryReadParams.json | jq '.properties.logId'Repository: fcoury/codex
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for ANY TypeScript/JavaScript client code in repo ==="
find . -path ./node_modules -prune -o -type f \( -name "*.ts" -o -name "*.js" \) -print | grep -v schema | grep -v node_modules | head -20Repository: fcoury/codex
Length of output: 890
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if there are actual client implementations ==="
find . -name "*client*" -type f | grep -v node_modules | grep -v ".git" | head -20Repository: fcoury/codex
Length of output: 885
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Look at how the protocol documentation describes wire format ==="
find codex-rs/app-server-protocol -name "*.md" -o -name "*.txt" | xargs cat 2>/dev/null | head -100Repository: fcoury/codex
Length of output: 124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check ts-rs documentation or comments about u64 handling ==="
grep -r "ts-rs\|u64" codex-rs/app-server-protocol/src --include="*.rs" -B 2 -A 2 | head -60Repository: fcoury/codex
Length of output: 3627
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check ALL u64 fields in v2.rs and their ts-rs annotations ==="
grep -B 2 "pub.*:\s*u64" codex-rs/app-server-protocol/src/protocol/v2.rs | grep -E "(pub struct|pub.*:\s*u64|#\[ts)" | head -80Repository: fcoury/codex
Length of output: 389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if SDK typescript imports from generated types ==="
grep -r "from.*protocol\|import.*HistoryEntryReadParams" sdk/typescript/src --include="*.ts" 2>/dev/nullRepository: fcoury/codex
Length of output: 235
🏁 Script executed:
#!/antml/bash
set -euo pipefail
echo "=== Get full context of HistoryEntryReadParams struct in Rust ==="
sed -n '545,560p' codex-rs/app-server-protocol/src/protocol/v2.rsRepository: fcoury/codex
Length of output: 577
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find exact line number and context of HistoryEntryReadParams ==="
grep -n "struct HistoryEntryReadParams" codex-rs/app-server-protocol/src/protocol/v2.rsRepository: fcoury/codex
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Get full HistoryEntryReadParams struct with decorators ==="
awk '/pub struct HistoryEntryReadParams/,/^}/' codex-rs/app-server-protocol/src/protocol/v2.rs | head -20Repository: fcoury/codex
Length of output: 227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Compare HistoryEntryReadParams with SessionConfiguredEvent which HAS the annotation ==="
echo "--- SessionConfiguredEvent (HAS #[ts(type='number')] on history_log_id) ---"
awk '/pub struct SessionConfiguredEvent/,/^}/' codex-rs/app-server-protocol/src/protocol/v2.rs | head -30
echo ""
echo "--- HistoryEntryReadParams (NO annotation) ---"
awk '/pub struct HistoryEntryReadParams/,/^}/' codex-rs/app-server-protocol/src/protocol/v2.rs | head -20Repository: fcoury/codex
Length of output: 380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if there are other u64 fields without the annotation ==="
rg -A 1 "pub.*:\s*u64[,\s]" codex-rs/app-server-protocol/src/protocol/v2.rs | grep -B 1 "pub.*:\s*u64" | grep -v "ts\(type" | head -40Repository: fcoury/codex
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Get lines 2485-2500 to see all decorators for HistoryEntryReadParams ==="
sed -n '2485,2500p' codex-rs/app-server-protocol/src/protocol/v2.rsRepository: fcoury/codex
Length of output: 597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find ALL u64 fields lacking #[ts(...)] annotation in v2.rs ==="
grep -n "pub.*u64" codex-rs/app-server-protocol/src/protocol/v2.rs | grep -v "#"Repository: fcoury/codex
Length of output: 272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check lines around HistoryEntryReadParams and surrounding types ==="
sed -n '2480,2520p' codex-rs/app-server-protocol/src/protocol/v2.rsRepository: fcoury/codex
Length of output: 1248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Look for ts-rs crate version and BigInt documentation ==="
grep -r "ts-rs\|serde_json" codex-rs/app-server-protocol/Cargo.toml | head -10Repository: fcoury/codex
Length of output: 180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check lines 1890-1910 to see SessionConfiguredEvent with the annotation ==="
sed -n '1890,1910p' codex-rs/app-server-protocol/src/protocol/v2.rsRepository: fcoury/codex
Length of output: 799
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify if HistoryEntryReadParams is actually part of wire protocol ==="
grep -n "HistoryEntryRead" codex-rs/app-server-protocol/src/protocol/common.rs | head -20Repository: fcoury/codex
Length of output: 396
Missing #[ts(type = "number")] annotation on u64 fields breaks TypeScript serialization.
logId: bigint and related u64 fields in HistoryEntryReadParams and HistoryEntry are generated without the #[ts(type = "number")] annotation present on other similar u64 fields (e.g., SessionConfiguredEvent.history_log_id). This causes bigint type generation in TypeScript, which cannot serialize with vanilla JSON.stringify() (throws TypeError). Wire transport requires either:
- Add
#[ts(type = "number")]to Rust field (matches codebase pattern for u64 fields), or - Change TypeScript type to
stringand convert at boundaries
Affected fields in v2.rs
- Line 2493:
HistoryEntryReadParams.log_id: u64(missing annotation) - Line 2502:
HistoryEntry.ts: u64(missing annotation)
Compare with line 1899: SessionConfiguredEvent.history_log_id: u64 which correctly has #[ts(type = "number")]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/app-server-protocol/schema/typescript/v2/HistoryEntryReadParams.ts`
at line 5, HistoryEntryReadParams and the HistoryEntry u64 fields are generating
TypeScript bigint types which break JSON serialization; update the Rust
definitions for the affected fields (e.g., the log_id field in the
HistoryEntryReadParams struct and the corresponding u64 in HistoryEntry) to
include the #[ts(type = "number")] annotation so they generate number types in
TypeScript consistent with other fields like
SessionConfiguredEvent.history_log_id, or alternatively change the generated
TypeScript types to string and add conversion at the serialization
boundary—prefer adding #[ts(type = "number")] on the Rust u64 fields to match
project conventions.
| import type { Thread } from "./Thread"; | ||
|
|
||
| export type ThreadForkResponse = { thread: Thread, model: string, modelProvider: string, serviceTier: ServiceTier | null, cwd: string, approvalPolicy: AskForApproval, sandbox: SandboxPolicy, reasoningEffort: ReasoningEffort | null, }; | ||
| export type ThreadForkResponse = { thread: Thread, model: string, modelProvider: string, serviceTier: ServiceTier | null, cwd: string, approvalPolicy: AskForApproval, sandbox: SandboxPolicy, reasoningEffort: ReasoningEffort | null, historyLogId: number, historyEntryCount: number, forkedFromThreadId: string | null, networkProxy: SessionNetworkProxyRuntime | null, }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check ThreadForkResponse and related field definitions across the codebase
echo "=== Searching for ThreadForkResponse in Rust ==="
rg -n "ThreadForkResponse|historyLogId|historyEntryCount|history_log_id|history_entry_count" codex-rs -g '*.rs' | head -50
echo -e "\n=== Searching for definitions in schema/JSON ==="
rg -n "ThreadForkResponse|historyLogId|historyEntryCount|history_log_id|history_entry_count" codex-rs -g '*.json' -g '*.yaml' -g '*.yml' | head -50
echo -e "\n=== Searching for TS type definitions ==="
rg -n "ThreadForkResponse|historyLogId|historyEntryCount" codex-rs -g '*.ts' | head -50Repository: fcoury/codex
Length of output: 10348
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check ThreadForkResponse.json schema definition
echo "=== ThreadForkResponse.json historyLogId/historyEntryCount definition ==="
jq '.properties | {historyLogId, historyEntryCount}' codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json
echo -e "\n=== SessionConfiguredNotification.json historyLogId definition (for comparison) ==="
jq '.properties | {historyLogId, historyEntryCount}' codex-rs/app-server-protocol/schema/json/v2/SessionConfiguredNotification.json
# Check if there's a proto or rust source for ThreadForkResponse
echo -e "\n=== Searching for Rust ThreadForkResponse definition ==="
rg -A 10 "^pub struct ThreadForkResponse|^pub enum ThreadForkResponse" codex-rs -g '*.rs' | head -30Repository: fcoury/codex
Length of output: 552
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find SessionConfiguredNotification files
echo "=== Finding SessionConfiguredNotification files ==="
fd -t f "SessionConfiguredNotification" codex-rs
echo -e "\n=== Check SessionConfiguredNotification.ts ==="
cat -n codex-rs/app-server-protocol/schema/typescript/v2/SessionConfiguredNotification.ts
# Check for any u64 → bigint codegen rules
echo -e "\n=== Search for codegen config or transformer rules ==="
fd -t f "Codegen\|codegen\|transformer\|tsconfig" codex-rs/app-server-protocol --max-depth 3 | head -20Repository: fcoury/codex
Length of output: 367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check SessionConfiguredNotification.ts (no v2 prefix)
echo "=== SessionConfiguredNotification.ts (root typescript dir) ==="
cat -n codex-rs/app-server-protocol/schema/typescript/SessionConfiguredNotification.ts
# Also check if there's a v2 version
echo -e "\n=== Checking for v2 version ==="
find codex-rs/app-server-protocol/schema/typescript -name "*SessionConfiguredNotification*"
# Look for codegen/transformer configuration
echo -e "\n=== Searching for codegen configuration ==="
fd -t f "\.ts$" codex-rs/app-server-protocol --max-depth 1 | head -20
ls -la codex-rs/app-server-protocol/ | grep -i build\|codegen\|genRepository: fcoury/codex
Length of output: 958
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if v2/ThreadForkResponse.ts is generated or hand-written
echo "=== ThreadForkResponse.ts header ==="
head -5 codex-rs/app-server-protocol/schema/typescript/v2/ThreadForkResponse.ts
# Search for Rust ThreadForkResponse struct
echo -e "\n=== Rust ThreadForkResponse struct definition ==="
rg -B 2 -A 15 "pub struct ThreadForkResponse" codex-rs -g '*.rs'
# Check for u64 in proto or Rust serde definitions
echo -e "\n=== Searching for history_log_id and history_entry_count in Rust structs ==="
rg -B 3 -A 3 "history_log_id|history_entry_count" codex-rs -g '*.rs' | grep -A 3 "struct\|u64\|usize" | head -40Repository: fcoury/codex
Length of output: 5167
Remove #[ts(type = "number")] overrides for 64-bit integer fields in Rust source.
history_log_id is u64 in Rust but forced to TypeScript number via an explicit #[ts(type = "number")] attribute override in codex-rs/app-server-protocol/src/protocol/v2.rs. This loses precision above 2^53 - 1. Remove the override to allow ts-rs to generate bigint, consistent with how SessionConfiguredNotification handles the same field elsewhere in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/app-server-protocol/schema/typescript/v2/ThreadForkResponse.ts` at
line 11, The generated TypeScript ThreadForkResponse forces the 64-bit Rust
field history_log_id into a JS number (historyLogId) losing precision; edit the
Rust declaration in protocol v2.rs where history_log_id is annotated with
#[ts(type = "number")] and remove that override so ts-rs will emit bigint for
u64, matching how SessionConfiguredNotification is handled, then regenerate the
TypeScript types.
Comment on lines
+2497
to
+2504
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] | ||
| #[serde(rename_all = "camelCase")] | ||
| #[ts(export_to = "v2/")] | ||
| pub struct HistoryEntry { | ||
| pub conversation_id: String, | ||
| pub ts: u64, | ||
| pub text: String, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify Rust source currently lacks a TS type override for HistoryEntry.ts
rg -n 'pub struct HistoryEntry|pub ts: u64|#\[ts\(type = "number"\)\]' codex-rs/app-server-protocol/src/protocol/v2.rs
# Verify generated TS currently uses bigint for ts
rg -n 'export type HistoryEntry|ts: bigint|ts: number' codex-rs/app-server-protocol/schema/typescript/v2/HistoryEntry.tsRepository: fcoury/codex
Length of output: 788
HistoryEntry.ts emits bigint for JSON transport—should emit number.
The ts field at line 2502 is u64 without a ts-rs override, generating ts: bigint in the TypeScript definition. JSON cannot safely transport bigint, creating a type mismatch for clients. Other u64 fields throughout the file correctly use #[ts(type = "number")], so this is an inconsistency.
Fix
pub struct HistoryEntry {
pub conversation_id: String,
+ #[ts(type = "number")]
pub ts: u64,
pub text: String,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] | |
| #[serde(rename_all = "camelCase")] | |
| #[ts(export_to = "v2/")] | |
| pub struct HistoryEntry { | |
| pub conversation_id: String, | |
| pub ts: u64, | |
| pub text: String, | |
| } | |
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] | |
| #[serde(rename_all = "camelCase")] | |
| #[ts(export_to = "v2/")] | |
| pub struct HistoryEntry { | |
| pub conversation_id: String, | |
| #[ts(type = "number")] | |
| pub ts: u64, | |
| pub text: String, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/app-server-protocol/src/protocol/v2.rs` around lines 2497 - 2504,
The HistoryEntry struct's ts field is u64 but missing the ts-rs override,
causing TypeScript to emit bigint; add the ts-rs attribute to force a number:
annotate the ts field in struct HistoryEntry with #[ts(type = "number")]
(matching other u64 fields in this file) so the generated HistoryEntry.ts uses
number for ts and aligns with JSON transport.
Comment on lines
+3380
to
+3408
| let (_, _thread) = match self.load_thread(&thread_id).await { | ||
| Ok(v) => v, | ||
| Err(error) => { | ||
| self.outgoing.send_error(request_id, error).await; | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| let config = Arc::clone(&self.config); | ||
| let entry = match tokio::task::spawn_blocking(move || { | ||
| codex_core::lookup_global_history_entry(log_id, offset, config.as_ref()) | ||
| }) | ||
| .await | ||
| { | ||
| Ok(entry) => entry, | ||
| Err(err) => { | ||
| self.send_internal_error( | ||
| request_id, | ||
| format!("failed to lookup history entry: {err}"), | ||
| ) | ||
| .await; | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| let response = HistoryEntryReadResponse { | ||
| entry: entry.map(Into::into), | ||
| }; | ||
| self.outgoing.send_response(request_id, response).await; |
There was a problem hiding this comment.
Scope history/entry/read to the requested thread before global lookup.
Line 3380 validates the thread exists, but Line 3389 reads by caller-provided log_id and offset globally without verifying that log_id belongs to that thread. This allows cross-thread history reads if a log_id is known.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/app-server/src/codex_message_processor.rs` around lines 3380 - 3408,
The code calls load_thread(&thread_id) but then performs a global lookup via
codex_core::lookup_global_history_entry(log_id, offset, ...) allowing
cross-thread reads; change to use a thread-scoped lookup or validate the
looked-up entry belongs to the requested thread: either call a thread-aware API
(e.g., lookup_thread_history_entry or similar) passing thread_id along with
log_id/offset, or after codex_core::lookup_global_history_entry verify the
returned entry's thread identifier matches thread_id and return an error (use
self.send_internal_error or self.outgoing.send_error) if it does not; update
references around load_thread, lookup_global_history_entry, and the
HistoryEntryReadResponse construction to ensure only entries from the requested
thread are returned.
codex-rs/tui/src/app_event_sender.rs
Outdated
| // Record inbound events for high-fidelity session replay. | ||
| // Avoid double-logging Ops; those are logged at the point of submission. | ||
| if !matches!(event, AppEvent::CodexOp(_)) { | ||
| if !matches!(event, AppEvent::CodexOp(_) | AppEvent::AgentCommand(_)) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find app_event.rs
APP_EVENT_FILE="$(fd '^app_event\.rs$' 2>/dev/null | head -n 1)"
if [[ -n "${APP_EVENT_FILE}" ]]; then
echo "=== AppEvent definition and Copy trait ==="
cat -n "${APP_EVENT_FILE}"
else
echo "Could not find app_event.rs, searching by pattern..."
rg -l "enum\s+AppEvent" 2>/dev/null | head -n 1
fi
echo
echo "=== app_event_sender.rs context around line 21 ==="
wc -l codex-rs/tui/src/app_event_sender.rs
head -n 30 codex-rs/tui/src/app_event_sender.rs | cat -nRepository: fcoury/codex
Length of output: 20633
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if AppEvent implements Copy
echo "=== Searching for Copy impl for AppEvent ==="
rg "impl\s+Copy\s+for\s+AppEvent|#\[derive\([^)]*Copy" codex-rs/tui/src/
# Check the full enum derive on AppEvent
echo
echo "=== Full AppEvent enum derive clause ==="
rg -B2 "pub\(crate\)\s+enum\s+AppEvent" codex-rs/tui/src/app_event.rs | head -n 5
# Check the full app_event_sender.rs file to see if it compiles
echo
echo "=== Full app_event_sender.rs for compilation context ==="
cat codex-rs/tui/src/app_event_sender.rsRepository: fcoury/codex
Length of output: 7328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if this file has any tests or if it's actively used
echo "=== Checking for tests or usage of AppEventSender::send ==="
rg -l "AppEventSender|app_event_sender" codex-rs/tui/src/ | head -10
# Look for any compiler warnings or errors in the build
echo
echo "=== Checking for any related compiler artifacts or error docs ==="
find . -name "*.rs" -path "*/codex-rs/tui/src/*" | wc -l
# Double-check the exact line being reviewed
echo
echo "=== Exact context around line 21 with line numbers ==="
sed -n '15,27p' codex-rs/tui/src/app_event_sender.rs | cat -nRepository: fcoury/codex
Length of output: 1299
Use a reference in matches! to avoid moving event before subsequent uses.
AppEvent does not implement Copy, so the matches! macro on line 21 moves event. Since event is used again on lines 22 and 24, pass a reference to matches! instead:
Fix
- if !matches!(event, AppEvent::CodexOp(_) | AppEvent::AgentCommand(_)) {
+ if !matches!(&event, AppEvent::CodexOp(_) | AppEvent::AgentCommand(_)) {
session_log::log_inbound_app_event(&event);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if !matches!(event, AppEvent::CodexOp(_) | AppEvent::AgentCommand(_)) { | |
| if !matches!(&event, AppEvent::CodexOp(_) | AppEvent::AgentCommand(_)) { | |
| session_log::log_inbound_app_event(&event); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/tui/src/app_event_sender.rs` at line 21, The matches! call is moving
`event` (AppEvent is not Copy) which breaks later uses; change the pattern to
match a reference instead (use `&event` in the matches! invocation) so it
borrows rather than moves, keeping subsequent uses of `event` valid and
preserving the existing patterns `AppEvent::CodexOp(_) |
AppEvent::AgentCommand(_)`.
codex-rs/tui/src/app.rs
Outdated
Comment on lines
1423
to
1425
| self.thread_attach_retry_state.clear(); | ||
| self.suppress_shutdown_complete = false; | ||
| self.active_thread_id = None; |
There was a problem hiding this comment.
Scheduled attach-retry tasks are not canceled on thread-state reset
reset_thread_event_state() clears retry metadata, but previously spawned retry tasks still fire and can enqueue stale AppEvent::ThreadCreated later. That can resurrect old attach flows after session reset.
Suggested mitigation
// App fields
- thread_attach_retry_state: HashMap<ThreadId, ThreadAttachRetryState>,
+ thread_attach_retry_state: HashMap<ThreadId, ThreadAttachRetryState>,
+ thread_attach_retry_tasks: HashMap<ThreadId, JoinHandle<()>>,
fn clear_thread_attach_retry_state(&mut self, thread_id: ThreadId) {
self.thread_attach_retry_state.remove(&thread_id);
+ if let Some(handle) = self.thread_attach_retry_tasks.remove(&thread_id) {
+ handle.abort();
+ }
}
fn reset_thread_event_state(&mut self) {
self.abort_all_thread_event_listeners();
self.thread_event_channels.clear();
self.agent_picker_threads.clear();
self.thread_attach_retry_state.clear();
+ for (_, handle) in self.thread_attach_retry_tasks.drain() {
+ handle.abort();
+ }
self.suppress_shutdown_complete = false;
...
}
- tokio::spawn(async move {
+ let handle = tokio::spawn(async move {
tokio::time::sleep(delay).await;
app_event_tx.send(AppEvent::ThreadCreated(thread_id));
});
+ if let Some(previous) = self.thread_attach_retry_tasks.insert(thread_id, handle) {
+ previous.abort();
+ }Also applies to: 3399-3403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/tui/src/app.rs` around lines 1423 - 1425, reset_thread_event_state()
currently clears thread_attach_retry_state but does not cancel previously
spawned retry tasks, so those tasks can still enqueue stale
AppEvent::ThreadCreated after a reset; update reset_thread_event_state to first
cancel/abort any outstanding retry tasks (e.g., by storing and signalling a
CancellationToken/AbortHandle or keeping JoinHandles and aborting them)
referenced from thread_attach_retry_state before calling clear(), and harden the
retry task callback that enqueues AppEvent::ThreadCreated to verify the current
attach context/state (e.g., check active_thread_id or a unique attach attempt
id) before emitting the event; apply the same cancellation/validation change to
the other reset site around the code referenced at 3399-3403.
codex-rs/tui/src/app.rs
Outdated
Comment on lines
+3373
to
+3375
| if let Some(last_scheduled_at) = state.last_scheduled_at | ||
| && now.saturating_duration_since(last_scheduled_at) < THREAD_ATTACH_RETRY_DELAY | ||
| { |
There was a problem hiding this comment.
Retry de-duplication window is too short for later backoff attempts
The duplicate-scheduling guard always uses a fixed 250ms window, but retries are scheduled at 500ms/750ms/... on later attempts. That allows overlapping retries for the same thread and can burn through attempts faster than intended.
Suggested fix
fn maybe_schedule_thread_attach_retry(&mut self, thread_id: ThreadId, err: &str) {
let now = Instant::now();
let state = self.thread_attach_retry_state.entry(thread_id).or_default();
- if let Some(last_scheduled_at) = state.last_scheduled_at
- && now.saturating_duration_since(last_scheduled_at) < THREAD_ATTACH_RETRY_DELAY
- {
- tracing::debug!(
- "attach retry already pending for thread {thread_id}; suppressing duplicate scheduling"
- );
- return;
- }
+ if let Some(last_scheduled_at) = state.last_scheduled_at {
+ let pending_delay =
+ THREAD_ATTACH_RETRY_DELAY.saturating_mul(u32::from(state.attempts.max(1)));
+ if now.saturating_duration_since(last_scheduled_at) < pending_delay {
+ tracing::debug!(
+ "attach retry already pending for thread {thread_id}; suppressing duplicate scheduling"
+ );
+ return;
+ }
+ }Also applies to: 3390-3397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/tui/src/app.rs` around lines 3373 - 3375, The duplicate-scheduling
guard currently compares now.saturating_duration_since(state.last_scheduled_at)
against a fixed THREAD_ATTACH_RETRY_DELAY, which allows later backoff retries
(500ms/750ms/...) to overlap; update both checks (the one using
state.last_scheduled_at around the shown block and the similar one at the other
occurrence) to compare against a dynamic delay derived from the thread's current
retry/backoff state instead of the fixed constant—e.g., compute a delay =
THREAD_ATTACH_RETRY_DELAY * (state.attach_attempts.saturating_add(1)) or use an
existing per-thread next_retry_delay field (state.next_retry_delay) and replace
the fixed THREAD_ATTACH_RETRY_DELAY in the condition with that computed/field
value so retries are de-duplicated according to the actual backoff schedule.
codex-rs/tui/src/chatwidget/agent.rs
Outdated
Comment on lines
+603
to
+605
| EventMsg::TurnComplete(_) | EventMsg::TurnAborted(_) => { | ||
| current_turn_id = None; | ||
| } |
There was a problem hiding this comment.
Clear pending server requests when a turn completes or aborts.
At Line 603 and Line 1727, current_turn_id is reset but pending_server_requests is not. If approvals are left pending when a turn ends, stale entries remain and can leak state across subsequent operations.
🔧 Proposed fix
-EventMsg::TurnComplete(_) | EventMsg::TurnAborted(_) => {
- current_turn_id = None;
-}
+EventMsg::TurnComplete(_) | EventMsg::TurnAborted(_) => {
+ current_turn_id = None;
+ pending_server_requests.clear();
+}Also applies to: 1727-1729
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/tui/src/chatwidget/agent.rs` around lines 603 - 605, When handling
EventMsg::TurnComplete(_) and EventMsg::TurnAborted(_) (the match arm that
currently sets current_turn_id = None), also clear the pending_server_requests
map so stale approval entries don't leak into subsequent turns; update the same
matching logic that touches current_turn_id (and the analogous spot near the
other TurnComplete/TurnAborted handler) to call pending_server_requests.clear()
or remove only entries associated with the finished turn id if you prefer more
targeted cleanup, ensuring both current_turn_id and pending_server_requests are
synchronized when a turn ends.
codex-rs/tui/src/chatwidget/agent.rs
Outdated
Comment on lines
+992
to
+1017
| let Some(request_id) = pending_server_requests.exec_approvals.remove(&id) else { | ||
| send_warning_event( | ||
| app_event_tx, | ||
| format!("exec approval ignored because request id `{id}` was not pending"), | ||
| ); | ||
| return; | ||
| }; | ||
| let response = CommandExecutionRequestApprovalResponse { | ||
| decision: CommandExecutionApprovalDecision::from(decision), | ||
| }; | ||
| let result = match serde_json::to_value(response) { | ||
| Ok(value) => value, | ||
| Err(err) => { | ||
| send_error_event( | ||
| app_event_tx, | ||
| format!("failed to encode exec approval: {err}"), | ||
| ); | ||
| return; | ||
| } | ||
| }; | ||
| if let Err(err) = client.resolve_server_request(request_id, result).await { | ||
| send_error_event( | ||
| app_event_tx, | ||
| format!("failed to submit exec approval: {err}"), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Don’t drop pending request IDs before resolve_server_request succeeds.
At Lines 992/1020/1063/1098/1139, the pending ID is removed before the async resolve call. If resolve fails, the mapping is lost and the user cannot retry the response submission.
🔧 Proposed fix pattern (apply to each response branch)
- let Some(request_id) = pending_server_requests.exec_approvals.remove(&id) else {
+ let Some(request_id) = pending_server_requests.exec_approvals.get(&id).cloned() else {
send_warning_event(
app_event_tx,
format!("exec approval ignored because request id `{id}` was not pending"),
);
return;
};
...
if let Err(err) = client.resolve_server_request(request_id, result).await {
send_error_event(
app_event_tx,
format!("failed to submit exec approval: {err}"),
);
+ return;
}
+pending_server_requests.exec_approvals.remove(&id);Also applies to: 1020-1052, 1063-1095, 1098-1135, 1139-1177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/tui/src/chatwidget/agent.rs` around lines 992 - 1017, The code
currently removes the pending ID from pending_server_requests.exec_approvals
before awaiting client.resolve_server_request, which loses the mapping if the
async call fails; instead, look up or clone the request_id without removing it
(e.g., use get(&id).cloned()), call client.resolve_server_request(request_id,
result).await, and only call pending_server_requests.exec_approvals.remove(&id)
after the resolve succeeds; keep the same send_warning_event/send_error_event
flows and ensure errors do not drop the pending entry so users can retry.
There was a problem hiding this comment.
Pull request overview
This PR embeds codex-app-server in-process for codex-tui and codex-exec, so both surfaces share the same app-server runtime/protocol mapping layer while avoiding JSON serialization on the in-process hot path (typed channels for requests/events).
Changes:
- Introduces
codex-app-server-clientas aSend-safe in-process embedding facade (worker task, backpressure/lag signaling, typed request helpers, graceful shutdown). - Refactors app-server outgoing messages to carry typed core events (
OutgoingMessage::CoreEvent) and serializes those to legacycodex/event/*JSON-RPC notifications only at transport boundaries. - Updates TUI codepaths/tests to route intent via
AgentCommand/AppEvent::AgentCommand/SubmitThreadAgentCommandinstead of constructing raw protocolOpdirectly; expands v2 protocol/schema (new RPCs + notifications including MCP elicitation).
Reviewed changes
Copilot reviewed 118 out of 119 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| codex-rs/tui/src/status_indicator_widget.rs | Switches interrupt dispatch to AgentCommand::Interrupt. |
| codex-rs/tui/src/lib.rs | Registers new agent_command module. |
| codex-rs/tui/src/chatwidget/tests.rs | Updates widget construction and app-event expectations for agent command flow. |
| codex-rs/tui/src/chatwidget.rs | Removes ThreadManager dependency in ctor path; exposes additional agent helpers; adds op_sender(). |
| codex-rs/tui/src/bottom_pane/mod.rs | Updates tests to assert interrupt is sent via AgentCommand. |
| codex-rs/tui/src/bottom_pane/approval_overlay.rs | Routes approval/elicitation responses via SubmitThreadAgentCommand + AgentCommand variants. |
| codex-rs/tui/src/app_event_sender.rs | Avoids replay double-logging for AgentCommand events (similar to CodexOp). |
| codex-rs/tui/src/app_event.rs | Adds AgentCommand + SubmitThreadAgentCommand + ThreadCreated app events. |
| codex-rs/tui/src/agent_command.rs | New intent-level command enum with lossless conversion into protocol Op. |
| codex-rs/tui/Cargo.toml | Adds dependency on codex-app-server-client. |
| codex-rs/exec/Cargo.toml | Adds dependencies on codex-app-server-client and codex-app-server-protocol. |
| codex-rs/core/src/lib.rs | Adds lookup_global_history_entry convenience wrapper for global history lookup. |
| codex-rs/app-server/src/transport.rs | Custom-serializes OutgoingMessage::CoreEvent to legacy JSON-RPC notifications; updates opt-out filtering. |
| codex-rs/app-server/src/thread_state.rs | Adds module docs and expands struct documentation for thread/subscription bookkeeping. |
| codex-rs/app-server/src/outgoing_message.rs | Introduces OutgoingMessage::CoreEvent and legacy_event_method(); renames sender helper accordingly. |
| codex-rs/app-server/src/message_processor.rs | Adds typed request/notification processing and makes session source configurable (but contains a compile-blocking move bug; see comments). |
| codex-rs/app-server/src/lib.rs | Exposes in_process module; threads session source through message processor args. |
| codex-rs/app-server/src/bespoke_event_handling.rs | Adds v2 notifications (warning/background/streamError/undo/shutdown/MCP startup/skills) + MCP elicitation request flow. |
| codex-rs/app-server/src/app_server_tracing.rs | Adds span builder for typed in-process requests (typed_request_span). |
| codex-rs/app-server/README.md | Documents in-process runtime model and adds RPC alias guidance + MCP elicitation docs. |
| codex-rs/app-server-protocol/src/protocol/v2.rs | Adds v2 request/response/notification types (undo/context/config/shutdown/memories/history/prompts/MCP tools + new notifications). |
| codex-rs/app-server-protocol/schema/typescript/v2/index.ts | Re-exports newly generated v2 TS types. |
| codex-rs/app-server-protocol/schema/typescript/v2/WarningNotification.ts | Generated TS type for warning notification. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadUndoStartedNotification.ts | Generated TS type for thread/undo/started notification. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadUndoResponse.ts | Generated TS type for thread/undo response. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadUndoParams.ts | Generated TS type for thread/undo params. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadUndoCompletedNotification.ts | Generated TS type for thread/undo/completed notification. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadTurnContextSetResponse.ts | Generated TS type for context update response. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadTurnContextSetParams.ts | Generated TS type for context update params. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadStartResponse.ts | Generated TS type updated with enriched bootstrap fields. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadShutdownResponse.ts | Generated TS type for thread/shutdown response. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadShutdownParams.ts | Generated TS type for thread/shutdown params. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadShutdownCompletedNotification.ts | Generated TS type for shutdown completion notification. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadShellCommandRunResponse.ts | Generated TS type for shell command response. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadShellCommandRunParams.ts | Generated TS type for shell command params. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadResumeResponse.ts | Generated TS type updated with enriched bootstrap fields. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadForkResponse.ts | Generated TS type updated with enriched bootstrap fields. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadContextSnapshot.ts | Generated TS type for thread context snapshot. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadConfigReloadResponse.ts | Generated TS type for config reload response. |
| codex-rs/app-server-protocol/schema/typescript/v2/ThreadConfigReloadParams.ts | Generated TS type for config reload params. |
| codex-rs/app-server-protocol/schema/typescript/v2/StreamErrorNotification.ts | Generated TS type for stream error notification. |
| codex-rs/app-server-protocol/schema/typescript/v2/SkillsUpdatedNotification.ts | Generated TS type for skills update notification. |
| codex-rs/app-server-protocol/schema/typescript/v2/SessionNetworkProxyRuntime.ts | Generated TS type for session proxy runtime bind addresses. |
| codex-rs/app-server-protocol/schema/typescript/v2/PromptCustomListResponse.ts | Generated TS type for custom prompt listing response. |
| codex-rs/app-server-protocol/schema/typescript/v2/PromptCustomListParams.ts | Generated TS type for custom prompt listing params. |
| codex-rs/app-server-protocol/schema/typescript/v2/MemoryUpdateResponse.ts | Generated TS type for memory update response. |
| codex-rs/app-server-protocol/schema/typescript/v2/MemoryUpdateParams.ts | Generated TS type for memory update params. |
| codex-rs/app-server-protocol/schema/typescript/v2/MemoryDropResponse.ts | Generated TS type for memory drop response. |
| codex-rs/app-server-protocol/schema/typescript/v2/MemoryDropParams.ts | Generated TS type for memory drop params. |
| codex-rs/app-server-protocol/schema/typescript/v2/McpToolsListResponse.ts | Generated TS type for MCP tools list response. |
| codex-rs/app-server-protocol/schema/typescript/v2/McpToolsListParams.ts | Generated TS type for MCP tools list params. |
| codex-rs/app-server-protocol/schema/typescript/v2/McpServerStartupUpdatedNotification.ts | Generated TS type for MCP startup updated notification. |
| codex-rs/app-server-protocol/schema/typescript/v2/McpServerStartupStatus.ts | Generated TS type for MCP startup status enum. |
| codex-rs/app-server-protocol/schema/typescript/v2/McpServerStartupFailure.ts | Generated TS type for MCP startup failure item. |
| codex-rs/app-server-protocol/schema/typescript/v2/McpServerStartupCompletedNotification.ts | Generated TS type for MCP startup completed notification. |
| codex-rs/app-server-protocol/schema/typescript/v2/McpElicitationRequestResponse.ts | Generated TS type for MCP elicitation response payload. |
| codex-rs/app-server-protocol/schema/typescript/v2/McpElicitationRequestParams.ts | Generated TS type for MCP elicitation request params. |
| codex-rs/app-server-protocol/schema/typescript/v2/McpElicitationDecision.ts | Generated TS union type for elicitation decision. |
| codex-rs/app-server-protocol/schema/typescript/v2/HistoryEntryReadResponse.ts | Generated TS type for history entry read response. |
| codex-rs/app-server-protocol/schema/typescript/v2/HistoryEntryReadParams.ts | Generated TS type for history entry read params. |
| codex-rs/app-server-protocol/schema/typescript/v2/HistoryEntry.ts | Generated TS type for history entry. |
| codex-rs/app-server-protocol/schema/typescript/v2/HistoryAddResponse.ts | Generated TS type for history append response. |
| codex-rs/app-server-protocol/schema/typescript/v2/HistoryAddParams.ts | Generated TS type for history append params. |
| codex-rs/app-server-protocol/schema/typescript/v2/BackgroundEventNotification.ts | Generated TS type for background event notification. |
| codex-rs/app-server-protocol/schema/typescript/ServerRequest.ts | Updates union to include MCP elicitation server request. |
| codex-rs/app-server-protocol/schema/typescript/ServerNotification.ts | Updates union to include new v2 notifications. |
| codex-rs/app-server-protocol/schema/typescript/ClientRequest.ts | Updates union to include new v2 RPCs + backward-compatible aliases. |
| codex-rs/app-server-protocol/schema/json/v2/WarningNotification.json | Generated JSON schema for warning notification. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadUndoStartedNotification.json | Generated JSON schema for undo started notification. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadUndoResponse.json | Generated JSON schema for undo response. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadUndoParams.json | Generated JSON schema for undo params. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadUndoCompletedNotification.json | Generated JSON schema for undo completed notification. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadTurnContextSetResponse.json | Generated JSON schema for context update response. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadTurnContextSetParams.json | Generated JSON schema for context update params. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json | Generated JSON schema updated with enriched bootstrap fields. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadShutdownResponse.json | Generated JSON schema for shutdown response. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadShutdownParams.json | Generated JSON schema for shutdown params. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadShutdownCompletedNotification.json | Generated JSON schema for shutdown completed notification. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadShellCommandRunResponse.json | Generated JSON schema for shell command response. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadShellCommandRunParams.json | Generated JSON schema for shell command params. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json | Generated JSON schema updated with enriched bootstrap fields. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json | Generated JSON schema updated with enriched bootstrap fields. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadConfigReloadResponse.json | Generated JSON schema for config reload response. |
| codex-rs/app-server-protocol/schema/json/v2/ThreadConfigReloadParams.json | Generated JSON schema for config reload params. |
| codex-rs/app-server-protocol/schema/json/v2/StreamErrorNotification.json | Generated JSON schema for stream error notification. |
| codex-rs/app-server-protocol/schema/json/v2/SkillsUpdatedNotification.json | Generated JSON schema for skills updated notification. |
| codex-rs/app-server-protocol/schema/json/v2/PromptCustomListResponse.json | Generated JSON schema for custom prompt list response. |
| codex-rs/app-server-protocol/schema/json/v2/PromptCustomListParams.json | Generated JSON schema for custom prompt list params. |
| codex-rs/app-server-protocol/schema/json/v2/MemoryUpdateResponse.json | Generated JSON schema for memory update response. |
| codex-rs/app-server-protocol/schema/json/v2/MemoryUpdateParams.json | Generated JSON schema for memory update params. |
| codex-rs/app-server-protocol/schema/json/v2/MemoryDropResponse.json | Generated JSON schema for memory drop response. |
| codex-rs/app-server-protocol/schema/json/v2/MemoryDropParams.json | Generated JSON schema for memory drop params. |
| codex-rs/app-server-protocol/schema/json/v2/McpToolsListResponse.json | Generated JSON schema for MCP tools list response. |
| codex-rs/app-server-protocol/schema/json/v2/McpToolsListParams.json | Generated JSON schema for MCP tools list params. |
| codex-rs/app-server-protocol/schema/json/v2/McpServerStartupUpdatedNotification.json | Generated JSON schema for MCP startup updated notification. |
| codex-rs/app-server-protocol/schema/json/v2/McpServerStartupCompletedNotification.json | Generated JSON schema for MCP startup completed notification. |
| codex-rs/app-server-protocol/schema/json/v2/HistoryEntryReadResponse.json | Generated JSON schema for history entry read response. |
| codex-rs/app-server-protocol/schema/json/v2/HistoryEntryReadParams.json | Generated JSON schema for history entry read params. |
| codex-rs/app-server-protocol/schema/json/v2/HistoryAddResponse.json | Generated JSON schema for history append response. |
| codex-rs/app-server-protocol/schema/json/v2/HistoryAddParams.json | Generated JSON schema for history append params. |
| codex-rs/app-server-protocol/schema/json/v2/BackgroundEventNotification.json | Generated JSON schema for background event notification. |
| codex-rs/app-server-protocol/schema/json/ServerRequest.json | Generated JSON schema updated for MCP elicitation server request. |
| codex-rs/app-server-protocol/schema/json/ServerNotification.json | Generated JSON schema updated for new server notifications. |
| codex-rs/app-server-protocol/schema/json/McpElicitationRequestResponse.json | Generated JSON schema for MCP elicitation response payload. |
| codex-rs/app-server-protocol/schema/json/McpElicitationRequestParams.json | Generated JSON schema for MCP elicitation request params. |
| codex-rs/app-server-protocol/schema/json/ClientRequest.json | Generated JSON schema updated for new v2 RPCs + aliases. |
| codex-rs/app-server-client/src/lib.rs | New in-process client facade crate with backpressure/lag/shutdown handling. |
| codex-rs/app-server-client/README.md | Documentation for new in-process client crate. |
| codex-rs/app-server-client/Cargo.toml | New crate manifest for codex-app-server-client. |
| codex-rs/README.md | Documents that codex exec and TUI run through app-server in-process. |
| codex-rs/Cargo.toml | Adds app-server-client to workspace members and dependencies. |
| codex-rs/Cargo.lock | Adds lockfile entries for new crate and updated deps. |
Comments suppressed due to low confidence (1)
codex-rs/app-server/src/message_processor.rs:389
match codex_request { ... }movescodex_request, but the function then callscodex_request.experimental_reason()and latermatch codex_request { ... }again. This won’t compile (use-after-move). Consider matching on&codex_requestfor the initialization/guard checks (or usingif matches!(&codex_request, ...)) so the request value remains available for later dispatch, or restructure into a single match that performs the guards and then delegates.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+294
to
+301
| request.id().clone(), | ||
| JSONRPCErrorError { | ||
| code: -32001, | ||
| message: "in-process app-server event queue is full".to_string(), | ||
| data: None, | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The overload error code is hard-coded as -32001 here. Since this value is already defined as OVERLOADED_ERROR_CODE in codex-app-server (and used by both transport and in-process runtimes), consider defining a named constant in this crate (e.g., const OVERLOADED_ERROR_CODE: i64 = -32001;) and using it in both places to avoid magic numbers and keep semantics aligned if the code ever changes.
fcoury
force-pushed
the
feat/tui-app-server
branch
from
d6b8906 to
ab1c02f
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
codex-rs/core/src/mcp_connection_manager.rs (1)
319-342:⚠️ Potential issue | 🟠 MajorHandle event-send failure before awaiting
rxto avoid stuck elicitation futures.If
tx_event.send(...)fails, the pending responder remains stored andrx.awaitcan hang indefinitely.Suggested fix
let (tx, rx) = oneshot::channel(); { let mut lock = elicitation_requests.lock().await; lock.insert((server_name.clone(), id.clone()), tx); } - let _ = tx_event - .send(Event { - id: "mcp_elicitation_request".to_string(), - msg: EventMsg::ElicitationRequest(ElicitationRequestEvent { - server_name, - id: match id.clone() { - rmcp::model::NumberOrString::String(value) => { - ProtocolRequestId::String(value.to_string()) - } - rmcp::model::NumberOrString::Number(value) => { - ProtocolRequestId::Integer(value) - } - }, - request, - }), - }) - .await; + if tx_event + .send(Event { + id: "mcp_elicitation_request".to_string(), + msg: EventMsg::ElicitationRequest(ElicitationRequestEvent { + server_name: server_name.clone(), + id: match id.clone() { + rmcp::model::NumberOrString::String(value) => { + ProtocolRequestId::String(value.to_string()) + } + rmcp::model::NumberOrString::Number(value) => { + ProtocolRequestId::Integer(value) + } + }, + request, + }), + }) + .await + .is_err() + { + let mut lock = elicitation_requests.lock().await; + lock.remove(&(server_name.clone(), id.clone())); + return Err(anyhow!("failed to emit MCP elicitation request")); + } rx.await .context("elicitation request channel closed unexpectedly")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/mcp_connection_manager.rs` around lines 319 - 342, The code inserts a responder into elicitation_requests then sends the Event via tx_event but doesn't handle the send() failure, which can leave the stored oneshot responder and cause rx.await to hang; change the flow in the block around oneshot::channel(), elicitation_requests, and tx_event.send(...) so that you first insert the (server_name.clone(), id.clone()) => tx into elicitation_requests, attempt tx_event.send(...).await and if send returns Err, acquire the elicitation_requests lock again and remove the previously inserted key (or otherwise take ownership of the tx) and return an Err immediately (propagating context) instead of awaiting rx; only on Ok from send should you await rx. Reference symbols: elicitation_requests, tx_event.send(Event { id: "mcp_elicitation_request", msg: EventMsg::ElicitationRequest(ElicitationRequestEvent { server_name, id: ProtocolRequestId::..., request }) }), oneshot::channel()'s tx/rx, and rx.await.codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json (1)
3247-3264:⚠️ Potential issue | 🟠 MajorBackward-compatibility risk in
elicitation_requestevent shape.Line 3263 now makes
requestmandatory, which rejects legacy payloads that only providemessage. That introduces a wire-shape break during migration.Proposed compatibility-safe schema update
{ "properties": { "id": { "$ref": "#/definitions/v2/RequestId" }, + "message": { + "type": "string" + }, "request": { "$ref": "#/definitions/ElicitationRequest" }, "server_name": { "type": "string" }, "type": { "enum": [ "elicitation_request" ], "title": "ElicitationRequestEventMsgType", "type": "string" } }, + "anyOf": [ + { "required": ["message"] }, + { "required": ["request"] } + ], "required": [ "id", - "request", "server_name", "type" ], "title": "ElicitationRequestEventMsg", "type": "object" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json` around lines 3247 - 3264, The schema change made "request" required on the elicitation_request event and breaks legacy payloads that only include "message"; revert this compatibility break by removing "request" from the required array for the elicitation_request event (the object whose "type" enum contains "elicitation_request" and that references ElicitationRequest) or alternatively make validation accept either property via oneOf; ensure the properties include both "request" and "message" and that only "id" and "server_name" (or the original required set) remain mandatory so payloads with only "message" still validate.codex-rs/app-server-protocol/schema/json/EventMsg.json (1)
2016-2033:⚠️ Potential issue | 🟠 MajorPreserve backward compatibility for legacy
message-based elicitation payloads.Line 2032 and Line 7825 now require
requestand removemessage, which is a wire-shape breaking change for older clients. Keep support for both fields during migration.Suggested compatibility schema shape
"properties": { "id": { "$ref": "#/definitions/RequestId" }, + "message": { + "type": "string" + }, "request": { "$ref": "#/definitions/ElicitationRequest" }, "server_name": { "type": "string" }, "type": { "enum": [ "elicitation_request" ], "title": "ElicitationRequestEventMsgType", "type": "string" } }, - "required": [ - "id", - "request", - "server_name", - "type" - ], + "required": [ + "id", + "server_name", + "type" + ], + "anyOf": [ + { "required": ["request"] }, + { "required": ["message"] } + ],Also applies to: 7809-7826
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/EventMsg.json` around lines 2016 - 2033, The schema change made the elicitation_request event wire-incompatible by requiring "request" and removing legacy "message"; update the EventMsg.json definition for the object with "type": "elicitation_request" (the ElicitationRequestEventMsg shape) to accept both payload shapes during migration: make both "request" and "message" optional in properties and replace the current "required" list that forces "request" with an anyOf/oneOf constraint that enforces at least one of ["request","message"] is present (or use a discriminator-free oneOf that accepts either the new {required:["request"]} or legacy {required:["message"]} variant), keeping "id" and "server_name" required as before.codex-rs/core/src/plugins/manager.rs (1)
517-525:⚠️ Potential issue | 🟡 MinorNormalize app connector IDs before storing them.
You validate with
trim()but persist the untrimmed string, which can leave invisible whitespace bugs in connector IDs.🔧 Proposed normalization fix
.filter_map(|app| { - if app.id.trim().is_empty() { + let id = app.id.trim(); + if id.is_empty() { warn!( plugin = %plugin_root.display(), "plugin app config is missing an app id" ); None } else { - Some(AppConnectorId(app.id)) + Some(AppConnectorId(id.to_string())) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/plugins/manager.rs` around lines 517 - 525, The code validates app.id with app.id.trim().is_empty() but stores the original untrimmed string; change the storage to a normalized, trimmed value so invisible whitespace can't persist: in the branch that returns Some(AppConnectorId(app.id)), replace it with creating AppConnectorId from the trimmed string (e.g., app.id.trim().to_string()) so the saved connector id is normalized; update the construction in the same block (where AppConnectorId is created) to use the trimmed value.codex-rs/artifacts/src/client.rs (1)
65-81:⚠️ Potential issue | 🟠 MajorPrevent caller env from overriding reserved runtime env keys.
At lines 80-82 and 105-107,
request.envis applied after internal env setup. Since Rust'sCommand::env()uses last-write-wins semantics, duplicate keys allow callers to overwriteCODEX_ARTIFACT_BUILD_ENTRYPOINT,CODEX_ARTIFACT_RENDER_ENTRYPOINT, andELECTRON_RUN_AS_NODE, potentially bypassing runtime configuration controls.Apply
request.envbefore setting reserved vars to ensure internal env vars take precedence:🔐 Suggested fix
- command - .arg(&script_path) - .current_dir(&request.cwd) - .env("CODEX_ARTIFACT_BUILD_ENTRYPOINT", runtime.build_js_path()) - .env( - "CODEX_ARTIFACT_RENDER_ENTRYPOINT", - runtime.render_cli_path(), - ) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()); - if js_runtime.requires_electron_run_as_node() { - command.env("ELECTRON_RUN_AS_NODE", "1"); - } for (key, value) in &request.env { command.env(key, value); } + command + .arg(&script_path) + .current_dir(&request.cwd) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .env("CODEX_ARTIFACT_BUILD_ENTRYPOINT", runtime.build_js_path()) + .env( + "CODEX_ARTIFACT_RENDER_ENTRYPOINT", + runtime.render_cli_path(), + ); + if js_runtime.requires_electron_run_as_node() { + command.env("ELECTRON_RUN_AS_NODE", "1"); + }Also applies to: execute_render (lines 105-107)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/artifacts/src/client.rs` around lines 65 - 81, The request.env map is applied after internal env vars so callers can overwrite reserved keys; reorder or guard this in both execute_build and execute_render: apply the loop over request.env (referencing request.env and the for (key, value) in &request.env loop) before setting CODEX_ARTIFACT_BUILD_ENTRYPOINT (runtime.build_js_path()), CODEX_ARTIFACT_RENDER_ENTRYPOINT (runtime.render_cli_path()), and ELECTRON_RUN_AS_NODE (js_runtime.requires_electron_run_as_node()), or alternatively skip any keys matching those reserved names when iterating request.env so the Command built from Command::new(js_runtime.executable_path()) cannot be influenced by caller-supplied overrides.
♻️ Duplicate comments (3)
codex-rs/app-server/src/in_process.rs (1)
232-239:⚠️ Potential issue | 🟠 MajorBound shutdown enqueue wait to avoid indefinite hangs.
self.client_tx.send(...).awaitcan block forever when the queue is full, preventing the existing timeout/abort cleanup from running.🔧 Suggested fix
- if self - .client_tx - .send(InProcessClientMessage::Shutdown { done_tx }) - .await - .is_ok() - { + let shutdown_enqueued = timeout( + SHUTDOWN_TIMEOUT, + self.client_tx.send(InProcessClientMessage::Shutdown { done_tx }), + ) + .await + .is_ok_and(|send_result| send_result.is_ok()); + if shutdown_enqueued { let _ = timeout(SHUTDOWN_TIMEOUT, done_rx).await; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/in_process.rs` around lines 232 - 239, The send to self.client_tx can await forever when the channel is full, so wrap or replace the blocking send used for InProcessClientMessage::Shutdown { done_tx } with a bounded non-blocking or timed send and fall back to the existing shutdown timeout: either use a try_send on client_tx and, on failure, log/handle the backpressure and still run the timeout(SHUTDOWN_TIMEOUT, done_rx).await, or wrap the async send in timeout(SHUTDOWN_TIMEOUT, self.client_tx.send(...)).await and handle the Err (timeout) case by proceeding to wait on done_rx with the same SHUTDOWN_TIMEOUT; ensure you reference client_tx, InProcessClientMessage::Shutdown, done_tx/done_rx, and SHUTDOWN_TIMEOUT when applying the change.codex-rs/app-server-client/src/lib.rs (1)
521-544:⚠️ Potential issue | 🟠 MajorRun worker cleanup unconditionally before returning shutdown errors.
At Line 531–Line 537, the
??can return early and bypass the cleanup at Line 539–Line 542, leaving the worker task alive.🔧 Suggested fix
pub async fn shutdown(self) -> IoResult<()> { let mut worker_handle = self.worker_handle; let (response_tx, response_rx) = oneshot::channel(); + let mut shutdown_err: Option<IoError> = None; if self .command_tx .send(ClientCommand::Shutdown { response_tx }) .await .is_ok() && let Ok(command_result) = timeout(SHUTDOWN_TIMEOUT, response_rx).await { - command_result.map_err(|_| { - IoError::new( - ErrorKind::BrokenPipe, - "in-process app-server shutdown channel is closed", - ) - })??; + if let Err(err) = command_result + .map_err(|_| { + IoError::new( + ErrorKind::BrokenPipe, + "in-process app-server shutdown channel is closed", + ) + }) + .and_then(|inner| inner) + { + shutdown_err = Some(err); + } } if let Err(_elapsed) = timeout(SHUTDOWN_TIMEOUT, &mut worker_handle).await { worker_handle.abort(); let _ = worker_handle.await; } + if let Some(err) = shutdown_err { + return Err(err); + } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-client/src/lib.rs` around lines 521 - 544, The shutdown method currently may return early from the command_result??; path and skip stopping the worker; change the logic in shutdown (function shutdown, variables worker_handle, command_tx, response_rx, timeout and SHUTDOWN_TIMEOUT) so that you never return before running the worker cleanup: first attempt the send and await the response into a local Result (e.g. capture the result of timeout(...) and of command_result.map_err(...) into a variable), do not use the `??` early-return operator inline, then after that handling always run the worker cleanup block (timeout(..., &mut worker_handle), worker_handle.abort(), worker_handle.await) and finally return any stored error (if the command_result was Err) or Ok(()) — in short, convert the inline `??` early return into capturing the error and returning it only after the worker cleanup has executed.codex-rs/app-server/src/message_processor.rs (1)
327-338:⚠️ Potential issue | 🟠 MajorAvoid full-payload logging in protocol handlers.
These handlers still emit full objects (
notification, typed notification, response, error). They can carry sensitive data; log only safe metadata.🔒 Redaction-safe logging change
pub(crate) async fn process_notification(&self, notification: JSONRPCNotification) { - tracing::info!("<- notification: {:?}", notification); + tracing::debug!(method = %notification.method, "<- notification received"); } pub(crate) async fn process_client_notification(&self, notification: ClientNotification) { - tracing::info!("<- typed notification: {:?}", notification); + let method = serde_json::to_value(¬ification) + .ok() + .and_then(|v| v.get("method").and_then(serde_json::Value::as_str).map(ToOwned::to_owned)) + .unwrap_or_else(|| "<unknown>".to_string()); + tracing::debug!(method = %method, "<- typed notification received"); } pub(crate) async fn process_response(&mut self, response: JSONRPCResponse) { - tracing::info!("<- response: {:?}", response); let JSONRPCResponse { id, result, .. } = response; + tracing::debug!(response_id = ?id, "<- response received"); self.outgoing.notify_client_response(id, result).await } pub(crate) async fn process_error(&mut self, err: JSONRPCError) { - tracing::error!("<- error: {:?}", err); - self.outgoing.notify_client_error(err.id, err.error).await; + let JSONRPCError { id, error, .. } = err; + tracing::warn!(response_id = ?id, code = error.code, "<- error received"); + self.outgoing.notify_client_error(id, error).await; }Also applies to: 373-383
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/message_processor.rs` around lines 327 - 338, The handlers process_notification and process_client_notification currently log entire payloads (notification, typed notification); change them to log only non-sensitive metadata (e.g., method name, notification.kind/type, optional id or source) instead of printing the whole object — replace tracing::info!("<- notification: {:?}", notification) and tracing::info!("<- typed notification: {:?}", notification) with structured logs that extract and log safe fields (method, kind/type, id/source, timestamp) and avoid Debug-printing the full payload; apply the same redaction-safe change to the similar logging at the other locations mentioned (the block around lines 373-383) so responses/errors are also logged only with safe metadata.
🧹 Nitpick comments (8)
codex-rs/app-server/src/codex_message_processor.rs (1)
7270-7305: Add one more regression case forservice_tier: Some(None).The current test covers
Some(Fast)vsSome(Flex). A second case for explicit clear (Some(None)) would lock behavior for the “clear tier while running” path too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/codex_message_processor.rs` around lines 7270 - 7305, Add a second regression assertion to the test around collect_resume_override_mismatches to cover the explicit "clear tier" case: construct a ThreadResumeParams with service_tier: Some(None) and a ThreadConfigSnapshot with service_tier: Some(Some(...)) (e.g., Flex) and assert that collect_resume_override_mismatches returns the expected mismatch string (e.g., "service_tier requested=None active=Some(Flex)" or matching the existing message format). Use the same test scaffolding (ThreadResumeParams, ThreadConfigSnapshot, collect_resume_override_mismatches) so the new case validates behavior when a running thread requests clearing the tier.codex-rs/core/src/plugins/render.rs (1)
38-41: Add a non-empty rendering regression test.Please add one test that validates list formatting for at least one plugin entry (including escaping behavior).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/plugins/render.rs` around lines 38 - 41, Add a new unit test that calls render_plugins_section with a non-empty slice containing at least one Plugin (use the same struct/constructor used elsewhere) and asserts the returned Option is Some(String) matching the expected markdown/list formatting and escaping; specifically create a plugin entry with characters that require escaping (e.g., parentheses, brackets, or backticks) and check that render_plugins_section produces the correct escaped list item string, e.g., assert_eq!(render_plugins_section(&[my_plugin]), Some(expected_string)); name the test something like render_plugins_section_formats_and_escapes_plugin_entries to make its purpose clear and reference render_plugins_section and the Plugin value used to build the test input.codex-rs/core/src/project_doc.rs (1)
116-121: Add a dedicated plugin-section regression test.Please add at least one assertion covering plugin section rendering (including placement relative to JS REPL/skills blocks and blank-line separators).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/project_doc.rs` around lines 116 - 121, Add a regression test that verifies plugin section rendering: create a test that constructs a ProjectDoc (or the same input used elsewhere) with plugins present so render_plugins_section produces output, then assert the produced document string (the result of calling the function that builds output using the plugins.and_then(render_plugins_section) branch) contains the plugin_section content and is placed correctly relative to JS REPL/skills blocks with the correct blank-line separators; specifically, exercise the code path that sets plugin_section via plugins.and_then(render_plugins_section), check that when output already contains JS REPL/skills blocks it inserts "\n\n" before plugin_section, and add at least one assertion for the case where output is empty (no preceding blocks) to ensure no extra blank lines are added.codex-rs/core/src/sandboxing/mod.rs (1)
420-426: Deduplicate the two execution wrappers.
execute_envandexecute_exec_request_with_after_spawnduplicate the same policy extraction/delegation logic. Consider funnelingexecute_envthrough the newer function to keep one code path.♻️ Proposed refactor
pub async fn execute_env( exec_request: ExecRequest, stdout_stream: Option<StdoutStream>, ) -> crate::error::Result<ExecToolCallOutput> { - let effective_policy = exec_request.sandbox_policy.clone(); - execute_exec_request(exec_request, &effective_policy, stdout_stream, None).await + execute_exec_request_with_after_spawn(exec_request, stdout_stream, None).await }Also applies to: 428-435
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/sandboxing/mod.rs` around lines 420 - 426, The two wrappers execute_env and execute_exec_request_with_after_spawn duplicate policy extraction/delegation; refactor execute_env to call the newer execute_exec_request_with_after_spawn to funnel through one code path: in execute_env obtain the effective_policy as currently done, then call execute_exec_request_with_after_spawn(exec_request, &effective_policy, stdout_stream, None).await (i.e., pass None for the after_spawn callback) and remove the duplicated delegation logic so execute_exec_request_with_after_spawn + execute_exec_request remain the single implementation.codex-rs/core/src/exec.rs (1)
234-252: Avoid dual source-of-truth for sandbox policy.
execute_exec_requesttakessandbox_policyas an argument while also receivingExecRequest.sandbox_policy(ignored at Line 249). This is easy to drift over time; either use one source or assert equality defensively.🔧 Defensive consistency check
pub(crate) async fn execute_exec_request( exec_request: ExecRequest, sandbox_policy: &SandboxPolicy, stdout_stream: Option<StdoutStream>, after_spawn: Option<Box<dyn FnOnce() + Send>>, ) -> Result<ExecToolCallOutput> { + debug_assert_eq!( + &exec_request.sandbox_policy, + sandbox_policy, + "mismatched sandbox policy in execute_exec_request" + ); + let ExecRequest { command, cwd, env,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/exec.rs` around lines 234 - 252, The function execute_exec_request currently takes sandbox_policy as a parameter but also destructures ExecRequest.sandbox_policy into _sandbox_policy_from_env and ignores it; replace this dual source-of-truth by either (1) using the ExecRequest.sandbox_policy consistently or (2) verifying they match and returning an error if they don't—i.e., in execute_exec_request compare the incoming sandbox_policy parameter with ExecRequest.sandbox_policy (the symbol _sandbox_policy_from_env) and bail with a clear error (or assert) on mismatch, or remove the ExecRequest.sandbox_policy field usage entirely and always use the sandbox_policy parameter to avoid drift.codex-rs/artifacts/src/runtime/installed.rs (1)
78-83: Validate extracted manifestschema_versionexplicitly.
After parsing on Line 78, the loader should fail fast on unsupported schema versions instead of relying on downstream behavior.♻️ Suggested guard (plus small error-surface extension)
--- a/codex-rs/artifacts/src/runtime/error.rs +++ b/codex-rs/artifacts/src/runtime/error.rs @@ #[error("invalid manifest at {path}")] InvalidManifest { path: PathBuf, #[source] source: serde_json::Error, }, + #[error("unsupported manifest schema version {found} at {path} (expected {expected})")] + UnsupportedManifestSchema { + path: PathBuf, + found: u32, + expected: u32, + },--- a/codex-rs/artifacts/src/runtime/installed.rs +++ b/codex-rs/artifacts/src/runtime/installed.rs @@ +const EXTRACTED_RUNTIME_SCHEMA_VERSION: u32 = 1; @@ let manifest = serde_json::from_slice::<ExtractedRuntimeManifest>(&manifest_bytes) .map_err(|source| ArtifactRuntimeError::InvalidManifest { path: manifest_path, source, })?; + if manifest.schema_version != EXTRACTED_RUNTIME_SCHEMA_VERSION { + return Err(ArtifactRuntimeError::UnsupportedManifestSchema { + path: root_dir.join("manifest.json"), + found: manifest.schema_version, + expected: EXTRACTED_RUNTIME_SCHEMA_VERSION, + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/artifacts/src/runtime/installed.rs` around lines 78 - 83, After deserializing the ExtractedRuntimeManifest in the block that sets manifest (serde_json::from_slice), add an explicit check of manifest.schema_version and fail fast for any unsupported versions before calling resolve_relative_runtime_path; return a clear ArtifactRuntimeError variant (e.g., ArtifactRuntimeError::UnsupportedSchemaVersion or extend ArtifactRuntimeError::InvalidManifest) that includes the manifest_path and the found schema_version so callers get immediate feedback about unsupported schema versions rather than relying on downstream failures.codex-rs/core/src/tools/handlers/artifacts.rs (1)
340-343: Prefer the exported base-URL constant in this assertion.
This test currently duplicates the release URL string literal; using the crate constant avoids drift if the default ever changes.♻️ Suggested tweak
- assert_eq!( - manager.config().release().base_url().as_str(), - "https://github.com/openai/codex/releases/download/" - ); + assert_eq!( + manager.config().release().base_url().as_str(), + codex_artifacts::DEFAULT_RELEASE_BASE_URL + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/tools/handlers/artifacts.rs` around lines 340 - 343, Replace the hard-coded URL string in the assertion that calls manager.config().release().base_url().as_str() with the crate's exported release base-URL constant (use the constant exported by the release/config module, e.g., RELEASE_BASE_URL or the crate's equivalent name) so the test compares against that constant instead of duplicating the literal.codex-rs/artifacts/src/runtime/manager.rs (1)
68-77: Consider documenting base URL format requirement.
Url::joinwith a path like"tag/file"behaves differently depending on whetherbase_urlends with a slash. The default URL includes a trailing slash, but if users provide a custom base URL without one, the join may produce unexpected results.Consider either:
- Documenting that
base_urlmust end with a slash, or- Ensuring the trailing slash programmatically in
new().♻️ Optional: Ensure trailing slash in base URL
pub fn new(base_url: Url, runtime_version: impl Into<String>) -> Self { + let base_url = if base_url.as_str().ends_with('/') { + base_url + } else { + Url::parse(&format!("{}/", base_url)).unwrap_or(base_url) + }; Self { base_url, runtime_version: runtime_version.into(), release_tag_prefix: DEFAULT_RELEASE_TAG_PREFIX.to_string(), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/artifacts/src/runtime/manager.rs` around lines 68 - 77, manifest_url uses Url::join with a path like "{tag}/{file}" which breaks if base_url lacks a trailing slash; update the constructor (new()) that creates/accepts self.base_url to normalize the URL by ensuring it ends with a '/' (or otherwise canonicalize it) before storing so manifest_url.join(...) always behaves predictably; reference the manifest_url method and the release_tag()/manifest_file_name() helpers when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@codex-rs/app-server-protocol/schema/json/McpServerElicitationRequestParams.json`:
- Around line 66-69: The JSON schema for McpServerElicitationRequestParams is
missing "turnId" in its "required" array while the generated TypeScript type
McpServerElicitationRequestParams treats turnId as mandatory (string | null);
update the JSON schema by adding "turnId" to the "required" list and ensure the
turnId property schema allows null (e.g., type: ["string","null"] or equivalent)
so the schema and the generated TS type are aligned.
In
`@codex-rs/app-server-protocol/schema/json/McpServerElicitationRequestResponse.json`:
- Around line 17-23: The schema's "content" property is currently missing a type
and not listed in "required", causing a mismatch with the generated TypeScript
which expects content: JsonValue | null; update the
McpServerElicitationRequestResponse JSON Schema by adding "content" to the
"required" array and define "content" to accept any JSON value (i.e., the JSON
Schema equivalent of JsonValue and nullable) so it aligns with the TS contract
(keep descriptive text in "description" and preserve nullability).
In `@codex-rs/app-server/src/app_server_tracing.rs`:
- Around line 86-93: The span currently hardcodes "in_process" into otel.name
and rpc.method (in the info_span! call), which collapses all in-process RPCs
into one bucket; implement a helper fn typed_request_method(request:
&ClientRequest) -> &'static str (mirror client_request_id's style) that
pattern-matches on ClientRequest variants and returns real, static RPC method
names, then replace the hardcoded "in_process" values in the info_span!
invocation (and any similar usages) with typed_request_method(request) so
otel.name and rpc.method carry the actual method name.
In `@codex-rs/app-server/src/lib.rs`:
- Line 589: The processor is being initialized with a hard-coded
SessionSource::VSCode; change run_main_with_transport to accept a SessionSource
parameter (thread session_source through its signature and callers) and use that
parameter when constructing the processor instead of SessionSource::VSCode;
follow the pattern in the in_process module for accepting and forwarding
session_source, and ensure callers (e.g., the CLI invocation) pass
SessionSource::Cli where appropriate so telemetry/routing use the correct
source.
In `@codex-rs/core/src/exec.rs`:
- Around line 743-745: The after_spawn() callback should be invoked inside
std::panic::catch_unwind to prevent its panic from skipping subsequent cleanup
(like consume_truncated_output) — wrap the call to after_spawn (the
Some(after_spawn) branch) with catch_unwind, record whether it returned Ok or
Err, then proceed to call consume_truncated_output and other termination/timeout
handling as normal, and finally if catch_unwind returned Err re-raise the panic
using std::panic::resume_unwind; refer to the after_spawn variable, the
consume_truncated_output call, and ensure panic handling does not alter existing
kill_on_drop behavior.
In `@codex-rs/core/src/plugins/render.rs`:
- Around line 14-18: The current rendering inserts plugin.display_name raw into
inline backticked markdown (in the lines.extend mapping in render.rs), which
allows backticks/newlines to break formatting; fix it by sanitizing/escaping the
display name before interpolation—add or use a helper like
escape_markdown_inline or sanitize_display_name that replaces backticks with
'\`' and collapses or removes newlines (or encodes them) and then use that
sanitized value in the format!("- `{}`", ...). Update the mapping over plugins
and any related helpers to ensure all plugin.display_name usages in markdown go
through this sanitizer.
---
Outside diff comments:
In
`@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json`:
- Around line 3247-3264: The schema change made "request" required on the
elicitation_request event and breaks legacy payloads that only include
"message"; revert this compatibility break by removing "request" from the
required array for the elicitation_request event (the object whose "type" enum
contains "elicitation_request" and that references ElicitationRequest) or
alternatively make validation accept either property via oneOf; ensure the
properties include both "request" and "message" and that only "id" and
"server_name" (or the original required set) remain mandatory so payloads with
only "message" still validate.
In `@codex-rs/app-server-protocol/schema/json/EventMsg.json`:
- Around line 2016-2033: The schema change made the elicitation_request event
wire-incompatible by requiring "request" and removing legacy "message"; update
the EventMsg.json definition for the object with "type": "elicitation_request"
(the ElicitationRequestEventMsg shape) to accept both payload shapes during
migration: make both "request" and "message" optional in properties and replace
the current "required" list that forces "request" with an anyOf/oneOf constraint
that enforces at least one of ["request","message"] is present (or use a
discriminator-free oneOf that accepts either the new {required:["request"]} or
legacy {required:["message"]} variant), keeping "id" and "server_name" required
as before.
In `@codex-rs/artifacts/src/client.rs`:
- Around line 65-81: The request.env map is applied after internal env vars so
callers can overwrite reserved keys; reorder or guard this in both execute_build
and execute_render: apply the loop over request.env (referencing request.env and
the for (key, value) in &request.env loop) before setting
CODEX_ARTIFACT_BUILD_ENTRYPOINT (runtime.build_js_path()),
CODEX_ARTIFACT_RENDER_ENTRYPOINT (runtime.render_cli_path()), and
ELECTRON_RUN_AS_NODE (js_runtime.requires_electron_run_as_node()), or
alternatively skip any keys matching those reserved names when iterating
request.env so the Command built from Command::new(js_runtime.executable_path())
cannot be influenced by caller-supplied overrides.
In `@codex-rs/core/src/mcp_connection_manager.rs`:
- Around line 319-342: The code inserts a responder into elicitation_requests
then sends the Event via tx_event but doesn't handle the send() failure, which
can leave the stored oneshot responder and cause rx.await to hang; change the
flow in the block around oneshot::channel(), elicitation_requests, and
tx_event.send(...) so that you first insert the (server_name.clone(),
id.clone()) => tx into elicitation_requests, attempt tx_event.send(...).await
and if send returns Err, acquire the elicitation_requests lock again and remove
the previously inserted key (or otherwise take ownership of the tx) and return
an Err immediately (propagating context) instead of awaiting rx; only on Ok from
send should you await rx. Reference symbols: elicitation_requests,
tx_event.send(Event { id: "mcp_elicitation_request", msg:
EventMsg::ElicitationRequest(ElicitationRequestEvent { server_name, id:
ProtocolRequestId::..., request }) }), oneshot::channel()'s tx/rx, and rx.await.
In `@codex-rs/core/src/plugins/manager.rs`:
- Around line 517-525: The code validates app.id with app.id.trim().is_empty()
but stores the original untrimmed string; change the storage to a normalized,
trimmed value so invisible whitespace can't persist: in the branch that returns
Some(AppConnectorId(app.id)), replace it with creating AppConnectorId from the
trimmed string (e.g., app.id.trim().to_string()) so the saved connector id is
normalized; update the construction in the same block (where AppConnectorId is
created) to use the trimmed value.
---
Duplicate comments:
In `@codex-rs/app-server-client/src/lib.rs`:
- Around line 521-544: The shutdown method currently may return early from the
command_result??; path and skip stopping the worker; change the logic in
shutdown (function shutdown, variables worker_handle, command_tx, response_rx,
timeout and SHUTDOWN_TIMEOUT) so that you never return before running the worker
cleanup: first attempt the send and await the response into a local Result (e.g.
capture the result of timeout(...) and of command_result.map_err(...) into a
variable), do not use the `??` early-return operator inline, then after that
handling always run the worker cleanup block (timeout(..., &mut worker_handle),
worker_handle.abort(), worker_handle.await) and finally return any stored error
(if the command_result was Err) or Ok(()) — in short, convert the inline `??`
early return into capturing the error and returning it only after the worker
cleanup has executed.
In `@codex-rs/app-server/src/in_process.rs`:
- Around line 232-239: The send to self.client_tx can await forever when the
channel is full, so wrap or replace the blocking send used for
InProcessClientMessage::Shutdown { done_tx } with a bounded non-blocking or
timed send and fall back to the existing shutdown timeout: either use a try_send
on client_tx and, on failure, log/handle the backpressure and still run the
timeout(SHUTDOWN_TIMEOUT, done_rx).await, or wrap the async send in
timeout(SHUTDOWN_TIMEOUT, self.client_tx.send(...)).await and handle the Err
(timeout) case by proceeding to wait on done_rx with the same SHUTDOWN_TIMEOUT;
ensure you reference client_tx, InProcessClientMessage::Shutdown,
done_tx/done_rx, and SHUTDOWN_TIMEOUT when applying the change.
In `@codex-rs/app-server/src/message_processor.rs`:
- Around line 327-338: The handlers process_notification and
process_client_notification currently log entire payloads (notification, typed
notification); change them to log only non-sensitive metadata (e.g., method
name, notification.kind/type, optional id or source) instead of printing the
whole object — replace tracing::info!("<- notification: {:?}", notification) and
tracing::info!("<- typed notification: {:?}", notification) with structured logs
that extract and log safe fields (method, kind/type, id/source, timestamp) and
avoid Debug-printing the full payload; apply the same redaction-safe change to
the similar logging at the other locations mentioned (the block around lines
373-383) so responses/errors are also logged only with safe metadata.
---
Nitpick comments:
In `@codex-rs/app-server/src/codex_message_processor.rs`:
- Around line 7270-7305: Add a second regression assertion to the test around
collect_resume_override_mismatches to cover the explicit "clear tier" case:
construct a ThreadResumeParams with service_tier: Some(None) and a
ThreadConfigSnapshot with service_tier: Some(Some(...)) (e.g., Flex) and assert
that collect_resume_override_mismatches returns the expected mismatch string
(e.g., "service_tier requested=None active=Some(Flex)" or matching the existing
message format). Use the same test scaffolding (ThreadResumeParams,
ThreadConfigSnapshot, collect_resume_override_mismatches) so the new case
validates behavior when a running thread requests clearing the tier.
In `@codex-rs/artifacts/src/runtime/installed.rs`:
- Around line 78-83: After deserializing the ExtractedRuntimeManifest in the
block that sets manifest (serde_json::from_slice), add an explicit check of
manifest.schema_version and fail fast for any unsupported versions before
calling resolve_relative_runtime_path; return a clear ArtifactRuntimeError
variant (e.g., ArtifactRuntimeError::UnsupportedSchemaVersion or extend
ArtifactRuntimeError::InvalidManifest) that includes the manifest_path and the
found schema_version so callers get immediate feedback about unsupported schema
versions rather than relying on downstream failures.
In `@codex-rs/artifacts/src/runtime/manager.rs`:
- Around line 68-77: manifest_url uses Url::join with a path like "{tag}/{file}"
which breaks if base_url lacks a trailing slash; update the constructor (new())
that creates/accepts self.base_url to normalize the URL by ensuring it ends with
a '/' (or otherwise canonicalize it) before storing so manifest_url.join(...)
always behaves predictably; reference the manifest_url method and the
release_tag()/manifest_file_name() helpers when making the change.
In `@codex-rs/core/src/exec.rs`:
- Around line 234-252: The function execute_exec_request currently takes
sandbox_policy as a parameter but also destructures ExecRequest.sandbox_policy
into _sandbox_policy_from_env and ignores it; replace this dual source-of-truth
by either (1) using the ExecRequest.sandbox_policy consistently or (2) verifying
they match and returning an error if they don't—i.e., in execute_exec_request
compare the incoming sandbox_policy parameter with ExecRequest.sandbox_policy
(the symbol _sandbox_policy_from_env) and bail with a clear error (or assert) on
mismatch, or remove the ExecRequest.sandbox_policy field usage entirely and
always use the sandbox_policy parameter to avoid drift.
In `@codex-rs/core/src/plugins/render.rs`:
- Around line 38-41: Add a new unit test that calls render_plugins_section with
a non-empty slice containing at least one Plugin (use the same
struct/constructor used elsewhere) and asserts the returned Option is
Some(String) matching the expected markdown/list formatting and escaping;
specifically create a plugin entry with characters that require escaping (e.g.,
parentheses, brackets, or backticks) and check that render_plugins_section
produces the correct escaped list item string, e.g.,
assert_eq!(render_plugins_section(&[my_plugin]), Some(expected_string)); name
the test something like
render_plugins_section_formats_and_escapes_plugin_entries to make its purpose
clear and reference render_plugins_section and the Plugin value used to build
the test input.
In `@codex-rs/core/src/project_doc.rs`:
- Around line 116-121: Add a regression test that verifies plugin section
rendering: create a test that constructs a ProjectDoc (or the same input used
elsewhere) with plugins present so render_plugins_section produces output, then
assert the produced document string (the result of calling the function that
builds output using the plugins.and_then(render_plugins_section) branch)
contains the plugin_section content and is placed correctly relative to JS
REPL/skills blocks with the correct blank-line separators; specifically,
exercise the code path that sets plugin_section via
plugins.and_then(render_plugins_section), check that when output already
contains JS REPL/skills blocks it inserts "\n\n" before plugin_section, and add
at least one assertion for the case where output is empty (no preceding blocks)
to ensure no extra blank lines are added.
In `@codex-rs/core/src/sandboxing/mod.rs`:
- Around line 420-426: The two wrappers execute_env and
execute_exec_request_with_after_spawn duplicate policy extraction/delegation;
refactor execute_env to call the newer execute_exec_request_with_after_spawn to
funnel through one code path: in execute_env obtain the effective_policy as
currently done, then call execute_exec_request_with_after_spawn(exec_request,
&effective_policy, stdout_stream, None).await (i.e., pass None for the
after_spawn callback) and remove the duplicated delegation logic so
execute_exec_request_with_after_spawn + execute_exec_request remain the single
implementation.
In `@codex-rs/core/src/tools/handlers/artifacts.rs`:
- Around line 340-343: Replace the hard-coded URL string in the assertion that
calls manager.config().release().base_url().as_str() with the crate's exported
release base-URL constant (use the constant exported by the release/config
module, e.g., RELEASE_BASE_URL or the crate's equivalent name) so the test
compares against that constant instead of duplicating the literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e48084a-4199-4931-9c8e-64d304eb7936
⛔ Files ignored due to path filters (2)
MODULE.bazel.lockis excluded by!**/*.lockcodex-rs/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (57)
codex-rs/Cargo.tomlcodex-rs/app-server-client/Cargo.tomlcodex-rs/app-server-client/README.mdcodex-rs/app-server-client/src/lib.rscodex-rs/app-server-protocol/Cargo.tomlcodex-rs/app-server-protocol/schema/json/EventMsg.jsoncodex-rs/app-server-protocol/schema/json/McpServerElicitationRequestParams.jsoncodex-rs/app-server-protocol/schema/json/McpServerElicitationRequestResponse.jsoncodex-rs/app-server-protocol/schema/json/ServerRequest.jsoncodex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.jsoncodex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.jsoncodex-rs/app-server-protocol/schema/typescript/ElicitationRequest.tscodex-rs/app-server-protocol/schema/typescript/ElicitationRequestEvent.tscodex-rs/app-server-protocol/schema/typescript/ServerRequest.tscodex-rs/app-server-protocol/schema/typescript/index.tscodex-rs/app-server-protocol/schema/typescript/v2/McpServerElicitationAction.tscodex-rs/app-server-protocol/schema/typescript/v2/McpServerElicitationRequestParams.tscodex-rs/app-server-protocol/schema/typescript/v2/McpServerElicitationRequestResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/index.tscodex-rs/app-server-protocol/src/protocol/common.rscodex-rs/app-server-protocol/src/protocol/v2.rscodex-rs/app-server/Cargo.tomlcodex-rs/app-server/README.mdcodex-rs/app-server/src/app_server_tracing.rscodex-rs/app-server/src/bespoke_event_handling.rscodex-rs/app-server/src/codex_message_processor.rscodex-rs/app-server/src/in_process.rscodex-rs/app-server/src/lib.rscodex-rs/app-server/src/message_processor.rscodex-rs/app-server/tests/common/models_cache.rscodex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rscodex-rs/app-server/tests/suite/v2/mod.rscodex-rs/artifacts/Cargo.tomlcodex-rs/artifacts/README.mdcodex-rs/artifacts/src/client.rscodex-rs/artifacts/src/lib.rscodex-rs/artifacts/src/runtime/error.rscodex-rs/artifacts/src/runtime/installed.rscodex-rs/artifacts/src/runtime/js_runtime.rscodex-rs/artifacts/src/runtime/manager.rscodex-rs/artifacts/src/runtime/manifest.rscodex-rs/artifacts/src/runtime/mod.rscodex-rs/artifacts/src/tests.rscodex-rs/codex-api/tests/models_integration.rscodex-rs/core/src/client_common.rscodex-rs/core/src/codex.rscodex-rs/core/src/exec.rscodex-rs/core/src/features.rscodex-rs/core/src/mcp_connection_manager.rscodex-rs/core/src/models_manager/model_info.rscodex-rs/core/src/plugins/manager.rscodex-rs/core/src/plugins/mod.rscodex-rs/core/src/plugins/render.rscodex-rs/core/src/project_doc.rscodex-rs/core/src/sandboxing/mod.rscodex-rs/core/src/tasks/user_shell.rscodex-rs/core/src/tools/handlers/artifacts.rs
✅ Files skipped from review due to trivial changes (1)
- codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json
🚧 Files skipped from review as they are similar to previous changes (5)
- codex-rs/app-server-client/README.md
- codex-rs/app-server/src/bespoke_event_handling.rs
- codex-rs/app-server-protocol/src/protocol/common.rs
- codex-rs/app-server-client/Cargo.toml
- codex-rs/Cargo.toml
Comment on lines
+66
to
+69
| "required": [ | ||
| "serverName", | ||
| "threadId" | ||
| ], |
There was a problem hiding this comment.
Schema/type contract drift: turnId is optional here but required in generated TS type.
At Line [66]-Line [69], turnId is not listed in required, but codex-rs/app-server-protocol/schema/typescript/v2/McpServerElicitationRequestParams.ts models it as mandatory (string | null). Please align both surfaces to avoid validator/codegen mismatch.
Proposed schema fix (if `turnId` should always be present, nullable when unknown)
"required": [
"serverName",
- "threadId"
+ "threadId",
+ "turnId"
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "required": [ | |
| "serverName", | |
| "threadId" | |
| ], | |
| "required": [ | |
| "serverName", | |
| "threadId", | |
| "turnId" | |
| ], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@codex-rs/app-server-protocol/schema/json/McpServerElicitationRequestParams.json`
around lines 66 - 69, The JSON schema for McpServerElicitationRequestParams is
missing "turnId" in its "required" array while the generated TypeScript type
McpServerElicitationRequestParams treats turnId as mandatory (string | null);
update the JSON schema by adding "turnId" to the "required" list and ensure the
turnId property schema allows null (e.g., type: ["string","null"] or equivalent)
so the schema and the generated TS type are aligned.
Comment on lines
+17
to
+23
| "content": { | ||
| "description": "Structured user input for accepted elicitations, mirroring RMCP `CreateElicitationResult`.\n\nThis is nullable because decline/cancel responses have no content." | ||
| } | ||
| }, | ||
| "required": [ | ||
| "action" | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== JSON Schema File ==="
if [ -f "codex-rs/app-server-protocol/schema/json/McpServerElicitationRequestResponse.json" ]; then
cat -n "codex-rs/app-server-protocol/schema/json/McpServerElicitationRequestResponse.json"
else
echo "File not found"
find . -name "*McpServerElicitationRequestResponse*" -type f
fi
echo ""
echo "=== TypeScript Generated File ==="
if [ -f "codex-rs/app-server-protocol/schema/typescript/v2/McpServerElicitationRequestResponse.ts" ]; then
cat -n "codex-rs/app-server-protocol/schema/typescript/v2/McpServerElicitationRequestResponse.ts"
else
echo "File not found"
find . -name "*McpServerElicitationRequestResponse*" -type f
fiRepository: fcoury/oldcodex
Length of output: 1616
Schema/TS contract mismatch on content field.
The JSON schema defines content as optional (not in required array) and lacks a type definition, but the generated TypeScript expects content: JsonValue | null as a required field. This allows payloads valid against the schema but invalid against the TypeScript contract.
Add "content" to the required array and define its type:
Proposed schema patch
"properties": {
"action": {
"$ref": "#/definitions/McpServerElicitationAction"
},
"content": {
- "description": "Structured user input for accepted elicitations, mirroring RMCP `CreateElicitationResult`.\n\nThis is nullable because decline/cancel responses have no content."
+ "type": ["array", "boolean", "number", "object", "string", "null"],
+ "description": "Structured user input for accepted elicitations, mirroring RMCP `CreateElicitationResult`.\n\nThis is nullable because decline/cancel responses have no content."
}
},
"required": [
- "action"
+ "action",
+ "content"
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "content": { | |
| "description": "Structured user input for accepted elicitations, mirroring RMCP `CreateElicitationResult`.\n\nThis is nullable because decline/cancel responses have no content." | |
| } | |
| }, | |
| "required": [ | |
| "action" | |
| ], | |
| "content": { | |
| "type": ["array", "boolean", "number", "object", "string", "null"], | |
| "description": "Structured user input for accepted elicitations, mirroring RMCP `CreateElicitationResult`.\n\nThis is nullable because decline/cancel responses have no content." | |
| } | |
| }, | |
| "required": [ | |
| "action", | |
| "content" | |
| ], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@codex-rs/app-server-protocol/schema/json/McpServerElicitationRequestResponse.json`
around lines 17 - 23, The schema's "content" property is currently missing a
type and not listed in "required", causing a mismatch with the generated
TypeScript which expects content: JsonValue | null; update the
McpServerElicitationRequestResponse JSON Schema by adding "content" to the
"required" array and define "content" to accept any JSON value (i.e., the JSON
Schema equivalent of JsonValue and nullable) so it aligns with the TS contract
(keep descriptive text in "description" and preserve nullability).
Comment on lines
+743
to
+745
| if let Some(after_spawn) = after_spawn { | ||
| after_spawn(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the exact file and lines mentioned
find . -type f -name "exec.rs" | head -20Repository: fcoury/oldcodex
Length of output: 122
🏁 Script executed:
# Count lines in the file to understand its size
wc -l codex-rs/core/src/exec.rsRepository: fcoury/oldcodex
Length of output: 89
🏁 Script executed:
# Read the section around lines 743-745 to verify the code matches
sed -n '730,760p' codex-rs/core/src/exec.rsRepository: fcoury/oldcodex
Length of output: 1252
🏁 Script executed:
# Search for the spawn_child_async call and context
rg -nP -A 20 'let\s+\w+\s+=\s+spawn_child_async' codex-rs/core/src/exec.rsRepository: fcoury/oldcodex
Length of output: 889
🏁 Script executed:
# Search for after_spawn callback type definitions
rg -nP 'after_spawn.*Option<Box<dyn FnOnce' codex-rs/core/src/exec.rsRepository: fcoury/oldcodex
Length of output: 168
🏁 Script executed:
# Look for all after_spawn usages
rg -nP '\bafter_spawn\b' codex-rs/core/src/exec.rs -C 4Repository: fcoury/oldcodex
Length of output: 1320
🏁 Script executed:
# Check if there are any panic-prone operations in callback registrations
rg -nP 'after_spawn\s*=' codex-rs/core/src/exec.rs -B 5 -A 10Repository: fcoury/oldcodex
Length of output: 41
🏁 Script executed:
# Read the consume_truncated_output function to understand what it does
sed -n '749,850p' codex-rs/core/src/exec.rsRepository: fcoury/oldcodex
Length of output: 3774
🏁 Script executed:
# Look for any cleanup/drop logic for Child processes
rg -nP 'impl.*Drop.*Child|fn drop|kill_child|terminate' codex-rs/core/src/exec.rs -C 3Repository: fcoury/oldcodex
Length of output: 1897
🏁 Script executed:
# Check the Child type definition and imports
rg -nP '^(use|type).*Child' codex-rs/core/src/exec.rs | head -20Repository: fcoury/oldcodex
Length of output: 128
🏁 Script executed:
# Find actual usages of after_spawn callback in tests and real code
rg -nP 'after_spawn\s*=' --type=rust -C 6Repository: fcoury/oldcodex
Length of output: 41
🏁 Script executed:
# Check what happens when we don't consume the child output
rg -nP 'spawn_child_async|Child\s*\{' codex-rs/core/src/exec.rs -C 2 | head -40Repository: fcoury/oldcodex
Length of output: 456
🏁 Script executed:
# Search for where after_spawn is passed when calling execute_exec_request
rg -nP 'execute_exec_request\(' --type=rust -C 5Repository: fcoury/oldcodex
Length of output: 3119
🏁 Script executed:
# Search for closures being passed to after_spawn
rg -nP 'after_spawn\s*:\s*' --type=rust -B 2 -A 2Repository: fcoury/oldcodex
Length of output: 3873
🏁 Script executed:
# Look for Box::new or move || patterns that create the callback
rg -nP 'Box::new\(.*\)|move\s*\|\|' codex-rs --type=rust -C 3 | grep -A 3 -B 3 'after\|spawn'Repository: fcoury/oldcodex
Length of output: 41014
🏁 Script executed:
# Check test code to see how after_spawn is used
rg -nP 'after_spawn' codex-rs/core/tests --type=rust -B 3 -A 3Repository: fcoury/oldcodex
Length of output: 41
🏁 Script executed:
# Search for where after_spawn callbacks are created/implemented
rg -nP 'Box::new\(|move\s*\|\|' codex-rs/shell-escalation/src/unix/escalate_server.rs -A 10 | grep -A 10 -B 10 'after_spawn'Repository: fcoury/oldcodex
Length of output: 41
🏁 Script executed:
# Look at the unix_escalation implementation
rg -nP 'after_spawn' codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs -B 5 -A 10Repository: fcoury/oldcodex
Length of output: 1947
🏁 Script executed:
# Check for callback implementations in test files
rg -nP 'after_spawn\s*=' codex-rs/core/tests --type=rust -B 3 -A 8Repository: fcoury/oldcodex
Length of output: 41
🏁 Script executed:
# Check if tokio::process::Child has Drop impl that kills process
python3 << 'SCRIPT'
import subprocess
result = subprocess.run(['rg', '-nP', 'impl.*Drop|kill|terminate', 'codex-rs/core/src/spawn.rs'],
capture_output=True, text=True)
print("In spawn.rs:")
print(result.stdout)
print("\nSearching for Child type behavior in Tokio documentation context...")
SCRIPTRepository: fcoury/oldcodex
Length of output: 296
🏁 Script executed:
# Check if there are any unwrap/expect calls in callback-related code
rg -nP '\b(unwrap|expect|panic!)\b' codex-rs/core/src/exec.rs -B 3 -A 1 | head -50Repository: fcoury/oldcodex
Length of output: 479
🏁 Script executed:
# Get more context about kill_on_drop from spawn.rs
sed -n '80,130p' codex-rs/core/src/spawn.rsRepository: fcoury/oldcodex
Length of output: 1830
🏁 Script executed:
# Search for actual callback implementations to see if panic is realistic
rg -nP 'after_spawn' codex-rs/shell-escalation -B 10 -A 5 | head -100Repository: fcoury/oldcodex
Length of output: 8056
🏁 Script executed:
# Check what happens with child process when it's dropped
rg -nP 'close_client_socket|FD_CLOEXEC|socket' codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs -B 2 -A 2Repository: fcoury/oldcodex
Length of output: 414
🏁 Script executed:
# Look for actual after_spawn callback implementations
fd -t f -e rs . codex-rs | xargs rg -l 'after_spawn.*Box::new|after_spawn.*\|' | head -10Repository: fcoury/oldcodex
Length of output: 41
🏁 Script executed:
# Check unix_escalation to see if there are socket operations that could panic
sed -n '740,800p' codex-rs/core/src/tools/runtimes/shell/unix_escalation.rsRepository: fcoury/oldcodex
Length of output: 2314
🏁 Script executed:
# Search for actual after_spawn callback implementations in the codebase
rg -nP 'after_spawn\s*:\s*Some|Box::new.*||move\s*\|' codex-rs/shell-escalation -B 5 -A 10Repository: fcoury/oldcodex
Length of output: 50374
🏁 Script executed:
# Look at where after_spawn is created/passed in shell-escalation
sed -n '540,570p' codex-rs/shell-escalation/src/unix/escalate_server.rsRepository: fcoury/oldcodex
Length of output: 1310
🏁 Script executed:
# Check if there are FD operations or socket operations that could panic
rg -nP 'close\(|dup2|fcntl.*socket' codex-rs/shell-escalation/src/unix -B 2 -A 2Repository: fcoury/oldcodex
Length of output: 2756
🏁 Script executed:
# Look at the EscalationSession to see how after_spawn callback is created
rg -nP 'after_spawn.*=|close_client_socket' codex-rs/shell-escalation/src/unix/escalate_server.rs -B 5 -A 10 | head -80Repository: fcoury/oldcodex
Length of output: 2384
Guard after_spawn panic to prevent losing output and bypassing cleanup logic.
If the callback panics at line 744, the panic unwinds before consume_truncated_output, which skips output collection and important cleanup handling (timeout signals, proper termination sequence). Although kill_on_drop(true) prevents process zombification, the loss of managed output handling is still critical.
🛠️ Proposed fix
- let child = spawn_child_async(SpawnChildRequest {
+ let mut child = spawn_child_async(SpawnChildRequest {
program: PathBuf::from(program),
args: args.into(),
arg0: arg0_ref,
cwd,
sandbox_policy,
@@
})
.await?;
if let Some(after_spawn) = after_spawn {
- after_spawn();
+ let callback_result =
+ std::panic::catch_unwind(std::panic::AssertUnwindSafe(after_spawn));
+ if callback_result.is_err() {
+ let _ = kill_child_process_group(&mut child);
+ let _ = child.start_kill();
+ return Err(CodexErr::Io(io::Error::other(
+ "after_spawn callback panicked",
+ )));
+ }
}
consume_truncated_output(child, expiration, stdout_stream).await🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/core/src/exec.rs` around lines 743 - 745, The after_spawn() callback
should be invoked inside std::panic::catch_unwind to prevent its panic from
skipping subsequent cleanup (like consume_truncated_output) — wrap the call to
after_spawn (the Some(after_spawn) branch) with catch_unwind, record whether it
returned Ok or Err, then proceed to call consume_truncated_output and other
termination/timeout handling as normal, and finally if catch_unwind returned Err
re-raise the panic using std::panic::resume_unwind; refer to the after_spawn
variable, the consume_truncated_output call, and ensure panic handling does not
alter existing kill_on_drop behavior.
Comment on lines
+14
to
+18
| lines.extend( | ||
| plugins | ||
| .iter() | ||
| .map(|plugin| format!("- `{}`", plugin.display_name)), | ||
| ); |
There was a problem hiding this comment.
Escape plugin display names before interpolating into markdown.
plugin.display_name is inserted raw into backticked markdown. A crafted name containing backticks/newlines can break formatting and alter instruction content.
🔧 Proposed hardening patch
- lines.extend(
- plugins
- .iter()
- .map(|plugin| format!("- `{}`", plugin.display_name)),
- );
+ lines.extend(plugins.iter().map(|plugin| {
+ let safe_name = plugin
+ .display_name
+ .replace('`', "\\`")
+ .replace('\n', " ")
+ .replace('\r', " ");
+ format!("- `{safe_name}`")
+ }));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| lines.extend( | |
| plugins | |
| .iter() | |
| .map(|plugin| format!("- `{}`", plugin.display_name)), | |
| ); | |
| lines.extend(plugins.iter().map(|plugin| { | |
| let safe_name = plugin | |
| .display_name | |
| .replace('`', "\\`") | |
| .replace('\n', " ") | |
| .replace('\r', " "); | |
| format!("- `{safe_name}`") | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/core/src/plugins/render.rs` around lines 14 - 18, The current
rendering inserts plugin.display_name raw into inline backticked markdown (in
the lines.extend mapping in render.rs), which allows backticks/newlines to break
formatting; fix it by sanitizing/escaping the display name before
interpolation—add or use a helper like escape_markdown_inline or
sanitize_display_name that replaces backticks with '\`' and collapses or removes
newlines (or encodes them) and then use that sanitized value in the format!("-
`{}`", ...). Update the mapping over plugins and any related helpers to ensure
all plugin.display_name usages in markdown go through this sanitizer.
ab1c02f to
b6fba47
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
codex-rs/core/src/config/mod.rs (1)
1829-1835:⚠️ Potential issue | 🟠 MajorPreserve explicit-vs-default distinction before pre-normalizing approval policy.
Line 1829 now applies fallback unconditionally, which pre-normalizes explicitly configured disallowed values and prevents
apply_requirement_constrained_value(...)from surfacing astartup_warningsmessage. It also logs"default approval policy..."for explicit values.💡 Proposed fix
- let mut approval_policy = approval_policy_override + let approval_policy_was_explicit = approval_policy_override.is_some() + || config_profile.approval_policy.is_some() + || cfg.approval_policy.is_some(); + + let mut approval_policy = approval_policy_override .or(config_profile.approval_policy) .or(cfg.approval_policy) .unwrap_or_else(|| { if active_project.is_trusted() { AskForApproval::OnRequest @@ - if let Err(err) = constrained_approval_policy.can_set(&approval_policy) { + if !approval_policy_was_explicit + && let Err(err) = constrained_approval_policy.can_set(&approval_policy) + { tracing::warn!( error = %err, "default approval policy is disallowed by requirements; falling back to required default" ); approval_policy = constrained_approval_policy.value(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/config/mod.rs` around lines 1829 - 1835, The current code unconditionally replaces approval_policy with constrained_approval_policy.value() when can_set fails, which pre-normalizes explicit user values and prevents apply_requirement_constrained_value(...) from surfacing startup_warnings; change the logic to only perform the fallback when the approval_policy was not explicitly provided (i.e., it is the default) — call constrained_approval_policy.can_set(&approval_policy) and if it errors, only assign approval_policy = constrained_approval_policy.value() when the original approval_policy is the default/unset sentinel; if the value was explicit, leave approval_policy untouched so apply_requirement_constrained_value(...) can run and emit warnings.codex-rs/core/src/skills/loader.rs (1)
1536-1579:⚠️ Potential issue | 🟡 MinorTest name is misleading.
The test
loads_skill_macos_permissions_from_yaml_non_macos_does_not_create_profileasserts that thepermission_profileis created withmacos: Some(MacOsSeatbeltProfileExtensions { ... }), which contradicts thedoes_not_create_profilein the name.If the intended behavior is that macOS permissions are now preserved on non-macOS platforms, consider renaming to something like
loads_skill_macos_permissions_from_yaml_on_non_macos.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/skills/loader.rs` around lines 1536 - 1579, Rename the misleading test function loads_skill_macos_permissions_from_yaml_non_macos_does_not_create_profile to a name that matches its assertions (e.g., loads_skill_macos_permissions_from_yaml_on_non_macos) and update the function identifier wherever referenced; ensure the #[cfg(not(target_os = "macos"))] attribute and any test helpers (load_skills_for_test, write_skill_metadata_at) remain the same so behavior is unchanged.codex-rs/core/src/seatbelt.rs (1)
267-287:⚠️ Potential issue | 🟠 MajorUnix-socket-only configs can be silently dropped from network policy.
At Line 267, restricted mode does not consider unix-socket allowances. If
portsis empty,has_proxy_configis false, and managed network is off, configured unix socket access won’t be emitted at all.🔧 Proposed fix
fn dynamic_network_policy( sandbox_policy: &SandboxPolicy, enforce_managed_network: bool, proxy: &ProxyPolicyInputs, ) -> String { - let should_use_restricted_network_policy = - !proxy.ports.is_empty() || proxy.has_proxy_config || enforce_managed_network; + let unix_socket_rules = unix_socket_policy(proxy); + let should_use_restricted_network_policy = !proxy.ports.is_empty() + || proxy.has_proxy_config + || enforce_managed_network + || !unix_socket_rules.is_empty(); if should_use_restricted_network_policy { let mut policy = String::new(); if proxy.allow_local_binding { policy.push_str("; allow loopback local binding and loopback traffic\n"); policy.push_str("(allow network-bind (local ip \"localhost:*\"))\n"); policy.push_str("(allow network-inbound (local ip \"localhost:*\"))\n"); policy.push_str("(allow network-outbound (remote ip \"localhost:*\"))\n"); } for port in &proxy.ports { policy.push_str(&format!( "(allow network-outbound (remote ip \"localhost:{port}\"))\n" )); } - let unix_socket_policy = unix_socket_policy(proxy); - if !unix_socket_policy.is_empty() { + if !unix_socket_rules.is_empty() { policy.push_str("; allow unix domain sockets for local IPC\n"); - policy.push_str(&unix_socket_policy); + policy.push_str(&unix_socket_rules); } return format!("{policy}{MACOS_SEATBELT_NETWORK_POLICY}"); }Please also add a regression test for “unix sockets configured with no proxy ports/env”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/seatbelt.rs` around lines 267 - 287, The current restricted-policy gate (should_use_restricted_network_policy) ignores unix socket allowances when proxy.ports is empty and proxy.has_proxy_config and enforce_managed_network are false, so unix_socket_policy(proxy) can be dropped; change the condition to also enter the policy block when unix_socket_policy(proxy) is non-empty (call unix_socket_policy before the if or compute a bool unix_allowed) and ensure its contents are appended and returned alongside MACOS_SEATBELT_NETWORK_POLICY; update the code paths using should_use_restricted_network_policy and the function that builds the policy (the block that returns format!("{policy}{MACOS_SEATBELT_NETWORK_POLICY}")) to include unix socket policy even when ports/config flags are absent, and add a regression test that creates a proxy with unix socket config but no ports/env to assert the generated policy contains the unix socket rules.codex-rs/core/src/client_common.rs (1)
170-180:⚠️ Potential issue | 🟠 MajorFix API compatibility and type safety for tool fields.
output_formatshould use a typed enum with lowercase values (png,jpeg,webpper API docs). More critically,search_content_typesis not documented in the OpenAI Responses API—it may be an unsupported field causing the web_search errors noted in the TODO comment on line 171. Remove this field or verify it's supported before merging.♻️ Proposed refactor
+ #[derive(Debug, Clone, Serialize, PartialEq)] + #[serde(rename_all = "lowercase")] + pub(crate) enum ImageOutputFormat { + Png, + Jpeg, + Webp, + } + #[derive(Debug, Clone, Serialize, PartialEq)] #[serde(tag = "type")] pub(crate) enum ToolSpec { @@ #[serde(rename = "image_generation")] - ImageGeneration { output_format: String }, + ImageGeneration { output_format: ImageOutputFormat }, @@ #[serde(rename = "web_search")] WebSearch { #[serde(skip_serializing_if = "Option::is_none")] external_web_access: Option<bool>, - #[serde(skip_serializing_if = "Option::is_none")] - search_content_types: Option<Vec<String>>, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/client_common.rs` around lines 170 - 180, The ImageGeneration variant currently uses a String for output_format and the WebSearch variant includes an undocumented search_content_types field; change ImageGeneration::output_format to a typed enum (e.g., OutputFormat with lowercase variants "png", "jpeg", "webp") to enforce valid values and remove (or gate behind verification) the WebSearch::search_content_types field—keep only external_web_access in the WebSearch struct to avoid sending unsupported fields to the Responses API (update serde rename/attributes accordingly and adapt any code that constructs or matches on these variants).codex-rs/core/src/mcp_connection_manager.rs (1)
331-356:⚠️ Potential issue | 🔴 CriticalHandle elicitation event send failures to avoid stuck awaits.
At Line 336, send failure is ignored, but Line 354 still awaits the oneshot response while its sender remains stored in the pending map. That can block indefinitely.
🐛 Proposed fix
let (tx, rx) = oneshot::channel(); + let key = (server_name.clone(), id.clone()); { let mut lock = elicitation_requests.lock().await; - lock.insert((server_name.clone(), id.clone()), tx); + lock.insert(key.clone(), tx); } - let _ = tx_event + if let Err(err) = tx_event .send(Event { id: "mcp_elicitation_request".to_string(), msg: EventMsg::ElicitationRequest(ElicitationRequestEvent { turn_id: None, server_name, @@ request, }), }) - .await; + .await + { + let mut lock = elicitation_requests.lock().await; + lock.remove(&key); + return Err(anyhow!("failed to dispatch elicitation request: {err}")); + } rx.await .context("elicitation request channel closed unexpectedly")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/mcp_connection_manager.rs` around lines 331 - 356, The code stores a oneshot sender in elicitation_requests and then ignores failures from tx_event.send, which can leave the sender in the map and make rx.await block forever; update the block around elicitation_requests, tx_event.send and rx.await so that you check the result of tx_event.send(Event { ... }) and if it Errs remove the previously inserted sender from elicitation_requests (using the same key (server_name.clone(), id.clone())) and return/propagate an error instead of awaiting rx; ensure the map removal happens under the same async lock to avoid races and that you do not leave the sender alive in the map when send fails (refer to elicitation_requests, tx_event.send, tx/rx oneshot pair, and ProtocolRequestId conversion to locate the code).codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json (1)
3265-3289:⚠️ Potential issue | 🟠 Major
elicitation_requestevent shape changed incompatibly (message→request).Making
requestrequired without a compatibility alias can break existing clients that still decode the previous field. Keep both during migration (accept either, emit canonical one).Transitional compatibility pattern
"properties": { "id": { "$ref": "#/definitions/v2/RequestId" }, + "message": { + "$ref": "#/definitions/ElicitationRequest" + }, "request": { "$ref": "#/definitions/ElicitationRequest" }, @@ - "required": [ - "id", - "request", - "server_name", - "type" - ], + "anyOf": [ + { "required": ["id", "request", "server_name", "type"] }, + { "required": ["id", "message", "server_name", "type"] } + ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json` around lines 3265 - 3289, The schema now breaks compatibility by renaming property "message" -> "request" and making "request" required; restore backward-compatibility by keeping both properties and requiring at least one. In the object that defines the elicitation event (the schema with enum "ElicitationRequestEventMsgType" and properties "request" and "server_name"), add a "message" property with the same schema as "request" (e.g., $ref to "#/definitions/ElicitationRequest"), remove the unconditional "request" from "required", and add a oneOf (or anyOf) constraint such as [{"required":["request"]},{"required":["message"]}] so either field is accepted. Also note producers should emit the canonical "request" field going forward.codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json (1)
4851-4875:⚠️ Potential issue | 🟠 Major
ElicitationRequestEventMsgintroduces a wire-compatibility break.Line 4874 now requires
requestand no legacymessagefallback exists. Existing clients that only understand the old shape will fail to decode this event.Proposed compatibility-safe schema adjustment
{ "properties": { "id": { "$ref": "#/definitions/RequestId" }, + "message": { + "type": "string" + }, "request": { "$ref": "#/definitions/ElicitationRequest" }, "server_name": { "type": "string" }, "turn_id": { "description": "Turn ID that this elicitation belongs to, when known.", "type": [ "string", "null" ] }, "type": { "enum": [ "elicitation_request" ], "title": "ElicitationRequestEventMsgType", "type": "string" } }, - "required": [ - "id", - "request", - "server_name", - "type" - ], + "required": ["id", "server_name", "type"], + "allOf": [ + { + "anyOf": [ + { "required": ["request"] }, + { "required": ["message"] } + ] + } + ], "title": "ElicitationRequestEventMsg", "type": "object" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json` around lines 4851 - 4875, The schema change to ElicitationRequestEventMsg currently makes "request" mandatory and drops the legacy "message" field, breaking wire compatibility; update the schema for the ElicitationRequestEventMsg definition so decoders accept either the new "request" or the old "message" shape (e.g., remove "request" from the strict "required" list and add an anyOf/oneOf clause that enforces presence of at least one of ["request","message"] or otherwise allow "message" as an alternative), ensuring the "request" property still references `#/definitions/ElicitationRequest` and the legacy "message" property (with its original schema) is preserved to support old clients.
♻️ Duplicate comments (6)
codex-rs/app-server/src/app_server_tracing.rs (1)
89-95:⚠️ Potential issue | 🟠 MajorUse real typed RPC method names in in-process span fields.
Line 92 and Line 94 still hardcode
"in_process", which collapses all in-process calls into one telemetry bucket. Please derive method names fromClientRequestso traces/metrics preserve per-RPC granularity.Suggested change
pub(crate) fn typed_request_span( request: &ClientRequest, connection_id: ConnectionId, session: &ConnectionSessionState, ) -> Span { + let method = typed_request_method(request); let span = info_span!( "app_server.request", otel.kind = "server", - otel.name = "in_process", + otel.name = method, rpc.system = "jsonrpc", - rpc.method = "in_process", + rpc.method = method, rpc.transport = "in-process", rpc.request_id = ?client_request_id(request), app_server.connection_id = ?connection_id, app_server.api_version = "v2", app_server.client_name = field::Empty, app_server.client_version = field::Empty, );fn typed_request_method(request: &ClientRequest) -> &'static str { match request { ClientRequest::Initialize { .. } => "initialize", // ...exhaustive mapping for the remaining variants... ClientRequest::FuzzyFileSearchSessionStop { .. } => "fuzzyFileSearchSession/stop", } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/app_server_tracing.rs` around lines 89 - 95, The span currently hardcodes "in_process" for otel.name and rpc.method; create a helper fn like typed_request_method(request: &ClientRequest) -> &'static str that returns the proper RPC method string for each ClientRequest variant (e.g., ClientRequest::Initialize => "initialize", etc.), then call that helper when building the span (use the returned &str for both otel.name and rpc.method in the info_span! invocation) so in-process spans preserve per-RPC granularity.codex-rs/app-server-protocol/schema/json/McpServerElicitationRequestParams.json (1)
603-606:⚠️ Potential issue | 🟠 MajorSchema/type drift remains for
turnIdrequiredness.Line 603–606 omits
turnIdfromrequired, while generated TS modelsturnIdas mandatory (string | null). This lets schema validation accept payloads that typed clients reject.Proposed schema fix
"required": [ "serverName", - "threadId" + "threadId", + "turnId" ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/McpServerElicitationRequestParams.json` around lines 603 - 606, The JSON schema McpServerElicitationRequestParams.json currently lists only "serverName" and "threadId" as required but the generated TypeScript models treat "turnId" as mandatory (string | null); update the schema by adding "turnId" to the "required" array so runtime validation matches the TS type, and re-generate/verify any codegen to ensure the models and schema no longer drift (refer to the "required" array and the "turnId" property in the same schema).codex-rs/app-server/src/lib.rs (1)
340-346:⚠️ Potential issue | 🟠 Major
session_sourceis still hard-coded to VSCode.Line 607 forces
SessionSource::VSCode, which can misclassify non-VSCode callers ofrun_main_with_transportand skew routing/telemetry semantics. Thread this through the runtime entrypoint instead of fixing it here.Proposed fix
pub async fn run_main( arg0_paths: Arg0DispatchPaths, cli_config_overrides: CliConfigOverrides, loader_overrides: LoaderOverrides, default_analytics_enabled: bool, ) -> IoResult<()> { run_main_with_transport( arg0_paths, cli_config_overrides, loader_overrides, default_analytics_enabled, AppServerTransport::Stdio, + SessionSource::VSCode, ) .await } pub async fn run_main_with_transport( arg0_paths: Arg0DispatchPaths, cli_config_overrides: CliConfigOverrides, loader_overrides: LoaderOverrides, default_analytics_enabled: bool, transport: AppServerTransport, + session_source: SessionSource, ) -> IoResult<()> { @@ let mut processor = MessageProcessor::new(MessageProcessorArgs { @@ - session_source: SessionSource::VSCode, + session_source, });Also applies to: 597-608
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/lib.rs` around lines 340 - 346, The code currently forces SessionSource::VSCode inside run_main_with_transport, misclassifying callers; update the function signature for run_main_with_transport to accept a SessionSource parameter (or an Option<SessionSource>) and replace the hard-coded SessionSource::VSCode use with that parameter, then propagate the new parameter from the runtime entrypoint that calls run_main_with_transport so callers supply the correct SessionSource; locate uses of run_main_with_transport and the place where SessionSource::VSCode is created and remove the hard-coded value so routing/telemetry see the threaded-in session_source instead.codex-rs/app-server/src/message_processor.rs (1)
327-338:⚠️ Potential issue | 🟠 MajorRedact inbound payload logs to avoid leaking auth and user data.
Line 330, Line 337, Line 375, and Line 382 still emit full protocol objects. These paths can carry sensitive fields and should log only safe metadata.
💡 Suggested fix
pub(crate) async fn process_notification(&self, notification: JSONRPCNotification) { - // Currently, we do not expect to receive any notifications from the - // client, so we just log them. - tracing::info!("<- notification: {:?}", notification); + tracing::debug!(method = %notification.method, "<- notification received"); } /// Handles typed notifications from in-process clients. pub(crate) async fn process_client_notification(&self, notification: ClientNotification) { - // Currently, we do not expect to receive any typed notifications from - // in-process clients, so we just log them. - tracing::info!("<- typed notification: {:?}", notification); + let _ = notification; + tracing::debug!("<- typed notification received (payload omitted)"); } /// Handle a standalone JSON-RPC response originating from the peer. pub(crate) async fn process_response(&mut self, response: JSONRPCResponse) { - tracing::info!("<- response: {:?}", response); let JSONRPCResponse { id, result, .. } = response; + tracing::debug!(response_id = ?id, "<- response received"); self.outgoing.notify_client_response(id, result).await } /// Handle an error object received from the peer. pub(crate) async fn process_error(&mut self, err: JSONRPCError) { - tracing::error!("<- error: {:?}", err); - self.outgoing.notify_client_error(err.id, err.error).await; + let JSONRPCError { id, error, .. } = err; + tracing::warn!(response_id = ?id, code = error.code, "<- error received"); + self.outgoing.notify_client_error(id, error).await; }Also applies to: 374-383
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/message_processor.rs` around lines 327 - 338, The two logging sites (process_notification and process_client_notification) currently log full inbound payloads and must be changed to redact sensitive fields; update tracing::info! in process_notification and process_client_notification so they do not print entire JSONRPCNotification or ClientNotification objects but instead log only safe metadata such as the notification type/method, an optional id or timestamp, and a truncated/omitted params payload (e.g., replace params with "(redacted)" or log its size/type), ensuring any auth/user fields are never included; locate the tracing::info! calls in those two functions and replace their arguments with a small struct or formatted string containing only the allowed metadata.codex-rs/app-server/src/in_process.rs (1)
256-263:⚠️ Potential issue | 🟠 MajorBound the shutdown enqueue wait to avoid a potential hang.
At Line 256,
client_tx.send(...).awaitcan block indefinitely on a full bounded channel, so the timeout/abort path at Line 265 may never run.🔧 Proposed fix
- if self - .client_tx - .send(InProcessClientMessage::Shutdown { done_tx }) - .await - .is_ok() - { + let shutdown_enqueued = timeout( + SHUTDOWN_TIMEOUT, + self.client_tx.send(InProcessClientMessage::Shutdown { done_tx }), + ) + .await + .is_ok_and(|result| result.is_ok()); + if shutdown_enqueued { let _ = timeout(SHUTDOWN_TIMEOUT, done_rx).await; }#!/bin/bash set -euo pipefail # Verify shutdown currently does an unbounded await on enqueue. sed -n '248,270p' codex-rs/app-server/src/in_process.rs # Verify the client queue is bounded. sed -n '314,321p' codex-rs/app-server/src/in_process.rs # Confirm no timeout wrapper exists around this specific send. rg -n 'client_tx\.send\(InProcessClientMessage::Shutdown|timeout\(SHUTDOWN_TIMEOUT,\s*self\.client_tx\.send' codex-rs/app-server/src/in_process.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/in_process.rs` around lines 256 - 263, The shutdown path currently awaits self.client_tx.send(InProcessClientMessage::Shutdown { done_tx }).await which can block indefinitely on a full bounded channel; wrap the enqueue in a timeout or use a non-blocking send to ensure the overall shutdown timeout (SHUTDOWN_TIMEOUT) can trigger: replace the direct .send(...).await with a timeout(SHUTDOWN_TIMEOUT, self.client_tx.send(...)).await (or attempt try_send and treat Err(TrySendError::Full) as a timeout/failure), and only await done_rx if the enqueue succeeded; update handling around InProcessClientMessage::Shutdown, client_tx, SHUTDOWN_TIMEOUT, and done_tx/done_rx accordingly so the shutdown path cannot hang forever.codex-rs/app-server-protocol/src/protocol/v2.rs (1)
4457-4460:⚠️ Potential issue | 🟠 MajorForce TS
numberfor JSON-facingminItems/maxItemsbounds.Lines 4457/4460/4482/4485 use
Option<u64>. Withts-rs, this typically emitsbigint, which is not JSON-compatible for these schema fields.🔧 Proposed fix
pub struct McpElicitationUntitledMultiSelectEnumSchema { @@ - #[ts(optional)] + #[ts(optional, type = "number")] pub min_items: Option<u64>, @@ - #[ts(optional)] + #[ts(optional, type = "number")] pub max_items: Option<u64>, @@ pub struct McpElicitationTitledMultiSelectEnumSchema { @@ - #[ts(optional)] + #[ts(optional, type = "number")] pub min_items: Option<u64>, @@ - #[ts(optional)] + #[ts(optional, type = "number")] pub max_items: Option<u64>,#!/bin/bash set -euo pipefail # Show the Rust fields currently typed as Option<u64>. rg -n -C2 'pub (min_items|max_items): Option<u64>' codex-rs/app-server-protocol/src/protocol/v2.rs # Inspect generated TypeScript for emitted field types. fd 'McpElicitation(UntitledMultiSelectEnumSchema|TitledMultiSelectEnumSchema)\.ts' codex-rs/app-server-protocol/schema/typescript/v2 \ | xargs -I{} sh -c 'echo "---- {}"; rg -n "minItems|maxItems|bigint|number" "{}"'Also applies to: 4482-4485
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/src/protocol/v2.rs` around lines 4457 - 4460, The TypeScript emit treats Rust u64 as bigint; to force JSON-compatible number types, add the ts-rs attribute #[ts(type = "number")] to the min_items and max_items fields (instead of changing their Rust type) in the relevant structs (e.g., UntitledMultiSelectEnumSchema and TitledMultiSelectEnumSchema and the other occurrences), i.e., place #[ts(optional)] and #[ts(type = "number")] (and keep serde(skip_serializing_if = "Option::is_none")) above pub min_items: Option<u64> and pub max_items: Option<u64> so generated .ts uses number for these schema bounds.
🧹 Nitpick comments (17)
codex-rs/app-server-client/README.md (1)
64-65: Clarify what exactly is aborted on shutdown timeout.“aborts” can be read as process-level termination. Suggest explicitly naming the aborted unit (e.g., worker task/runtime loop) to avoid operational misinterpretation.
✏️ Suggested wording
-- `shutdown()` performs a bounded graceful shutdown and then aborts if timeout - is exceeded. +- `shutdown()` performs a bounded graceful shutdown and then aborts the + in-process worker task if the timeout is exceeded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-client/README.md` around lines 64 - 65, The README's description of shutdown() is ambiguous about what "aborts" — update the sentence to explicitly name the aborted unit (for example: "aborts the worker task", "aborts the runtime loop", or "terminates the process") and choose the correct one that matches implementation; edit the `shutdown()` description to read something like "shutdown() performs a bounded graceful shutdown and aborts the worker runtime loop if the timeout is exceeded" (or "aborts the process" if that is the actual behavior) so readers know exactly what is being terminated.codex-rs/core/src/sandboxing/macos_permissions.rs (1)
78-79: Optional: run these pure merge tests on all targets.Line 78 restricts tests to macOS, but the helpers here are pure merge logic and can be validated cross-platform for wider CI coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/sandboxing/macos_permissions.rs` around lines 78 - 79, The tests module is unnecessarily restricted to macOS by the attribute #[cfg(all(test, target_os = "macos"))]; change it to run on all test targets by replacing that attribute with #[cfg(test)] (or removing the target_os condition) so the module tests (mod tests) and its helper functions for pure merge logic execute on CI across platforms.codex-rs/core/src/tools/handlers/mod.rs (1)
98-99: Either use or remove_cwdto avoid interface drift.Line 98 adds
_cwd, but it is currently unused in validation/normalization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/tools/handlers/mod.rs` around lines 98 - 99, The added parameter `_cwd: &Path` is unused and causing interface drift; either remove `_cwd` from the function signature that returns Result<Option<PermissionProfile>, String> and update all callers to stop passing a cwd, or actually use `_cwd` in the function's validation/normalization logic (e.g., resolve relative paths, check file existence, or derive context for PermissionProfile) and ensure the parameter name is not prefixed with `_` if used; update any caller sites accordingly to match the chosen approach.codex-rs/app-server-protocol/schema/json/v2/PluginListResponse.json (1)
9-11: Consider a stricter path type for response fields.Line 9 and Line 30 currently accept arbitrary strings. Reusing an absolute-path schema type here would keep request/response path guarantees consistent.
Also applies to: 30-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/v2/PluginListResponse.json` around lines 9 - 11, The response schema uses plain string types for file paths (the "path" properties in PluginListResponse.json); replace those loose "type: string" declarations with a reference to the existing absolute-path schema (i.e. use the shared absolute-path schema/$ref instead of a raw string) so request/response path validation is consistent — update both the top-level "path" field and the other occurrence around lines 30-32 to point to the absolute-path schema.codex-rs/app-server/src/codex_message_processor.rs (1)
3925-3930: Optional: centralize repeated config-reload error handling.Line 3925 (and the same pattern at Lines 4040, 4099, 4205, 4628, 5005, 5184) repeats
load_latest_config(...)+send_error+return. A small helper would reduce drift and keep request handlers tighter.♻️ Proposed refactor
+ async fn load_latest_config_or_send_error( + &self, + request_id: ConnectionRequestId, + fallback_cwd: Option<PathBuf>, + ) -> Option<Config> { + match self.load_latest_config(fallback_cwd).await { + Ok(config) => Some(config), + Err(error) => { + self.outgoing.send_error(request_id, error).await; + None + } + } + }- let config = match self.load_latest_config(None).await { - Ok(config) => config, - Err(error) => { - self.outgoing.send_error(request_id, error).await; - return; - } - }; + let Some(config) = self + .load_latest_config_or_send_error(request_id, None) + .await + else { + return; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/codex_message_processor.rs` around lines 3925 - 3930, Several handlers repeat the pattern of calling self.load_latest_config(...), sending an error via self.outgoing.send_error(request_id, error).await and returning; extract this into a small helper on the struct (e.g., a method like try_load_latest_config(&self, request_id) -> Result<Config, ()> or load_config_or_send_error(&self, request_id) -> Option<Config>) and replace direct calls at sites using load_latest_config, so callers call the helper (referencing load_latest_config, outgoing.send_error, and request_id) and either get the Config or early-return when the helper already sent the error.codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json (1)
11784-11786: UseAbsolutePathBufconsistently for plugin marketplace/local paths.
plugin/installrequires a normalized absolute path, but list/source path fields are plain strings. Aligning these types prevents handoff mismatches fromplugin/listtoplugin/install.Type-consistency patch
"PluginMarketplaceEntry": { "properties": { @@ "path": { - "type": "string" + "$ref": "#/definitions/v2/AbsolutePathBuf" }, @@ "PluginSource": { "oneOf": [ { "properties": { "path": { - "type": "string" + "$ref": "#/definitions/v2/AbsolutePathBuf" },Also applies to: 11852-11854, 11873-11875
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json` around lines 11784 - 11786, Several plugin-related path fields are plain strings but must be normalized absolute paths to match plugin/install expectations; update the JSON schema so fields like "marketplacePath" (and the other occurrences noted around the 11852–11854 and 11873–11875 regions) reference the v2/AbsolutePathBuf definition instead of a plain string. Locate the plugin/list and plugin/source (or list/source path) schema entries and replace their string type or $ref with "$ref": "#/definitions/v2/AbsolutePathBuf" so plugin/list outputs and plugin/install inputs use the same AbsolutePathBuf type.codex-rs/core/src/sandboxing/mod.rs (1)
458-473: Consider deduplicating execution wrapper logic.Line 462 and Line 471 repeat the same clone-and-dispatch flow. A small private helper would reduce drift risk.
Refactor sketch
+async fn execute_with_effective_policy( + exec_request: ExecRequest, + stdout_stream: Option<StdoutStream>, + after_spawn: Option<Box<dyn FnOnce() + Send>>, +) -> crate::error::Result<ExecToolCallOutput> { + let effective_policy = exec_request.sandbox_policy.clone(); + execute_exec_request(exec_request, &effective_policy, stdout_stream, after_spawn).await +} + pub async fn execute_env( exec_request: ExecRequest, stdout_stream: Option<StdoutStream>, ) -> crate::error::Result<ExecToolCallOutput> { - let effective_policy = exec_request.sandbox_policy.clone(); - execute_exec_request(exec_request, &effective_policy, stdout_stream, None).await + execute_with_effective_policy(exec_request, stdout_stream, None).await } pub async fn execute_exec_request_with_after_spawn( exec_request: ExecRequest, stdout_stream: Option<StdoutStream>, after_spawn: Option<Box<dyn FnOnce() + Send>>, ) -> crate::error::Result<ExecToolCallOutput> { - let effective_policy = exec_request.sandbox_policy.clone(); - execute_exec_request(exec_request, &effective_policy, stdout_stream, after_spawn).await + execute_with_effective_policy(exec_request, stdout_stream, after_spawn).await }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/sandboxing/mod.rs` around lines 458 - 473, Both execute_env and execute_exec_request_with_after_spawn duplicate the clone-and-dispatch logic for sandbox_policy; extract that into a small private helper (e.g., fn dispatch_with_policy(exec_request: ExecRequest, stdout_stream: Option<StdoutStream>, after_spawn: Option<Box<dyn FnOnce() + Send>>) -> impl Future<Output = crate::error::Result<ExecToolCallOutput>> or an async fn) that clones exec_request.sandbox_policy into effective_policy and calls execute_exec_request(exec_request, &effective_policy, stdout_stream, after_spawn). Then have execute_env and execute_exec_request_with_after_spawn call this helper, passing None or the provided after_spawn respectively to remove the duplicated clone-and-dispatch flow.codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json (3)
14071-14076: Alignsaved_pathacross image-generation item schemas.
saved_pathis now present onTurnItem.ImageGeneration, but theThreadItem.imageGenerationshape still cannot represent it. Keeping both aligned avoids field loss between live and persisted/readback views.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json` around lines 14071 - 14076, ThreadItem.imageGeneration schema is missing the saved_path property that exists on TurnItem.ImageGeneration, causing loss when persisting/reading back image-generation items; update the ThreadItem.imageGeneration JSON schema to include saved_path with the same type union ["string","null"] (mirror the property definition from TurnItem.ImageGeneration) so both shapes align, and ensure any accompanying required/optional lists match TurnItem.ImageGeneration's treatment of saved_path.
8509-8511: UseAbsolutePathBuffor plugin path fields for contract consistency.
PluginMarketplaceEntry.pathandPluginSource.pathare plainstring, whilePluginInstallParams.marketplacePathisAbsolutePathBuf. Aligning these prevents round-trip type drift.Suggested path typing alignment
"PluginMarketplaceEntry": { "properties": { "name": { "type": "string" }, "path": { - "type": "string" + "$ref": "#/definitions/AbsolutePathBuf" }, "plugins": { "items": { "$ref": "#/definitions/PluginSummary" }, "type": "array" } }, @@ "PluginSource": { "oneOf": [ { "properties": { "path": { - "type": "string" + "$ref": "#/definitions/AbsolutePathBuf" }, "type": { "enum": [ "local" ],Also applies to: 8530-8532
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json` around lines 8509 - 8511, Change the JSON schema for the plugin path fields from a plain string to the AbsolutePathBuf type to keep contracts consistent and avoid round-trip drift: update the "path" property in PluginMarketplaceEntry (schema name PluginMarketplaceEntry.path) and PluginSource (PluginSource.path) to use the AbsolutePathBuf schema/type (the same type used by PluginInstallParams.marketplacePath), ensuring any $ref or type declaration matches the existing AbsolutePathBuf schema definition used elsewhere in the spec.
8441-8450: Consider a temporary compatibility alias formarketplacePath.Line 8449 makes
marketplacePathmandatory immediately. If any existing v2 clients still send the previous field, this will hard-fail requests. A short transition window alias can reduce rollout risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json` around lines 8441 - 8450, Add a short-lived compatibility alias for the previous marketplace field so v2 doesn't hard-fail: introduce a new optional property (e.g., legacyMarketplacePath) with the same "$ref": "#/definitions/AbsolutePathBuf" alongside "marketplacePath" and then replace the strict "required": ["marketplacePath","pluginName"] with a oneOf/anyOf that enforces "pluginName" plus either "marketplacePath" or the legacy property (e.g., oneOf: [{required:["marketplacePath","pluginName"]},{required:["legacyMarketplacePath","pluginName"]}]). This preserves the existing AbsolutePathBuf typing and allows a transition window before removing the alias.codex-rs/app-server-client/Cargo.toml (1)
22-22: Consider removing redundantserde_jsonin dev-dependencies.
serde_jsonis already included in[dependencies](line 22), so the entry in[dev-dependencies](line 29) is redundant. While harmless, removing it would reduce manifest noise.♻️ Suggested fix
[dev-dependencies] pretty_assertions = { workspace = true } -serde_json = { workspace = true } tokio = { workspace = true, features = ["macros", "rt-multi-thread"] }Also applies to: 29-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-client/Cargo.toml` at line 22, Remove the redundant serde_json entry from the [dev-dependencies] section of Cargo.toml since serde_json is already declared in [dependencies]; locate the serde_json key under the dev-dependencies table and delete that entry to avoid duplicate manifest noise while keeping the primary serde_json dependency in place.codex-rs/app-server/tests/suite/v2/app_list.rs (1)
87-1135: Consider centralizingAppInfotest fixture construction.There is substantial repeated literal construction; a small helper/builder would reduce churn when
AppInfoevolves again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/tests/suite/v2/app_list.rs` around lines 87 - 1135, Tests repeatedly construct AppInfo literals; extract a centralized builder/helper to reduce duplication: create a helper function or struct (e.g., app_info_fixture or AppInfoBuilder) used by tests like list_apps_reports_is_enabled_from_config, list_apps_emits_updates_and_returns_after_both_lists_load, list_apps_waits_for_accessible_data_before_emitting_directory_updates, etc., to produce common variants (accessible/inaccessible, with install_url, with branding/metadata) and update usages of raw AppInfo { ... } instances and expected_* vectors to call the builder with succinct methods (e.g., .id("beta").name("Beta").accessible(true).install_url(...)) so changes to AppInfo fields only require updates in one place.codex-rs/app-server/README.md (1)
157-157: Tighten phrasing for readability.Line 157 can be shortened to “inaccessible in the current ChatGPT auth context” to reduce wordiness without changing meaning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/README.md` at line 157, Update the README entry for `plugin/install` to tighten the phrasing: replace the clause "that still are not accessible in the current ChatGPT auth context" with "inaccessible in the current ChatGPT auth context" so the description for the returned `appsNeedingAuth` is more concise while preserving meaning.codex-rs/core/src/connectors.rs (2)
432-441: Avoid overwriting existing plugin sources during enrichment.Line [437]-Line [439] replaces
connector.plugin_display_namesoutright. If provenance is partial, this can drop names already collected fromToolInfo. Prefer union + dedupe.Suggested fix (merge instead of replace)
pub fn with_app_plugin_sources( mut connectors: Vec<AppInfo>, tool_plugin_provenance: &ToolPluginProvenance, ) -> Vec<AppInfo> { for connector in &mut connectors { - connector.plugin_display_names = tool_plugin_provenance - .plugin_display_names_for_connector_id(connector.id.as_str()) - .to_vec(); + let mut merged: BTreeSet<String> = connector.plugin_display_names.drain(..).collect(); + merged.extend( + tool_plugin_provenance + .plugin_display_names_for_connector_id(connector.id.as_str()) + .iter() + .cloned(), + ); + connector.plugin_display_names = merged.into_iter().collect(); } connectors }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/connectors.rs` around lines 432 - 441, with_app_plugin_sources currently replaces connector.plugin_display_names with the provenance list, dropping any existing names from ToolInfo; instead, fetch the provenance names via tool_plugin_provenance.plugin_display_names_for_connector_id(connector.id.as_str()), merge them with the existing connector.plugin_display_names (append or extend), and deduplicate the combined list (e.g., use a HashSet or preserve order and filter duplicates) before assigning back to connector.plugin_display_names so you perform a union rather than an overwrite.
305-307: Comment is out of sync with behavior.The note says no separate enrichment pass is required, but this file still performs enrichment passes (Line [136] and Line [222]-Line [223]). Updating the comment will avoid future confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/connectors.rs` around lines 305 - 307, The existing comment above the "let tools = mcp_tools.values().filter_map(|tool|" line incorrectly states that a separate enrichment pass is not required; update that comment to reflect current behavior: acknowledge that while ToolInfo contains plugin provenance, this module still performs enrichment passes (see the earlier and later enrichment logic operating on mcp_tools/ToolInfo), so the comment should not assert enrichment is unnecessary and should instead document that additional enrichment is applied within this file for completeness and downstream needs.codex-rs/core/src/codex.rs (2)
4648-4670: Avoid takingservices.rolloutlock twice during rollback preflight.Line 4648 and Line 4666 acquire separate snapshots of the recorder. Reusing one snapshot avoids lock churn and prevents split-brain behavior if recorder state changes between reads.
♻️ Suggested refactor
- let rollout_path = { - let recorder = { - let guard = sess.services.rollout.lock().await; - guard.clone() - }; - let Some(recorder) = recorder else { + let recorder = { + let guard = sess.services.rollout.lock().await; + guard.clone() + }; + let Some(recorder) = recorder else { sess.send_event_raw(Event { id: turn_context.sub_id.clone(), msg: EventMsg::Error(ErrorEvent { message: "thread rollback requires a persisted rollout path".to_string(), codex_error_info: Some(CodexErrorInfo::ThreadRollbackFailed), }), }) .await; return; - }; - recorder.rollout_path().to_path_buf() }; - if let Some(recorder) = { - let guard = sess.services.rollout.lock().await; - guard.clone() - } && let Err(err) = recorder.flush().await - { + let rollout_path = recorder.rollout_path().to_path_buf(); + if let Err(err) = recorder.flush().await { sess.send_event_raw(Event { id: turn_context.sub_id.clone(), msg: EventMsg::Error(ErrorEvent { message: format!( "failed to flush rollout `{}` for rollback replay: {err}", rollout_path.display() ), codex_error_info: Some(CodexErrorInfo::ThreadRollbackFailed), }), }) .await; return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/codex.rs` around lines 4648 - 4670, The code currently locks sess.services.rollout twice and clones two separate recorder snapshots; capture the recorder once, reuse it for both computing rollout_path and later flush to avoid lock churn and inconsistent state. Specifically, when you obtain recorder (the Option from sess.services.rollout.lock().await) use that single cloned recorder variable for calling recorder.rollout_path().to_path_buf() and later for recorder.flush().await, and remove the second lock/clone block; keep the early None branch that sends the ErrorEvent unchanged but ensure all subsequent uses reference the same recorder variable.
5130-5147: Consider reusing one MCP tools snapshot per turn.
run_turnfetchesmcp_toolsfor plugin injection, andbuilt_toolsfetches all tools again. Reusing a per-turn snapshot can reduce repeated manager reads on hot paths.Also applies to: 5877-5891
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/core/src/codex.rs` around lines 5130 - 5147, run_turn currently calls the MCP manager twice: once to populate mcp_tools and again inside built_tools; instead capture a single per-turn snapshot by calling sess.services.mcp_connection_manager.read().await.list_all_tools().or_cancel(&cancellation_token).await once (as you already do for mcp_tools) and stash that HashMap in a local variable (e.g., mcp_tools_snapshot) or attach it to turn_context, then pass that snapshot into any downstream logic that previously called list_all_tools() (notably the built_tools construction/function) so those callers reuse the snapshot instead of re-reading the manager; update references to mcp_tools/built_tools to accept the snapshot parameter and keep the same error handling semantics (return None or empty HashMap) used in the original mcp_tools branch.
codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json
Show resolved
Hide resolved
Comment on lines
+1790
to
+1802
| "properties": { | ||
| "_meta": true, | ||
| "message": { | ||
| "type": "string" | ||
| }, | ||
| "mode": { | ||
| "enum": [ | ||
| "form" | ||
| ], | ||
| "type": "string" | ||
| }, | ||
| "requested_schema": true | ||
| }, |
There was a problem hiding this comment.
Type requested_schema instead of allowing any JSON.
requested_schema: true removes structural validation on this path, while the rest of the elicitation surface is strongly typed. Reusing McpElicitationSchema here would keep behavior consistent.
Suggested schema tightening
- "requested_schema": true
+ "requested_schema": {
+ "$ref": "#/definitions/McpElicitationSchema"
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "properties": { | |
| "_meta": true, | |
| "message": { | |
| "type": "string" | |
| }, | |
| "mode": { | |
| "enum": [ | |
| "form" | |
| ], | |
| "type": "string" | |
| }, | |
| "requested_schema": true | |
| }, | |
| "properties": { | |
| "_meta": true, | |
| "message": { | |
| "type": "string" | |
| }, | |
| "mode": { | |
| "enum": [ | |
| "form" | |
| ], | |
| "type": "string" | |
| }, | |
| "requested_schema": { | |
| "$ref": "#/definitions/McpElicitationSchema" | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json`
around lines 1790 - 1802, The schema currently sets "requested_schema": true
which disables validation; replace this with a reference to the existing
McpElicitationSchema so the "requested_schema" property is strongly typed.
Update the object that defines "requested_schema" in the same JSON schema block
to use "$ref": "#/definitions/McpElicitationSchema" (or the repository’s
canonical ref name) and ensure the property type and requiredness match
McpElicitationSchema; keep the surrounding properties ("_meta", "message",
"mode") unchanged.
Comment on lines
+11800
to
+11810
| "properties": { | ||
| "appsNeedingAuth": { | ||
| "items": { | ||
| "$ref": "#/definitions/v2/AppSummary" | ||
| }, | ||
| "type": "array" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "appsNeedingAuth" | ||
| ], |
There was a problem hiding this comment.
PluginInstallResponse.appsNeedingAuth should be optional-with-default for rollout safety.
Making this new field required can break compatibility with older responders that omit it. A default empty array keeps behavior stable and still gives clients a consistent shape.
Backward-compatible response schema
"PluginInstallResponse": {
"$schema": "http://json-schema.org/draft-07/schema#",
"properties": {
"appsNeedingAuth": {
+ "default": [],
"items": {
"$ref": "#/definitions/v2/AppSummary"
},
"type": "array"
}
},
- "required": [
- "appsNeedingAuth"
- ],
"title": "PluginInstallResponse",
"type": "object"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "properties": { | |
| "appsNeedingAuth": { | |
| "items": { | |
| "$ref": "#/definitions/v2/AppSummary" | |
| }, | |
| "type": "array" | |
| } | |
| }, | |
| "required": [ | |
| "appsNeedingAuth" | |
| ], | |
| "properties": { | |
| "appsNeedingAuth": { | |
| "default": [], | |
| "items": { | |
| "$ref": "#/definitions/v2/AppSummary" | |
| }, | |
| "type": "array" | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json`
around lines 11800 - 11810, The new property
PluginInstallResponse.appsNeedingAuth is currently listed as required; change
the schema so it is optional and provide a default empty array to preserve
rollout safety: remove "appsNeedingAuth" from the "required" array (or make the
array empty) and add "default": [] to the appsNeedingAuth property definition
(the object under "properties" -> "appsNeedingAuth" referencing
"#/definitions/v2/AppSummary") so older responders that omit the field are
accepted and clients still receive a consistent array shape.
Comment on lines
+615
to
+674
| EventMsg::ElicitationRequest(request) => { | ||
| if matches!(api_version, ApiVersion::V2) { | ||
| let permission_guard = thread_watch_manager | ||
| .note_permission_requested(&conversation_id.to_string()) | ||
| .await; | ||
| let turn_id = match request.turn_id.clone() { | ||
| Some(turn_id) => Some(turn_id), | ||
| None => { | ||
| let state = thread_state.lock().await; | ||
| state.active_turn_snapshot().map(|turn| turn.id) | ||
| } | ||
| }; | ||
| let server_name = request.server_name.clone(); | ||
| let request_body = match request.request.try_into() { | ||
| Ok(request_body) => request_body, | ||
| Err(err) => { | ||
| error!( | ||
| error = %err, | ||
| server_name, | ||
| request_id = ?request.id, | ||
| "failed to parse typed MCP elicitation schema" | ||
| ); | ||
| if let Err(err) = conversation | ||
| .submit(Op::ResolveElicitation { | ||
| server_name: request.server_name, | ||
| request_id: request.id, | ||
| decision: codex_protocol::approvals::ElicitationAction::Cancel, | ||
| content: None, | ||
| meta: None, | ||
| }) | ||
| .await | ||
| { | ||
| error!("failed to submit ResolveElicitation: {err}"); | ||
| } | ||
| return; | ||
| } | ||
| }; | ||
| let params = McpServerElicitationRequestParams { | ||
| thread_id: conversation_id.to_string(), | ||
| turn_id, | ||
| server_name: request.server_name.clone(), | ||
| request: request_body, | ||
| }; | ||
| let (pending_request_id, rx) = outgoing | ||
| .send_request(ServerRequestPayload::McpServerElicitationRequest(params)) | ||
| .await; | ||
| tokio::spawn(async move { | ||
| on_mcp_server_elicitation_response( | ||
| request.server_name, | ||
| request.id, | ||
| pending_request_id, | ||
| rx, | ||
| conversation, | ||
| thread_state, | ||
| permission_guard, | ||
| ) | ||
| .await; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle non-v2 elicitation requests explicitly to avoid unresolved approvals.
Line 615 only processes elicitation for API v2. For other API versions, the request is dropped without submitting Op::ResolveElicitation, which can leave the turn waiting on a response.
💡 Suggested fix
EventMsg::ElicitationRequest(request) => {
if matches!(api_version, ApiVersion::V2) {
let permission_guard = thread_watch_manager
.note_permission_requested(&conversation_id.to_string())
.await;
...
tokio::spawn(async move {
on_mcp_server_elicitation_response(
request.server_name,
request.id,
pending_request_id,
rx,
conversation,
thread_state,
permission_guard,
)
.await;
});
+ } else {
+ error!(
+ server_name = %request.server_name,
+ request_id = ?request.id,
+ "mcp server elicitation is only supported on api v2"
+ );
+ if let Err(err) = conversation
+ .submit(Op::ResolveElicitation {
+ server_name: request.server_name,
+ request_id: request.id,
+ decision: codex_protocol::approvals::ElicitationAction::Cancel,
+ content: None,
+ meta: None,
+ })
+ .await
+ {
+ error!("failed to submit ResolveElicitation: {err}");
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/app-server/src/bespoke_event_handling.rs` around lines 615 - 674,
The ElicitationRequest arm only handles ApiVersion::V2 and silently drops other
versions, leaving turns waiting; update the EventMsg::ElicitationRequest branch
to explicitly handle non-V2 cases by submitting a ResolveElicitation (e.g.,
Op::ResolveElicitation with decision=ElicitationAction::Cancel) via
conversation.submit (same pattern used in the parse-error branch) when
api_version != ApiVersion::V2, and log any submission errors; ensure you
reference request.server_name and request.id when constructing the
ResolveElicitation so the approval is resolved for the correct request.
Comment on lines
219
to
+223
| if codex_apps_ready || !accessible_connectors.is_empty() { | ||
| write_cached_accessible_connectors(cache_key, &accessible_connectors); | ||
| } | ||
| let accessible_connectors = | ||
| with_app_plugin_sources(accessible_connectors, &tool_plugin_provenance); |
There was a problem hiding this comment.
Cache currently stores pre-enriched connectors, causing cached-only reads to miss plugin sources.
Line [220] writes accessible_connectors before Line [222]-Line [223] applies with_app_plugin_sources. This means cached-only consumers can get stale/empty plugin_display_names compared to the non-cached path.
Suggested fix (cache enriched connectors)
- let accessible_connectors =
- filter_disallowed_connectors(accessible_connectors_from_mcp_tools(&tools));
- if codex_apps_ready || !accessible_connectors.is_empty() {
- write_cached_accessible_connectors(cache_key, &accessible_connectors);
- }
- let accessible_connectors =
- with_app_plugin_sources(accessible_connectors, &tool_plugin_provenance);
+ let accessible_connectors = with_app_plugin_sources(
+ filter_disallowed_connectors(accessible_connectors_from_mcp_tools(&tools)),
+ &tool_plugin_provenance,
+ );
+ if codex_apps_ready || !accessible_connectors.is_empty() {
+ write_cached_accessible_connectors(cache_key, &accessible_connectors);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/core/src/connectors.rs` around lines 219 - 223, The current flow
writes raw accessible_connectors to cache before enriching them with
with_app_plugin_sources, so cached reads miss plugin_display_names; change the
order to first produce an enriched collection (call
with_app_plugin_sources(accessible_connectors, &tool_plugin_provenance) and
store that result in a local variable) and then call
write_cached_accessible_connectors(cache_key, &enriched_accessible_connectors)
so the cache contains the enriched connectors with plugin_display_names.
Comment on lines
+142
to
161
| fn discover_marketplace_paths_from_roots( | ||
| additional_roots: &[AbsolutePathBuf], | ||
| home_dir: Option<&Path>, | ||
| ) -> Vec<PathBuf> { | ||
| let mut paths = Vec::new(); | ||
| if let Some(repo_root) = get_git_repo_root(cwd) { | ||
| let path = repo_root.join(MARKETPLACE_RELATIVE_PATH); | ||
|
|
||
| if let Some(home) = home_dir { | ||
| let path = home.join(MARKETPLACE_RELATIVE_PATH); | ||
| if path.is_file() { | ||
| paths.push(path); | ||
| } | ||
| } | ||
|
|
||
| if let Some(home) = home_dir() { | ||
| let path = home.join(MARKETPLACE_RELATIVE_PATH); | ||
| if path.is_file() { | ||
| paths.push(path); | ||
| for root in additional_roots { | ||
| if let Some(repo_root) = get_git_repo_root(root.as_path()) { | ||
| let path = repo_root.join(MARKETPLACE_RELATIVE_PATH); | ||
| if path.is_file() && !paths.contains(&path) { | ||
| paths.push(path); | ||
| } | ||
| } |
There was a problem hiding this comment.
Additional roots outside Git repos are currently skipped.
discover_marketplace_paths_from_roots only searches when get_git_repo_root(...) returns Some(...). If an additional root is valid but not inside a Git repo, its .agents/plugins/marketplace.json is never considered.
Suggested fix
fn discover_marketplace_paths_from_roots(
additional_roots: &[AbsolutePathBuf],
home_dir: Option<&Path>,
) -> Vec<PathBuf> {
let mut paths = Vec::new();
@@
for root in additional_roots {
- if let Some(repo_root) = get_git_repo_root(root.as_path()) {
- let path = repo_root.join(MARKETPLACE_RELATIVE_PATH);
- if path.is_file() && !paths.contains(&path) {
- paths.push(path);
- }
- }
+ let discovery_root =
+ get_git_repo_root(root.as_path()).unwrap_or_else(|| root.as_path().to_path_buf());
+ let path = discovery_root.join(MARKETPLACE_RELATIVE_PATH);
+ if path.is_file() && !paths.contains(&path) {
+ paths.push(path);
+ }
}
paths
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/core/src/plugins/marketplace.rs` around lines 142 - 161, The
function discover_marketplace_paths_from_roots currently only checks
additional_roots when get_git_repo_root(root.as_path()) returns Some(...), so
roots that are valid directories but not inside a git repo are skipped; update
the loop in discover_marketplace_paths_from_roots to also check the marketplace
file directly on the provided root when get_git_repo_root returns None (i.e.,
compute root.join(MARKETPLACE_RELATIVE_PATH), test is_file(), and push if not
already in paths) while preserving the existing deduplication check
(paths.contains) and existing handling for the repo_root path.
| has_workspace_section, | ||
| "built realtime startup context" | ||
| ); | ||
| info!("realtime startup context: {context}"); |
There was a problem hiding this comment.
Do not log full startup context payload at info level.
This logs raw user asks and local path data into normal logs. Keep only metadata (size/section flags) and avoid payload content.
Suggested change
- info!("realtime startup context: {context}");
+ info!(
+ approx_tokens = approx_token_count(&context),
+ bytes = context.len(),
+ has_recent_work_section,
+ has_workspace_section,
+ "realtime startup context built"
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/core/src/realtime_context.rs` at line 96, The info log currently
emits the entire startup context via info!("realtime startup context:
{context}"); change it to only emit safe metadata (e.g., payload size, section
flags, and other non-sensitive fields) instead of full payload or user text:
locate the log site using the symbol/context variable in realtime_context.rs and
replace the message to compute and log only metadata attributes (size, section
flags, counts) while omitting or redacting any fields containing user asks, file
paths, or full payload content.
Comment on lines
+125
to
+135
| fn build_recent_work_section(cwd: &Path, recent_threads: &[ThreadMetadata]) -> Option<String> { | ||
| let mut groups: HashMap<PathBuf, Vec<&ThreadMetadata>> = HashMap::new(); | ||
| for entry in recent_threads { | ||
| let group = | ||
| resolve_root_git_project_for_trust(&entry.cwd).unwrap_or_else(|| entry.cwd.clone()); | ||
| groups.entry(group).or_default().push(entry); | ||
| } | ||
|
|
||
| let current_group = | ||
| resolve_root_git_project_for_trust(cwd).unwrap_or_else(|| cwd.to_path_buf()); | ||
| let mut groups = groups.into_iter().collect::<Vec<_>>(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
RT_FILE="$(fd 'realtime_context\.rs$' | head -n1)"
GI_FILE="$(fd 'git_info\.rs$' | head -n1)"
echo "== Calls to resolve_root_git_project_for_trust in realtime_context =="
rg -n 'resolve_root_git_project_for_trust\(' "$RT_FILE"
echo
echo "== Implementation of resolve_root_git_project_for_trust =="
rg -n -A25 -B3 'fn resolve_root_git_project_for_trust' "$GI_FILE"Repository: fcoury/oldcodex
Length of output: 5206
Reduce repeated synchronous git-root resolution in startup path.
build_recent_work_section calls resolve_root_git_project_for_trust once per thread entry (line 129–130 in the loop) and again for the current directory (line 134). Each call spawns a synchronous git rev-parse subprocess (git_info.rs:618–622). With large recent-thread sets, this can noticeably block startup.
Memoize results by directory to eliminate duplicate subprocess calls:
Suggested change
fn build_recent_work_section(cwd: &Path, recent_threads: &[ThreadMetadata]) -> Option<String> {
+ let mut root_cache: HashMap<PathBuf, PathBuf> = HashMap::new();
let mut groups: HashMap<PathBuf, Vec<&ThreadMetadata>> = HashMap::new();
for entry in recent_threads {
- let group =
- resolve_root_git_project_for_trust(&entry.cwd).unwrap_or_else(|| entry.cwd.clone());
+ let group = root_cache
+ .entry(entry.cwd.clone())
+ .or_insert_with(|| {
+ resolve_root_git_project_for_trust(&entry.cwd)
+ .unwrap_or_else(|| entry.cwd.clone())
+ })
+ .clone();
groups.entry(group).or_default().push(entry);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/core/src/realtime_context.rs` around lines 125 - 135,
build_recent_work_section currently calls resolve_root_git_project_for_trust
repeatedly (for each ThreadMetadata in recent_threads and again for cwd),
causing duplicate synchronous git subprocesses; add a small local memo/cache
(e.g., HashMap<PathBuf, PathBuf>) inside build_recent_work_section and use it to
store and reuse the resolved root for each directory key when processing
recent_threads and when computing current_group so
resolve_root_git_project_for_trust is invoked at most once per unique directory.
Comment on lines
+348
to
+376
| for entry in entries { | ||
| let Some(first_user_message) = entry.first_user_message.as_deref() else { | ||
| continue; | ||
| }; | ||
| let ask = first_user_message | ||
| .split_whitespace() | ||
| .collect::<Vec<_>>() | ||
| .join(" "); | ||
| let dedupe_key = format!("{}:{ask}", entry.cwd.display()); | ||
| if ask.is_empty() || !seen.insert(dedupe_key) { | ||
| continue; | ||
| } | ||
| let ask = if ask.chars().count() > MAX_ASK_CHARS { | ||
| format!( | ||
| "{}...", | ||
| ask.chars() | ||
| .take(MAX_ASK_CHARS.saturating_sub(3)) | ||
| .collect::<String>() | ||
| ) | ||
| } else { | ||
| ask | ||
| }; | ||
| lines.push(format!("- {}: {ask}", entry.cwd.display())); | ||
| if seen.len() == max_asks { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| (lines.len() > 5).then(|| lines.join("\n")) |
There was a problem hiding this comment.
Only emit thread groups when at least one ask is present.
Current lines.len() > 5 allows empty “User asks” sections when metadata lines are present.
Suggested change
- for entry in entries {
+ let mut ask_count = 0usize;
+ for entry in entries {
let Some(first_user_message) = entry.first_user_message.as_deref() else {
continue;
};
@@
lines.push(format!("- {}: {ask}", entry.cwd.display()));
+ ask_count += 1;
if seen.len() == max_asks {
break;
}
}
- (lines.len() > 5).then(|| lines.join("\n"))
+ (ask_count > 0).then(|| lines.join("\n"))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/core/src/realtime_context.rs` around lines 348 - 376, The code
currently returns the thread-group text based on (lines.len() > 5) which can be
true even when no user "ask" lines were added (only metadata), so change the
logic to only emit the group when at least one ask was appended: add an
ask_count (or reuse a boolean like saw_ask) that you increment/set inside the
loop when you push the ask line (the lines.push(format!("- {}: {ask}",
entry.cwd.display())) call), and then replace the final (lines.len() >
5).then(...) with a check like (ask_count > 0).then(|| lines.join("\n")) so the
function only returns when there is at least one actual ask; reference symbols:
entries, first_user_message, ask, lines, seen, max_asks, MAX_ASK_CHARS.
Comment on lines
+57
to
+66
| async fn save_image_generation_result_to_cwd( | ||
| cwd: &Path, | ||
| call_id: &str, | ||
| result: &str, | ||
| ) -> Result<PathBuf> { | ||
| let bytes = BASE64_STANDARD | ||
| .decode(result.trim().as_bytes()) | ||
| .map_err(|err| { | ||
| CodexErr::InvalidRequest(format!("invalid image generation payload: {err}")) | ||
| })?; |
There was a problem hiding this comment.
Bound decoded image size before base64 decode to prevent memory spikes.
Line 62 decodes unbounded input into memory. A large payload can trigger excessive allocation before any guardrail.
💡 Suggested fix
+const MAX_IMAGE_OUTPUT_BYTES: usize = 10 * 1024 * 1024;
+
async fn save_image_generation_result_to_cwd(
cwd: &Path,
call_id: &str,
result: &str,
) -> Result<PathBuf> {
+ let payload = result.trim();
+ let max_base64_len = ((MAX_IMAGE_OUTPUT_BYTES + 2) / 3) * 4;
+ if payload.len() > max_base64_len {
+ return Err(CodexErr::InvalidRequest(format!(
+ "image generation payload exceeds {} bytes",
+ MAX_IMAGE_OUTPUT_BYTES
+ )));
+ }
+
let bytes = BASE64_STANDARD
- .decode(result.trim().as_bytes())
+ .decode(payload.as_bytes())
.map_err(|err| {
CodexErr::InvalidRequest(format!("invalid image generation payload: {err}"))
})?;
+ if bytes.len() > MAX_IMAGE_OUTPUT_BYTES {
+ return Err(CodexErr::InvalidRequest(format!(
+ "image generation payload exceeds {} bytes",
+ MAX_IMAGE_OUTPUT_BYTES
+ )));
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/core/src/stream_events_utils.rs` around lines 57 - 66, The function
save_image_generation_result_to_cwd decodes unbounded base64 into memory
(BASE64_STANDARD.decode) which can spike memory; add a pre-check on the incoming
base64 string (result.trim()) and reject values that would decode to more than a
safe maximum: define a constant like MAX_IMAGE_BYTES (or MAX_BASE64_LEN) and
compute an estimated_decoded = trimmed_len * 3 / 4 (accounting for padding) and
return a CodexErr::InvalidRequest if estimated_decoded > MAX_IMAGE_BYTES before
calling BASE64_STANDARD.decode, then proceed with decoding and existing error
mapping.
4c3dacf to
90088ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json (1)
9473-9573:⚠️ Potential issue | 🟠 MajorEncode the documented
command/execpreconditions in the schema.The descriptions say the server rejects several combinations, but the schema still accepts them: empty
command,tty/streaming withoutprocessId, conflicting timeout/output-cap flags, andcommand/exec/writewith neither bytes norcloseStdin. That makes the new public contract looser than the implementation.Schema tightening
"CommandExecParams": { "$schema": "http://json-schema.org/draft-07/schema#", "description": "Run a standalone command (argv vector) in the server sandbox without creating a thread or turn.\n\nThe final `command/exec` response is deferred until the process exits and is sent only after all `command/exec/outputDelta` notifications for that connection have been emitted.", "properties": { "command": { "description": "Command argv vector. Empty arrays are rejected.", "items": { "type": "string" }, + "minItems": 1, "type": "array" }, ... }, + "allOf": [ + { + "if": { + "properties": { "tty": { "const": true } }, + "required": ["tty"] + }, + "then": { + "required": ["processId"], + "properties": { + "streamStdin": { "const": true }, + "streamStdoutStderr": { "const": true } + } + } + }, + { + "if": { + "properties": { "streamStdin": { "const": true } }, + "required": ["streamStdin"] + }, + "then": { "required": ["processId"] } + }, + { + "if": { + "properties": { "streamStdoutStderr": { "const": true } }, + "required": ["streamStdoutStderr"] + }, + "then": { "required": ["processId"] } + }, + { + "not": { + "required": ["disableTimeout", "timeoutMs"], + "properties": { "disableTimeout": { "const": true } } + } + }, + { + "not": { + "required": ["disableOutputCap", "outputBytesCap"], + "properties": { "disableOutputCap": { "const": true } } + } + } + ], "required": [ "command" ], "title": "CommandExecParams", "type": "object" }, "CommandExecWriteParams": { "$schema": "http://json-schema.org/draft-07/schema#", "description": "Write stdin bytes to a running `command/exec` session, close stdin, or both.", "properties": { ... }, + "anyOf": [ + { "required": ["deltaBase64"] }, + { + "properties": { "closeStdin": { "const": true } }, + "required": ["closeStdin"] + } + ], "required": [ "processId" ], "title": "CommandExecWriteParams", "type": "object" },Also applies to: 9674-9699
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json` around lines 9473 - 9573, The schema currently allows combinations the docs forbid; tighten the CommandExecParams and related definitions by: enforce non-empty "command" (array minItems:1 and disallow empty strings if intended), require "processId" when "tty" is true or when "streamStdin" or "streamStdoutStderr" are true (use "if"/"then" conditional subschemas referencing "tty","streamStdin","streamStdoutStderr" to require non-null "processId"), add mutual-exclusion constraints so "disableTimeout" cannot co-occur with "timeoutMs" and "disableOutputCap" cannot co-occur with "outputBytesCap" (use "oneOf"/"not"/"required" patterns or "dependentSchemas"), and for the command/exec/write schema ensure either "bytes" is present or "closeStdin" is true (use an "anyOf"/"oneOf" conditional that requires at least one). Reference these symbols/fields: CommandExecParams, command, processId, tty, streamStdin, streamStdoutStderr, disableTimeout, timeoutMs, disableOutputCap, outputBytesCap, and the command/exec/write request schema.codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs (1)
291-297:⚠️ Potential issue | 🟡 MinorFilter control frames in assert_no_message to avoid test failures on keepalive traffic.
The function currently rejects any frame that arrives within the timeout, including WebSocket control frames (
Ping/Pong). This causes tests to fail when the server sends keepalive pings. Mirror the filtering pattern fromread_jsonrpc_messageby looping until timeout while skippingPingandPongframes (responding toPingwithPongas already done elsewhere in the codebase).Suggested fix
pub(super) async fn assert_no_message(stream: &mut WsClient, wait_for: Duration) -> Result<()> { - match timeout(wait_for, stream.next()).await { - Ok(Some(Ok(frame))) => bail!("unexpected frame while waiting for silence: {frame:?}"), - Ok(Some(Err(err))) => bail!("unexpected websocket read error: {err}"), - Ok(None) => bail!("websocket closed unexpectedly while waiting for silence"), - Err(_) => Ok(()), - } + let deadline = Instant::now() + wait_for; + loop { + let remaining = deadline.saturating_duration_since(Instant::now()); + match timeout(remaining, stream.next()).await { + Err(_) => return Ok(()), + Ok(Some(Ok(WebSocketMessage::Ping(payload)))) => { + stream.send(WebSocketMessage::Pong(payload)).await?; + } + Ok(Some(Ok(WebSocketMessage::Pong(_) | WebSocketMessage::Frame(_)))) => {} + Ok(Some(Ok(frame))) => { + bail!("unexpected frame while waiting for silence: {frame:?}"); + } + Ok(Some(Err(err))) => bail!("unexpected websocket read error: {err}"), + Ok(None) => bail!("websocket closed unexpectedly while waiting for silence"), + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs` around lines 291 - 297, The assert_no_message function currently treats any incoming frame as a failure, causing test flakes when keepalive Ping/Pong frames arrive; modify assert_no_message to mirror read_jsonrpc_message by looping until the timeout and filtering control frames: on receiving a Ping, send back a Pong (using the same send path as elsewhere), ignore incoming Pong frames, continue the loop (i.e., don't bail) for Ping/Pong and only bail on non-control frames or errors; keep the timeout semantics so Err(_) still returns Ok(()) when silent.codex-rs/app-server/src/codex_message_processor.rs (1)
1574-1636:⚠️ Potential issue | 🟠 MajorBuild
command/execfrom the effective config before starting the proxy.
cwd, env policy, network proxy startup, and Windows sandbox level are all taken fromself.config, but the request’s sandbox override is only validated/applied later. That means acommand/execfor a different repo cwd or with a per-requestsandbox_policycan still run with the wrong network/sandbox setup. Please derive/reload the effective config first, then use that same config forcreate_env,start_proxy,WindowsSandboxLevel::from_config, and the fallback policy.Also applies to: 1659-1688
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/codex_message_processor.rs` around lines 1574 - 1636, Construct the effective per-request config (applying the request's sandbox overrides) before computing cwd, calling create_env, starting the network proxy, calling WindowsSandboxLevel::from_config, and deriving the fallback sandbox policy; then use that derived config everywhere those values are taken (replace uses of self.config with the effective_config when building env, calling spec.start_proxy, WindowsSandboxLevel::from_config, and when selecting the sandbox_policy fallback). Do the same fix for the second block around the other occurrence (the code near the 1659–1688 region) so both proxy startup and sandbox-level decisions use the same effective_config consistently.codex-rs/app-server-protocol/schema/json/EventMsg.json (1)
2024-2048:⚠️ Potential issue | 🟠 MajorKeep
elicitation_requestbackward-compatible.Both copies of
ElicitationRequestEventMsgreplace the required top-levelmessagefield with requiredrequest. That is a wire break for existing generated clients still deserializing the legacy event shape, which conflicts with the stated goal of preserving protocol compatibility. Please makerequestadditive or accept both shapes until the next protocol version.Also applies to: 7836-7860
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/EventMsg.json` around lines 2024 - 2048, The schema change replaced the legacy top-level "message" with a required "request", breaking wire compatibility; update ElicitationRequestEventMsg to accept either shape by adding an anyOf that requires at least one of "message" or "request" (e.g. anyOf: [{"required":["message"]},{"required":["request"]}]) and make "request" additive (remove it from the sole required list), then apply the same fix to the other copy referenced (the other ElicitationRequestEventMsg instance). Ensure the "request" property remains defined and keep "message" property present so both legacy and new clients can deserialize.codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json (1)
2494-2585:⚠️ Potential issue | 🟠 MajorEnforce the documented
command/execinvariants in the schema.This definition still accepts requests the server says it will reject, e.g.
{"command":[]},{"timeoutMs":-1}, or{"tty":true}without a client-visibleprocessId. That leaks basic contract validation to runtime and will mislead generated clients.Suggested schema constraints
"command": { "description": "Command argv vector. Empty arrays are rejected.", + "minItems": 1, "items": { "type": "string" }, "type": "array" }, @@ "timeoutMs": { "description": "Optional timeout in milliseconds.\n\nWhen omitted, the server default applies. Cannot be combined with `disableTimeout`.", "format": "int64", + "minimum": 0.0, "type": [ "integer", "null" ] }, @@ - "required": [ - "command" - ], + "required": [ + "command" + ], + "allOf": [ + { + "if": { + "properties": { + "tty": { "const": true } + } + }, + "then": { + "required": ["processId"], + "properties": { + "streamStdin": { "const": true }, + "streamStdoutStderr": { "const": true } + } + } + }, + { + "if": { + "properties": { + "streamStdin": { "const": true } + } + }, + "then": { "required": ["processId"] } + }, + { + "if": { + "properties": { + "streamStdoutStderr": { "const": true } + } + }, + "then": { "required": ["processId"] } + } + ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json` around lines 2494 - 2585, Add schema constraints so the JSON accepts only the documented requests: set "command" to require at least one element (add "minItems": 1), enforce non-negative timeouts by adding "minimum": 0 to "timeoutMs", and add conditional rules (JSON Schema if/then) that require "processId" when "tty" is true or when "streamStdin" or "streamStdoutStderr" are true; also enforce that "size" is only allowed when "tty" is true via a similar if/then, and add mutual-exclusion constraints (using "not" + "required" or "oneOf") to forbid combining "disableOutputCap" with "outputBytesCap" and to forbid combining "disableTimeout" with "timeoutMs".
♻️ Duplicate comments (4)
codex-rs/app-server/src/bespoke_event_handling.rs (1)
615-674:⚠️ Potential issue | 🟠 MajorResolve unsupported elicitation requests instead of dropping them.
For non-
ApiVersion::V2clients this branch currently does nothing, so the core-side elicitation never gets a matchingOp::ResolveElicitationand the turn can stay stuck waiting for approval.Suggested fix
EventMsg::ElicitationRequest(request) => { if matches!(api_version, ApiVersion::V2) { let permission_guard = thread_watch_manager .note_permission_requested(&conversation_id.to_string()) .await; let turn_id = match request.turn_id.clone() { Some(turn_id) => Some(turn_id), None => { let state = thread_state.lock().await; state.active_turn_snapshot().map(|turn| turn.id) } }; let server_name = request.server_name.clone(); let request_body = match request.request.try_into() { Ok(request_body) => request_body, Err(err) => { error!( error = %err, server_name, request_id = ?request.id, "failed to parse typed MCP elicitation schema" ); if let Err(err) = conversation .submit(Op::ResolveElicitation { server_name: request.server_name, request_id: request.id, decision: codex_protocol::approvals::ElicitationAction::Cancel, content: None, meta: None, }) .await { error!("failed to submit ResolveElicitation: {err}"); } return; } }; let params = McpServerElicitationRequestParams { thread_id: conversation_id.to_string(), turn_id, server_name: request.server_name.clone(), request: request_body, }; let (pending_request_id, rx) = outgoing .send_request(ServerRequestPayload::McpServerElicitationRequest(params)) .await; tokio::spawn(async move { on_mcp_server_elicitation_response( request.server_name, request.id, pending_request_id, rx, conversation, thread_state, permission_guard, ) .await; }); + } else { + error!( + server_name = %request.server_name, + request_id = ?request.id, + "mcp server elicitation is only supported on api v2" + ); + if let Err(err) = conversation + .submit(Op::ResolveElicitation { + server_name: request.server_name, + request_id: request.id, + decision: codex_protocol::approvals::ElicitationAction::Cancel, + content: None, + meta: None, + }) + .await + { + error!("failed to submit ResolveElicitation: {err}"); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/bespoke_event_handling.rs` around lines 615 - 674, The EventMsg::ElicitationRequest arm currently ignores non-ApiVersion::V2 clients causing core-side elicitation to never receive an Op::ResolveElicitation; update the else branch for the ApiVersion check in the EventMsg::ElicitationRequest handler to call conversation.submit(Op::ResolveElicitation { server_name: request.server_name.clone(), request_id: request.id, decision: codex_protocol::approvals::ElicitationAction::Cancel, content: None, meta: None }).await and log any submission error (matching the existing error handling used when request parsing fails) so unsupported elicitation requests are explicitly resolved instead of dropped.codex-rs/app-server/src/message_processor.rs (1)
337-340:⚠️ Potential issue | 🟠 MajorDon't log raw typed notifications.
This new in-process handler repeats the full-payload logging pattern already called out elsewhere. Please log only safe metadata or a constant message here.
🔒 Redaction-safe logging sketch
pub(crate) async fn process_client_notification(&self, notification: ClientNotification) { // Currently, we do not expect to receive any typed notifications from // in-process clients, so we just log them. - tracing::info!("<- typed notification: {:?}", notification); + let _ = notification; + tracing::debug!("<- typed notification received"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/message_processor.rs` around lines 337 - 340, The current process_client_notification implementation logs the entire ClientNotification payload via tracing::info! which exposes raw typed data; change it to avoid logging the full payload — replace the tracing::info! call in process_client_notification to emit only a constant message or a small set of redaction-safe metadata (e.g., notification type/variant name, id, or other non-sensitive fields) instead of the entire ClientNotification object, and remove or redact any sensitive fields before logging.codex-rs/app-server/src/in_process.rs (1)
252-268:⚠️ Potential issue | 🟠 MajorBound the shutdown enqueue wait.
Lines 256-259 can still block forever on a full bounded queue, so
shutdown()may never reach the timeout/abort logic on Lines 265-267. That breaks the bounded-shutdown guarantee in the exact wedged state this path needs to handle.Proposed fix
- if self - .client_tx - .send(InProcessClientMessage::Shutdown { done_tx }) - .await - .is_ok() - { + let shutdown_enqueued = matches!( + timeout( + SHUTDOWN_TIMEOUT, + self.client_tx.send(InProcessClientMessage::Shutdown { done_tx }), + ) + .await, + Ok(Ok(())) + ); + if shutdown_enqueued { let _ = timeout(SHUTDOWN_TIMEOUT, done_rx).await; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/in_process.rs` around lines 252 - 268, The shutdown method can block forever on awaiting self.client_tx.send(InProcessClientMessage::Shutdown { done_tx }) if the bounded channel is full; wrap that send future with the SHUTDOWN_TIMEOUT (e.g., timeout(SHUTDOWN_TIMEOUT, self.client_tx.send(...))).await and treat a timeout as a failed enqueue so execution continues to the runtime abort path: if the timed send returns Err(elapsed) or Err(send_error) skip waiting on done_rx and proceed to the runtime_handle timeout/abort logic (using the existing SHUTDOWN_TIMEOUT and runtime_handle symbols).codex-rs/app-server-client/src/lib.rs (1)
531-553:⚠️ Potential issue | 🟠 MajorKeep
shutdown()bounded and run cleanup before returning errors.
shutdown()can still fail its contract in two ways: the send on Lines 534-537 can wait forever on a full bounded queue, and the??on Line 546 can return before the join/abort cleanup runs. That makes the shutdown error path capable of leaking the worker task.Proposed fix
pub async fn shutdown(self) -> IoResult<()> { let mut worker_handle = self.worker_handle; let (response_tx, response_rx) = oneshot::channel(); - if self - .command_tx - .send(ClientCommand::Shutdown { response_tx }) - .await - .is_ok() - && let Ok(command_result) = timeout(SHUTDOWN_TIMEOUT, response_rx).await - { - command_result.map_err(|_| { - IoError::new( - ErrorKind::BrokenPipe, - "in-process app-server shutdown channel is closed", - ) - })??; - } + let mut shutdown_err = None; + let shutdown_enqueued = matches!( + timeout( + SHUTDOWN_TIMEOUT, + self.command_tx.send(ClientCommand::Shutdown { response_tx }), + ) + .await, + Ok(Ok(())) + ); + if shutdown_enqueued { + if let Ok(command_result) = timeout(SHUTDOWN_TIMEOUT, response_rx).await { + if let Err(err) = command_result + .map_err(|_| { + IoError::new( + ErrorKind::BrokenPipe, + "in-process app-server shutdown channel is closed", + ) + }) + .and_then(|inner| inner) + { + shutdown_err = Some(err); + } + } + } if let Err(_elapsed) = timeout(SHUTDOWN_TIMEOUT, &mut worker_handle).await { warn!("in-process app-server worker did not shut down within timeout; aborting"); worker_handle.abort(); let _ = worker_handle.await; } - Ok(()) + if let Some(err) = shutdown_err { + return Err(err); + } + Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-client/src/lib.rs` around lines 531 - 553, The shutdown() method can block on sending ClientCommand::Shutdown via command_tx and can return early from the command_result?? before cleaning up worker_handle; fix by making the send bounded (use try_send or send with timeout using the same SHUTDOWN_TIMEOUT/select) so it cannot wait forever, and restructure shutdown() so the worker_handle cleanup (the timeout+abort+await block that references worker_handle) always runs before any Err is returned — ensure you still await the response_rx with timeout (timeout(SHUTDOWN_TIMEOUT, response_rx)) but capture its result and only return an error after executing the worker_handle cleanup logic; reference command_tx, ClientCommand::Shutdown { response_tx }, response_rx, timeout, SHUTDOWN_TIMEOUT, and worker_handle.
🟡 Minor comments (4)
codex-rs/app-server-protocol/schema/json/v2/CommandExecWriteParams.json-3-23 (1)
3-23:⚠️ Potential issue | 🟡 MinorReject no-op
command/exec/writerequests.This schema currently accepts payloads like
{ "processId": "..." }or{ "processId": "...", "closeStdin": false }, even though the contract says the call should write bytes, close stdin, or both. Tighten validation so callers must provide a real action, or relax the description if no-op writes are intentional.🛠️ Suggested schema tightening
"required": [ "processId" ], + "anyOf": [ + { + "properties": { + "deltaBase64": { + "type": "string" + } + }, + "required": [ + "deltaBase64" + ] + }, + { + "properties": { + "closeStdin": { + "const": true + } + }, + "required": [ + "closeStdin" + ] + } + ], "title": "CommandExecWriteParams", "type": "object"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/v2/CommandExecWriteParams.json` around lines 3 - 23, The schema currently allows no-op objects (e.g., only "processId") so update CommandExecWriteParams.json to require an actual action: add a top-level oneOf (or anyOf) that enforces either (A) presence of "deltaBase64" as a non-empty string (use "type":"string" with "minLength":1 and disallow null) or (B) presence of "closeStdin" with a boolean value true (or both), while keeping "processId" required; alternatively, if no-op requests are intentional, revise the "description" to state that writes may be no-ops. Reference properties: "deltaBase64", "closeStdin", and "processId".codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs-229-244 (1)
229-244:⚠️ Potential issue | 🟡 MinorFail fast on duplicate responses too.
This helper already treats duplicate notifications as a protocol error, but a second response with the same id is still silently accepted and overwrites the first one. That can let routing tests pass against broken JSON-RPC behavior.
Possible fix
match message { JSONRPCMessage::Response(candidate) if candidate.id == target_id => { - response = Some(candidate); + if response.replace(candidate).is_some() { + bail!( + "received duplicate response for id `{id}` before completing paired read" + ); + } } JSONRPCMessage::Notification(candidate) if candidate.method == method => { if notification.replace(candidate).is_some() { bail!(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs` around lines 229 - 244, The loop that reads messages accepts a second Response with the same id silently; update the JSONRPCMessage::Response(target_id) branch to mirror the Notification handling: when a matching Response is seen, call response.replace(candidate) and if that returns Some(_) then bail with a protocol-error message like "received duplicate response for id `{target_id}` before completing paired read"; this uses the existing variables response, target_id and the JSONRPCMessage::Response pattern to locate and change the assignment to a replace+error path.codex-rs/app-server-protocol/schema/json/ClientRequest.json-150-159 (1)
150-159:⚠️ Potential issue | 🟡 MinorPublish the same
command/execconstraints the server already enforces.The schema still accepts empty
commandarrays, zerorows/cols, andcommand/exec/writepayloads that omit bothdeltaBase64andcloseStdin, butcodex-rs/app-server/src/command_exec.rsrejects all three at runtime. That drift means generated validators and docs allow requests that are guaranteed to fail on the wire. Please encode those invariants upstream as well (minItems: 1, positive terminal dimensions, and a write-requestanyOf) rather than only in server-side validation.Also applies to: 274-295, 309-331
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/ClientRequest.json` around lines 150 - 159, The JSON schema currently allows invalid command-exec requests that the server rejects at runtime; update the CommandExecParams and related definitions to match server invariants: add "minItems": 1 to the "command" array in CommandExecParams (and the other command-array schemas referenced at the other ranges), enforce positive terminal dimensions by setting "minimum": 1 for "rows" and "cols", and change the command/exec/write request schema to require at least one of "deltaBase64" or "closeStdin" (use an "anyOf" that ensures either deltaBase64 exists or closeStdin is true). Reference the symbols CommandExecParams, command, rows, cols, and the write request fields deltaBase64/closeStdin when making these edits so generated validators/docs match runtime checks.codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json-4605-4610 (1)
4605-4610:⚠️ Potential issue | 🟡 Minor
saved_pathstill will not round-trip through history.The live event and turn-item shapes now expose
saved_path, but the persisted/raw image-generation item variants in this same schema still omit it.thread/read/thread/resumewill drop the file path after reload, so clients cannot reliably reopen generated images later.Follow-up fields to add on the persisted/raw variants
@@ { "properties": { "id": { "type": "string" }, "result": { "type": "string" }, "revised_prompt": { "type": [ "string", "null" ] }, + "saved_path": { + "type": [ + "string", + "null" + ] + }, "status": { "type": "string" }, @@ { "properties": { "id": { "type": "string" }, "result": { "type": "string" }, "revisedPrompt": { "type": [ "string", "null" ] }, + "savedPath": { + "type": [ + "string", + "null" + ] + }, "status": { "type": "string" },Also applies to: 14628-14633
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json` around lines 4605 - 4610, The persisted/raw image-generation item variants in the JSON schema are missing the "saved_path" property so persisted thread/read and thread/resume lose the file path; update the relevant persisted/raw image-generation item schema objects to include "saved_path" with the same type union ["string","null"] as used by the live event and turn-item shapes (ensure the property name exactly matches "saved_path"), and mirror this change for the other affected persisted variants referenced (also add to the same variant schemas noted around lines 14628-14633).
🧹 Nitpick comments (9)
codex-rs/app-server-protocol/schema/json/v2/PluginListResponse.json (1)
72-76: Add URI validation to the URL fields.These properties are documented as URLs, but the schema currently accepts any string. Adding
"format": "uri"here would catch malformed marketplace metadata before it reaches UI/browser-launch code.Also applies to: 90-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/json/v2/PluginListResponse.json` around lines 72 - 76, The URL properties (e.g., privacyPolicyUrl) currently allow any string or null; add JSON Schema URI validation by adding "format": "uri" to those property definitions that use "type": ["string", "null"] so the schema enforces a well-formed URI (do the same for the other URL fields referenced around lines 90-100, such as termsOfServiceUrl/iconUrl or equivalent property names). Ensure the format is applied alongside the existing type array (i.e., keep ["string","null"] and add "format": "uri") for each URL property.codex-rs/app-server-protocol/schema/typescript/v2/CommandExecWriteParams.ts (1)
9-22: Require an actual write or close action.
{ processId },{ processId, closeStdin: false }, and{ processId, deltaBase64: null }all satisfy this type even though the contract says the request should write bytes, close stdin, or both. Please tighten the Rust protocol source to require at least one effective action, or reject empty writes server-side before regenerating this schema.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/schema/typescript/v2/CommandExecWriteParams.ts` around lines 9 - 22, The generated type CommandExecWriteParams allows no-op payloads; ensure the protocol requires at least one effective action by enforcing (a) deltaBase64 is non-null/non-empty OR (b) closeStdin is true. Fix this in the Rust protocol source that defines the command/exec write message (update validation to reject {processId} or {processId, deltaBase64: null} or {processId, closeStdin:false}), then regen the TypeScript schema so CommandExecWriteParams reflects the tightened contract (processId plus either a non-empty deltaBase64 or closeStdin=true).codex-rs/app-server/tests/suite/v2/thread_name_websocket.rs (2)
140-146: Reuse the existing websocket init helper.This is a copy of
initialize_both_clientsfromcodex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs, Lines 132-152. Making the shared helperpub(super)would keep the request-id and timeout setup in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/tests/suite/v2/thread_name_websocket.rs` around lines 140 - 146, Duplicate initialize_both_clients should be removed and the shared helper made accessible: change the original helper's visibility to pub(super) (the one that sends initialize requests and waits on read_response_for_id with DEFAULT_READ_TIMEOUT and specific request IDs), then import/use that helper from this test instead of re-defining it; ensure you keep using send_initialize_request, read_response_for_id and DEFAULT_READ_TIMEOUT so the timeout and request-id behavior remains centralized.
70-80: These assertions are lossy for "no extra messages".
read_response_and_notification_for_methodandread_notification_for_methoddiscard non-matching frames on the way to the expectedthread/name/updatedmessage (seecodex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs, Lines 205-253). That means the laterassert_no_messageonly checks what is left in the socket, so an unexpected earlier broadcast can be swallowed and the test still passes.Also applies to: 118-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/tests/suite/v2/thread_name_websocket.rs` around lines 70 - 80, The test currently uses read_response_and_notification_for_method and read_notification_for_method which discard non-matching frames, allowing earlier unexpected broadcasts to be swallowed and later assert_no_message to miss them; modify the test (or the helpers) so that reads do not drop unrelated frames: either change read_response_and_notification_for_method/read_notification_for_method to return all consumed frames (e.g., Vec of frames) or provide a non-destructive peek/drain helper and then assert that the only frames received are the expected thread/name/updated notifications and responses, using those returned frames to validate and then call assert_no_message to ensure no extra frames remain; refer to the helper functions read_response_and_notification_for_method, read_notification_for_method and assert_no_message when updating the logic.codex-rs/app-server/tests/suite/v2/plugin_list.rs (1)
288-288:workspace_defaultis created but not configured.The
TempDiris created and passed tocwdsbut has no.gitdirectory, marketplace, or config. If the intent is to test behavior with multiple cwds including an empty workspace, consider adding a brief comment. Otherwise, verify this doesn't cause unintended test behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/tests/suite/v2/plugin_list.rs` at line 288, The TempDir created as workspace_default (via TempDir::new()) is passed into cwds but left uninitialized (no .git, marketplace, or config), so either add a short comment explaining this empty-workspace case is intentional or initialize the temp dir before use: create a .git directory and any required marketplace/config files or populate it with the minimal fixtures your test expects; update the test around workspace_default and cwds to ensure the directory contents match the scenario being tested (or remove workspace_default if unused).codex-rs/app-server/src/lib.rs (1)
504-510: Verify error handling behavior for StateRuntime initialization.The initialization now uses
.ok()to silently discard errors, converting failures toNone. This meanslog_dbwill be unavailable without any warning if initialization fails.Consider logging a warning when initialization fails so operators can diagnose issues:
Suggested improvement
- let log_db = codex_state::StateRuntime::init( - config.sqlite_home.clone(), - config.model_provider_id.clone(), - ) - .await - .ok() - .map(log_db::start); + let log_db = match codex_state::StateRuntime::init( + config.sqlite_home.clone(), + config.model_provider_id.clone(), + ) + .await + { + Ok(runtime) => Some(log_db::start(runtime)), + Err(err) => { + warn!(error = %err, "Failed to initialize state runtime; log_db disabled"); + None + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server/src/lib.rs` around lines 504 - 510, The code silently converts StateRuntime::init failures to None by calling .ok(), losing the error; change this to explicitly handle the Result from codex_state::StateRuntime::init(config.sqlite_home.clone(), config.model_provider_id.clone()).await so that on Err you emit a warning with the error details (e.g., using tracing::warn! or log::warn!) and on Ok you call .map(log_db::start) or otherwise initialize log_db; specifically replace the .ok().map(...) chain with a match or .map_err(|e| { warn!("StateRuntime::init failed: {:?}", e); e }).ok().map(log_db::start) (or equivalent) so failures are logged while preserving the existing success path.codex-rs/app-server-protocol/src/protocol/v2.rs (3)
836-855: Consider wrapping Core types instead of exposing them directly.
AdditionalMacOsPermissionsdirectly usesCoreMacOsPreferencesPermissionandCoreMacOsAutomationPermissionfromcodex_protocol. This couples the v2 public API surface to internal Core type serialization, which could cause breaking changes if Core types evolve. Other patterns in this file wrap Core types with v2-specific enums/structs.If intentional for expedience, consider adding a comment explaining this design choice. Otherwise, consider creating v2 wrapper types with
Fromimplementations for stability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/src/protocol/v2.rs` around lines 836 - 855, AdditionalMacOsPermissions currently exposes CoreMacOsPreferencesPermission and CoreMacOsAutomationPermission directly, coupling the v2 API to core types; either create v2 wrapper types (e.g., V2MacOsPreferencesPermission and V2MacOsAutomationPermission) and implement From<CoreMacOs...> for each, then change AdditionalMacOsPermissions fields to use those wrappers and update the From<CoreMacOsSeatbeltProfileExtensions> implementation to convert into the wrappers, or if this exposure is intentional, add a clear inline comment above AdditionalMacOsPermissions documenting the decision and the risk of coupling to Core types for future maintainers.
1856-1877: Document or validate mutually exclusive options.
CommandExecParamsdocuments thatoutputBytesCapcannot be combined withdisableOutputCap, andtimeoutMscannot be combined withdisableTimeout. These constraints are only documented in comments and not enforced at the type level.Consider either:
- Using an enum to make invalid states unrepresentable
- Adding explicit validation in the request handler with clear error messages
- At minimum, ensure the server validates these constraints and returns informative errors
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/src/protocol/v2.rs` around lines 1856 - 1877, CommandExecParams documents mutually exclusive fields but doesn't enforce them; add a validation method (e.g., impl CommandExecParams { pub fn validate(&self) -> Result<(), ProtocolError> }) that checks (1) output_bytes_cap.is_some() && disable_output_cap == true and (2) timeout_ms.is_some() && disable_timeout == true and returns clear, specific errors naming the offending fields, then call this validate() from the request handler that constructs/processes CommandExecParams (e.g., in the command execution request handler such as handle_command_exec_request or the place that deserializes CommandExecParams) to reject invalid requests with an informative error; alternatively, if you prefer type-level safety, replace the pair (output_bytes_cap, disable_output_cap) and the pair (timeout_ms, disable_timeout) with enums making the mutually exclusive states unrepresentable.
4440-4448: Verify untagged enum variant ordering for correct deserialization.
McpElicitationPrimitiveSchemauses#[serde(untagged)]with variants ordered as:Enum,String,Number,Boolean. With untagged enums, serde tries variants in order and picks the first that deserializes successfully.The
Enumvariant should remain first since it has additional discriminating fields (enum,oneOf,items). However, ensure test coverage validates that:
- A plain
{"type": "string"}deserializes asString, notEnum- An enum with
{"type": "string", "enum": [...]}deserializes asEnumThe test at lines 5575-5669 appears to cover this, but verify edge cases are handled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codex-rs/app-server-protocol/src/protocol/v2.rs` around lines 4440 - 4448, McpElicitationPrimitiveSchema uses #[serde(untagged)] so variant order matters; keep Enum as the first variant and add/adjust unit tests to explicitly assert that deserializing {"type":"string"} yields McpElicitationPrimitiveSchema::String(...) while deserializing {"type":"string","enum":[...]} yields McpElicitationPrimitiveSchema::Enum(...). Update or add tests around the existing coverage (the tests that exercise primitive schema deserialization) to parse both JSON snippets, assert the enum variant via pattern matching, and fail if serde picks the wrong variant; if any flakiness remains, consider making the Enum discriminators more explicit in McpElicitationEnumSchema (fields like "enum"/"oneOf"/"items") to ensure deterministic matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec5b445b-00e8-495d-b4fd-57cf3367afe6
⛔ Files ignored due to path filters (1)
codex-rs/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (193)
.devcontainer/Dockerfile.github/workflows/rust-release.ymlcodex-rs/Cargo.tomlcodex-rs/app-server-client/Cargo.tomlcodex-rs/app-server-client/README.mdcodex-rs/app-server-client/src/lib.rscodex-rs/app-server-protocol/schema/json/ClientRequest.jsoncodex-rs/app-server-protocol/schema/json/EventMsg.jsoncodex-rs/app-server-protocol/schema/json/ServerNotification.jsoncodex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.jsoncodex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.jsoncodex-rs/app-server-protocol/schema/json/v2/CommandExecOutputDeltaNotification.jsoncodex-rs/app-server-protocol/schema/json/v2/CommandExecParams.jsoncodex-rs/app-server-protocol/schema/json/v2/CommandExecResizeParams.jsoncodex-rs/app-server-protocol/schema/json/v2/CommandExecResizeResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/CommandExecResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/CommandExecTerminateParams.jsoncodex-rs/app-server-protocol/schema/json/v2/CommandExecTerminateResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/CommandExecWriteParams.jsoncodex-rs/app-server-protocol/schema/json/v2/CommandExecWriteResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/ConfigReadResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/PluginListParams.jsoncodex-rs/app-server-protocol/schema/json/v2/PluginListResponse.jsoncodex-rs/app-server-protocol/schema/json/v2/RawResponseItemCompletedNotification.jsoncodex-rs/app-server-protocol/schema/json/v2/ThreadResumeParams.jsoncodex-rs/app-server-protocol/schema/json/v2/WindowsSandboxSetupStartParams.jsoncodex-rs/app-server-protocol/schema/typescript/ClientRequest.tscodex-rs/app-server-protocol/schema/typescript/ServerNotification.tscodex-rs/app-server-protocol/schema/typescript/WebSearchContextSize.tscodex-rs/app-server-protocol/schema/typescript/WebSearchLocation.tscodex-rs/app-server-protocol/schema/typescript/WebSearchToolConfig.tscodex-rs/app-server-protocol/schema/typescript/index.tscodex-rs/app-server-protocol/schema/typescript/v2/CommandExecOutputDeltaNotification.tscodex-rs/app-server-protocol/schema/typescript/v2/CommandExecOutputStream.tscodex-rs/app-server-protocol/schema/typescript/v2/CommandExecParams.tscodex-rs/app-server-protocol/schema/typescript/v2/CommandExecResizeParams.tscodex-rs/app-server-protocol/schema/typescript/v2/CommandExecResizeResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/CommandExecResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/CommandExecTerminalSize.tscodex-rs/app-server-protocol/schema/typescript/v2/CommandExecTerminateParams.tscodex-rs/app-server-protocol/schema/typescript/v2/CommandExecTerminateResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/CommandExecWriteParams.tscodex-rs/app-server-protocol/schema/typescript/v2/CommandExecWriteResponse.tscodex-rs/app-server-protocol/schema/typescript/v2/PluginInterface.tscodex-rs/app-server-protocol/schema/typescript/v2/PluginListParams.tscodex-rs/app-server-protocol/schema/typescript/v2/PluginMarketplaceEntry.tscodex-rs/app-server-protocol/schema/typescript/v2/PluginSource.tscodex-rs/app-server-protocol/schema/typescript/v2/PluginSummary.tscodex-rs/app-server-protocol/schema/typescript/v2/ProfileV2.tscodex-rs/app-server-protocol/schema/typescript/v2/ToolsV2.tscodex-rs/app-server-protocol/schema/typescript/v2/TurnStartParams.tscodex-rs/app-server-protocol/schema/typescript/v2/WindowsSandboxSetupStartParams.tscodex-rs/app-server-protocol/schema/typescript/v2/index.tscodex-rs/app-server-protocol/src/export.rscodex-rs/app-server-protocol/src/protocol/common.rscodex-rs/app-server-protocol/src/protocol/mappers.rscodex-rs/app-server-protocol/src/protocol/v1.rscodex-rs/app-server-protocol/src/protocol/v2.rscodex-rs/app-server-test-client/src/lib.rscodex-rs/app-server/Cargo.tomlcodex-rs/app-server/README.mdcodex-rs/app-server/src/app_server_tracing.rscodex-rs/app-server/src/bespoke_event_handling.rscodex-rs/app-server/src/bin/test_notify_capture.rscodex-rs/app-server/src/codex_message_processor.rscodex-rs/app-server/src/command_exec.rscodex-rs/app-server/src/config_api.rscodex-rs/app-server/src/in_process.rscodex-rs/app-server/src/lib.rscodex-rs/app-server/src/message_processor.rscodex-rs/app-server/src/outgoing_message.rscodex-rs/app-server/tests/common/mcp_process.rscodex-rs/app-server/tests/suite/v2/command_exec.rscodex-rs/app-server/tests/suite/v2/config_rpc.rscodex-rs/app-server/tests/suite/v2/connection_handling_websocket.rscodex-rs/app-server/tests/suite/v2/initialize.rscodex-rs/app-server/tests/suite/v2/mod.rscodex-rs/app-server/tests/suite/v2/plugin_install.rscodex-rs/app-server/tests/suite/v2/plugin_list.rscodex-rs/app-server/tests/suite/v2/thread_list.rscodex-rs/app-server/tests/suite/v2/thread_metadata_update.rscodex-rs/app-server/tests/suite/v2/thread_name_websocket.rscodex-rs/app-server/tests/suite/v2/thread_resume.rscodex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rscodex-rs/app-server/tests/suite/v2/windows_sandbox_setup.rscodex-rs/cli/Cargo.tomlcodex-rs/cli/src/login.rscodex-rs/cli/src/main.rscodex-rs/cli/tests/debug_clear_memories.rscodex-rs/config/src/config_requirements.rscodex-rs/core/config.schema.jsoncodex-rs/core/src/agent/control.rscodex-rs/core/src/apply_patch.rscodex-rs/core/src/client.rscodex-rs/core/src/client_common.rscodex-rs/core/src/codex.rscodex-rs/core/src/codex_delegate.rscodex-rs/core/src/codex_tests.rscodex-rs/core/src/codex_tests_guardian.rscodex-rs/core/src/compact.rscodex-rs/core/src/compact_remote.rscodex-rs/core/src/config/config_tests.rscodex-rs/core/src/config/managed_features.rscodex-rs/core/src/config/mod.rscodex-rs/core/src/config/network_proxy_spec.rscodex-rs/core/src/config/permissions.rscodex-rs/core/src/config/profile.rscodex-rs/core/src/context_manager/updates.rscodex-rs/core/src/exec.rscodex-rs/core/src/exec_policy.rscodex-rs/core/src/features.rscodex-rs/core/src/features/legacy.rscodex-rs/core/src/git_info.rscodex-rs/core/src/guardian.rscodex-rs/core/src/guardian_prompt.mdcodex-rs/core/src/guardian_tests.rscodex-rs/core/src/landlock.rscodex-rs/core/src/lib.rscodex-rs/core/src/mcp_tool_call.rscodex-rs/core/src/memories/mod.rscodex-rs/core/src/memories/phase1.rscodex-rs/core/src/memories/phase2.rscodex-rs/core/src/memories/start.rscodex-rs/core/src/memories/tests.rscodex-rs/core/src/memories/usage.rscodex-rs/core/src/memory_trace.rscodex-rs/core/src/mention_syntax.rscodex-rs/core/src/mentions.rscodex-rs/core/src/network_proxy_loader.rscodex-rs/core/src/path_utils.rscodex-rs/core/src/plugins/curated_repo.rscodex-rs/core/src/plugins/injection.rscodex-rs/core/src/plugins/manager.rscodex-rs/core/src/plugins/manifest.rscodex-rs/core/src/plugins/marketplace.rscodex-rs/core/src/plugins/mod.rscodex-rs/core/src/plugins/store.rscodex-rs/core/src/project_doc.rscodex-rs/core/src/realtime_conversation.rscodex-rs/core/src/rollout/list.rscodex-rs/core/src/rollout/metadata.rscodex-rs/core/src/rollout/recorder.rscodex-rs/core/src/rollout/tests.rscodex-rs/core/src/safety.rscodex-rs/core/src/sandboxing/mod.rscodex-rs/core/src/seatbelt.rscodex-rs/core/src/shell_snapshot.rscodex-rs/core/src/skills/injection.rscodex-rs/core/src/skills/invocation_utils.rscodex-rs/core/src/spawn.rscodex-rs/core/src/state/service.rscodex-rs/core/src/state_db.rscodex-rs/core/src/stream_events_utils.rscodex-rs/core/src/tasks/compact.rscodex-rs/core/src/tasks/mod.rscodex-rs/core/src/tasks/regular.rscodex-rs/core/src/tasks/review.rscodex-rs/core/src/tasks/undo.rscodex-rs/core/src/tasks/user_shell.rscodex-rs/core/src/tools/handlers/agent_jobs.rscodex-rs/core/src/tools/handlers/mcp.rscodex-rs/core/src/tools/handlers/multi_agents.rscodex-rs/core/src/tools/handlers/shell.rscodex-rs/core/src/tools/handlers/unified_exec.rscodex-rs/core/src/tools/js_repl/mod.rscodex-rs/core/src/tools/network_approval.rscodex-rs/core/src/tools/orchestrator.rscodex-rs/core/src/tools/registry.rscodex-rs/core/src/tools/runtimes/apply_patch.rscodex-rs/core/src/tools/runtimes/shell.rscodex-rs/core/src/tools/runtimes/shell/unix_escalation.rscodex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rscodex-rs/core/src/tools/runtimes/unified_exec.rscodex-rs/core/src/tools/sandboxing.rscodex-rs/core/src/tools/spec.rscodex-rs/core/src/turn_timing.rscodex-rs/core/src/unified_exec/mod.rscodex-rs/core/src/unified_exec/process.rscodex-rs/core/src/unified_exec/process_manager.rscodex-rs/core/tests/common/lib.rscodex-rs/core/tests/responses_headers.rscodex-rs/core/tests/suite/approvals.rscodex-rs/core/tests/suite/client.rscodex-rs/core/tests/suite/client_websockets.rscodex-rs/core/tests/suite/exec.rscodex-rs/core/tests/suite/memories.rscodex-rs/core/tests/suite/permissions_messages.rscodex-rs/core/tests/suite/plugins.rscodex-rs/core/tests/suite/realtime_conversation.rscodex-rs/core/tests/suite/request_permissions.rscodex-rs/core/tests/suite/rollout_list_find.rscodex-rs/core/tests/suite/skill_approval.rscodex-rs/core/tests/suite/user_shell_cmd.rs
✅ Files skipped from review due to trivial changes (5)
- codex-rs/app-server-protocol/src/protocol/v1.rs
- codex-rs/app-server-protocol/schema/typescript/WebSearchContextSize.ts
- codex-rs/app-server-protocol/schema/typescript/v2/TurnStartParams.ts
- codex-rs/app-server-protocol/schema/json/v2/CommandExecWriteResponse.json
- codex-rs/app-server-protocol/schema/typescript/v2/CommandExecResizeResponse.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- codex-rs/app-server-client/README.md
- codex-rs/app-server-protocol/schema/json/v2/PluginListParams.json
- codex-rs/Cargo.toml
- codex-rs/app-server-client/Cargo.toml
Comment on lines
2732
to
2734
| "action": { | ||
| "$ref": "#/definitions/v2/WebSearchAction" | ||
| "$ref": "#/definitions/v2/ResponsesApiWebSearchAction" | ||
| }, |
There was a problem hiding this comment.
Keep the existing web-search action wire shape on existing surfaces.
These refs now switch public payloads over to ResponsesApiWebSearchAction, which changes discriminator values from openPage / findInPage to open_page / find_in_page. That is a wire-format break for existing consumers of web_search_end, TurnItem.WebSearch, and web_search_call. Prefer keeping v2/WebSearchAction on these established schemas, or accepting both spellings during migration.
Also applies to: 8238-8240, 13191-13195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json`
around lines 2732 - 2734, The reference for the "action" property should keep
the existing wire shape by continuing to use v2/WebSearchAction (or accept both
discriminators) instead of switching to ResponsesApiWebSearchAction; update the
schema refs that currently point to
"#/definitions/v2/ResponsesApiWebSearchAction" (including the places around the
shown diff and the other instances at 8238-8240 and 13191-13195) to point to
"#/definitions/v2/WebSearchAction" or to a union/oneOf that accepts both
v2/WebSearchAction and v2/ResponsesApiWebSearchAction so existing consumers of
web_search_end, TurnItem.WebSearch, and web_search_call keep the original
discriminator values ("openPage"/"findInPage") while also allowing the new
snake_case variants during migration.
Comment on lines
+5201
to
+5301
| "ResponsesApiWebSearchAction": { | ||
| "oneOf": [ | ||
| { | ||
| "properties": { | ||
| "queries": { | ||
| "items": { | ||
| "type": "string" | ||
| }, | ||
| "type": [ | ||
| "array", | ||
| "null" | ||
| ] | ||
| }, | ||
| "query": { | ||
| "type": [ | ||
| "string", | ||
| "null" | ||
| ] | ||
| }, | ||
| "type": { | ||
| "enum": [ | ||
| "search" | ||
| ], | ||
| "title": "SearchResponsesApiWebSearchActionType", | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "type" | ||
| ], | ||
| "title": "SearchResponsesApiWebSearchAction", | ||
| "type": "object" | ||
| }, | ||
| { | ||
| "properties": { | ||
| "type": { | ||
| "enum": [ | ||
| "open_page" | ||
| ], | ||
| "title": "OpenPageResponsesApiWebSearchActionType", | ||
| "type": "string" | ||
| }, | ||
| "url": { | ||
| "type": [ | ||
| "string", | ||
| "null" | ||
| ] | ||
| } | ||
| }, | ||
| "required": [ | ||
| "type" | ||
| ], | ||
| "title": "OpenPageResponsesApiWebSearchAction", | ||
| "type": "object" | ||
| }, | ||
| { | ||
| "properties": { | ||
| "pattern": { | ||
| "type": [ | ||
| "string", | ||
| "null" | ||
| ] | ||
| }, | ||
| "type": { | ||
| "enum": [ | ||
| "find_in_page" | ||
| ], | ||
| "title": "FindInPageResponsesApiWebSearchActionType", | ||
| "type": "string" | ||
| }, | ||
| "url": { | ||
| "type": [ | ||
| "string", | ||
| "null" | ||
| ] | ||
| } | ||
| }, | ||
| "required": [ | ||
| "type" | ||
| ], | ||
| "title": "FindInPageResponsesApiWebSearchAction", | ||
| "type": "object" | ||
| }, | ||
| { | ||
| "properties": { | ||
| "type": { | ||
| "enum": [ | ||
| "other" | ||
| ], | ||
| "title": "OtherResponsesApiWebSearchActionType", | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "type" | ||
| ], | ||
| "title": "OtherResponsesApiWebSearchAction", | ||
| "type": "object" | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
Require action-specific fields on ResponsesApiWebSearchAction.
Every branch only requires type, so payloads like { "type": "open_page" } and { "type": "find_in_page" } now validate. That removes the schema guarantees generated clients need to render or replay these actions safely. open_page should require a non-null url, find_in_page should require at least pattern (and likely url), and search should require query or queries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/app-server-protocol/schema/json/EventMsg.json` around lines 5201 -
5301, The ResponsesApiWebSearchAction union currently only requires "type",
allowing invalid payloads; update each variant so action-specific fields are
required: in SearchResponsesApiWebSearchAction require either "query" or
"queries" (e.g. add a oneOf/anyOf requiring "query" OR "queries" and ensure
"queries" is an array of strings), in OpenPageResponsesApiWebSearchAction add
"url" to its required list and make its schema non-null string, and in
FindInPageResponsesApiWebSearchAction add "pattern" (and "url" if intended) to
required and make those non-null strings; adjust the corresponding titles
(SearchResponsesApiWebSearchAction, OpenPageResponsesApiWebSearchAction,
FindInPageResponsesApiWebSearchAction, OtherResponsesApiWebSearchAction) so
generated clients can rely on these fields.
Comment on lines
+7
to
+17
| "cols": { | ||
| "description": "Terminal width in character cells.", | ||
| "format": "uint16", | ||
| "minimum": 0.0, | ||
| "type": "integer" | ||
| }, | ||
| "rows": { | ||
| "description": "Terminal height in character cells.", | ||
| "format": "uint16", | ||
| "minimum": 0.0, | ||
| "type": "integer" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "CommandExecResizeParams.json" | head -20Repository: fcoury/oldcodex
Length of output: 133
🏁 Script executed:
find . -type f -path "*schema/json/v2*" | head -20Repository: fcoury/oldcodex
Length of output: 1607
🏁 Script executed:
# Check if the schema directory structure exists
fd -t f "\.json$" | grep -i "command\|resize" | head -20Repository: fcoury/oldcodex
Length of output: 1131
🌐 Web query:
JSON Schema Draft-07 format keyword numeric constraint enforcement
💡 Result:
In JSON Schema Draft-07, the format keyword is primarily an annotation and its validation (assertion) behavior is optional—a Draft-07–compliant validator is allowed to ignore format for pass/fail unless it explicitly chooses to implement it as an assertion. This is stated in the Draft-07 Validation spec: format “can … be implemented as assertions”, but that functionality is optional. [1] (Many test suites also treat format validation as optional.) [2]
What this means for numeric constraints
- Draft-07
formatis defined for specific kinds of instance values (most commonly strings, e.g.,date-time,email,uri). Draft-07 numeric validation is done with numeric constraint keywords, notformat. - To enforce numeric constraints, use:
type: "number"/type: "integer"minimum/exclusiveMinimummaximum/exclusiveMaximummultipleOf
These are assertion keywords in Draft-07 (i.e., they must be enforced by a conforming validator). [1]
Practical enforcement guidance
- If you rely on
formatfor validation, you must choose/configure a validator that enforces formats (and know which formats it supports). For example, Ajv documents howformatvalidation is handled and configured. [3]
Sources: Draft-07 Validation specification [1], Draft-07 format overview noting optional validation [2], Ajv format-validation documentation [3].
[1] (json-schema.org)
[2] (learnjsonschema.com)
[3] (ajv.js.org)
Citations:
- 1: https://json-schema.org/draft-07/draft-handrews-json-schema-validation-01?utm_source=openai
- 2: https://www.learnjsonschema.com/draft7/validation/format/?utm_source=openai
- 3: https://ajv.js.org/guide/formats?utm_source=openai
🌐 Web query:
JSON Schema uint16 format validation maximum constraint
💡 Result:
- JSON Schema has no built-in
uint16type or format. In standard JSON Schema,formatis generally annotation-only by default (validators may choose to enforce it, but they’re not required to). [2][3] uint16does exist in the OpenAPI Format Registry (“unsigned 16-bit integer”), but that’s OpenAPI-specific, not a JSON Schema core feature. [1]
How to validate a uint16 range (0…65535) in JSON Schema
Use numeric range keywords—not format:
{
"type": "integer",
"minimum": 0,
"maximum": 65535
}maximum constrains the instance value to be ≤ the keyword value. [4]
If you still want format: "uint16"
You can add it as an annotation (or rely on a validator that supports it as an extension):
{
"type": "integer",
"format": "uint16",
"minimum": 0,
"maximum": 65535
}But the range check must still come from minimum/maximum if you need portable, standards-based validation. [2][3][4]
Sources: OpenAPI Format Registry [1]; JSON Schema 2020-12 release notes on format behavior [2]; python-jsonschema docs noting format isn’t mandated to validate [3]; JSON Schema keyword maximum semantics [4].
[1] (spec.openapis.org)
[2] (json-schema.org)
[3] (python-jsonschema.readthedocs.io)
[4] (dev.fyicenter.com)
Citations:
- 1: https://spec.openapis.org/registry/format/?utm_source=openai
- 2: https://json-schema.org/draft/2020-12/release-notes?utm_source=openai
- 3: https://python-jsonschema.readthedocs.io/en/v4.13.0/validate/?utm_source=openai
- 4: https://dev.fyicenter.com/1000598_JSON_Schema_Validation_Keywords.html?utm_source=openai
🏁 Script executed:
cat -n ./codex-rs/app-server-protocol/schema/json/v2/CommandExecResizeParams.jsonRepository: fcoury/oldcodex
Length of output: 1611
🏁 Script executed:
# Search for CommandExecResizeParams in Rust code to find the backend type definition
rg -t rust "CommandExecResizeParams|ExecResizeParams" --max-count=20Repository: fcoury/oldcodex
Length of output: 1257
🏁 Script executed:
# Look for struct definitions with cols and rows fields
rg -t rust "struct.*Resize" -A 10 --max-count=10Repository: fcoury/oldcodex
Length of output: 1935
🏁 Script executed:
# Search for u16 usage in terminal/exec context
rg -t rust "cols|rows" -B 2 -A 2 --max-count=20Repository: fcoury/oldcodex
Length of output: 50372
🏁 Script executed:
rg -t rust "struct CommandExecTerminalSize" -A 15Repository: fcoury/oldcodex
Length of output: 1423
🏁 Script executed:
# Also check for serde attributes and deserialization behavior
rg -t rust "CommandExecTerminalSize" -B 2 -A 8 | head -50Repository: fcoury/oldcodex
Length of output: 4080
Add explicit maximum: 65535.0 bounds to cols and rows fields.
The schema uses format: "uint16" but JSON Schema Draft-07 treats format as optional annotation. Without explicit maximum, validators won't enforce the upper bound, and the schema accepts values above 65535 even though the Rust backend deserializes to u16. Add "maximum": 65535.0 to both fields to match the backend contract and ensure validation consistency.
Suggested schema tightening
"cols": {
"description": "Terminal width in character cells.",
"format": "uint16",
"minimum": 0.0,
+ "maximum": 65535.0,
"type": "integer"
},
"rows": {
"description": "Terminal height in character cells.",
"format": "uint16",
"minimum": 0.0,
+ "maximum": 65535.0,
"type": "integer"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/app-server-protocol/schema/json/v2/CommandExecResizeParams.json`
around lines 7 - 17, The cols and rows integer fields in
CommandExecResizeParams.json currently declare "format": "uint16" but lack
explicit upper bounds; add "maximum": 65535.0 to both the "cols" and "rows"
property objects so the JSON Schema enforces the u16 upper limit and matches the
Rust backend deserialization for those fields.
Comment on lines
+1
to
+5
| { | ||
| "$schema": "http://json-schema.org/draft-07/schema#", | ||
| "description": "Empty success response for `command/exec/resize`.", | ||
| "title": "CommandExecResizeResponse", | ||
| "type": "object" |
There was a problem hiding this comment.
Close this "empty" response schema.
As written, Line 5 still allows arbitrary object properties, so validators will accept non-empty payloads even though the contract says this is an empty success response. Please set additionalProperties to false.
Proposed fix
{
"$schema": "http://json-schema.org/draft-07/schema#",
+ "additionalProperties": false,
"description": "Empty success response for `command/exec/resize`.",
"title": "CommandExecResizeResponse",
"type": "object"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/app-server-protocol/schema/json/v2/CommandExecResizeResponse.json`
around lines 1 - 5, The schema for CommandExecResizeResponse currently defines
an object but allows arbitrary properties; update the JSON Schema
(CommandExecResizeResponse) to explicitly close the object by adding
"additionalProperties": false so validators will reject any non-empty payloads
and enforce the empty-response contract.
Comment on lines
+1
to
+5
| { | ||
| "$schema": "http://json-schema.org/draft-07/schema#", | ||
| "description": "Empty success response for `command/exec/terminate`.", | ||
| "title": "CommandExecTerminateResponse", | ||
| "type": "object" |
There was a problem hiding this comment.
Make the terminate response schema match its empty payload contract.
Line 5 currently permits any object shape, which is broader than both the description here and the generated TypeScript Record<string, never> response type. Add additionalProperties: false so the schema rejects stray fields.
Proposed fix
{
"$schema": "http://json-schema.org/draft-07/schema#",
+ "additionalProperties": false,
"description": "Empty success response for `command/exec/terminate`.",
"title": "CommandExecTerminateResponse",
"type": "object"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { | |
| "$schema": "http://json-schema.org/draft-07/schema#", | |
| "description": "Empty success response for `command/exec/terminate`.", | |
| "title": "CommandExecTerminateResponse", | |
| "type": "object" | |
| { | |
| "$schema": "http://json-schema.org/draft-07/schema#", | |
| "additionalProperties": false, | |
| "description": "Empty success response for `command/exec/terminate`.", | |
| "title": "CommandExecTerminateResponse", | |
| "type": "object" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@codex-rs/app-server-protocol/schema/json/v2/CommandExecTerminateResponse.json`
around lines 1 - 5, The JSON schema "CommandExecTerminateResponse" currently
allows arbitrary object properties though it represents an empty success
response; update the schema for CommandExecTerminateResponse to forbid stray
fields by adding additionalProperties: false to the object definition so the
generated TypeScript type Record<string, never> and runtime validation both
reflect an empty payload contract.
Comment on lines
+55
to
+56
| let resume_resp: JSONRPCResponse = read_response_for_id(&mut ws1, 10).await?; | ||
| let resume: ThreadResumeResponse = to_response::<ThreadResumeResponse>(resume_resp)?; |
There was a problem hiding this comment.
Bound these websocket reads with timeout(...).
read_response_for_id, read_notification_for_method, and read_response_and_notification_for_method keep polling until they see a matching frame (see codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs, Lines 190-253). Awaiting them directly here means a missing response or broadcast will hang the test instead of failing fast.
⏱️ Suggested fix
- let resume_resp: JSONRPCResponse = read_response_for_id(&mut ws1, 10).await?;
+ let resume_resp: JSONRPCResponse =
+ timeout(DEFAULT_READ_TIMEOUT, read_response_for_id(&mut ws1, 10)).await??;
@@
- let (rename_resp, ws1_notification) =
- read_response_and_notification_for_method(&mut ws1, 11, "thread/name/updated").await?;
+ let (rename_resp, ws1_notification) = timeout(
+ DEFAULT_READ_TIMEOUT,
+ read_response_and_notification_for_method(&mut ws1, 11, "thread/name/updated"),
+ )
+ .await??;
@@
- let ws2_notification =
- read_notification_for_method(&mut ws2, "thread/name/updated").await?;
+ let ws2_notification = timeout(
+ DEFAULT_READ_TIMEOUT,
+ read_notification_for_method(&mut ws2, "thread/name/updated"),
+ )
+ .await??;Also applies to: 70-77, 118-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codex-rs/app-server/tests/suite/v2/thread_name_websocket.rs` around lines 55
- 56, Wrap all awaits that call read_response_for_id,
read_notification_for_method, and read_response_and_notification_for_method with
a tokio::time::timeout to avoid hanging tests; replace direct awaits like let
resume_resp = read_response_for_id(&mut ws1, 10).await? with a timeout call and
handle the Result returned by timeout (map Err to a test failure by returning an
Err or panicking) so timeouts fail fast. Apply the same pattern to the other
usages in this file (the reads around the resume/notification handling and in
the ranges noted) and ensure the subsequent to_response::<ThreadResumeResponse>
(and similar) still runs only on the successful timed read.
d217370 to
1a2e174
Compare
fcoury
force-pushed
the
feat/tui-app-server
branch
from
d0ec0a3 to
89619ac
Compare
## Summary - start a code mode worker once per turn and let it pump nested tool calls through a dedicated queue - simplify code mode request/response dispatch around request ids and generic runner-unavailable errors - clean up the code mode process API and runner protocol plumbing ## Testing - not run yet
fcoury
force-pushed
the
feat/tui-app-server
branch
from
89619ac to
1bff842
Compare
## TL;DR Bring the Python app-server SDK from `main-with-prs-13953-and-14232` onto current `main` as a standalone SDK-only PR. - adds the new `sdk/python` and `sdk/python-runtime` package trees - keeps the scope to the SDK payload only, without the unrelated branch-history or workflow changes from the source branch - regenerates `sdk/python/src/codex_app_server/generated/v2_all.py` against current `main` schema so the extracted SDK matches today's protocol definitions ## Validation - `PYTHONPATH=sdk/python/src python3 -m pytest sdk/python/tests` Co-authored-by: Codex <noreply@openai.com>
## Summary - restore `use_linux_sandbox_bwrap` as a removed feature key so older `--enable` callers parse again - keep it as a no-op by leaving runtime behavior unchanged - add regression coverage for the legacy `--enable` path ## Testing - Not run (updated and pushed quickly)
fcoury
force-pushed
the
feat/tui-app-server
branch
from
1bff842 to
ea36017
Compare
## What changed - `codex-stdio-to-uds` now tolerates `NotConnected` when `shutdown(Write)` happens after the peer has already closed. - The socket test was rewritten to send stdin from a fixture file and to read an exact request payload length instead of waiting on EOF timing. ## Why this fixes the flake - This one exposed a real cross-platform runtime edge case: on macOS, the peer can close first after a successful exchange, and `shutdown(Write)` can report `NotConnected` even though the interaction already succeeded. - Treating that specific ordering as a harmless shutdown condition removes the production-level false failure. - The old test compounded the problem by depending on EOF timing, which varies by platform and scheduler. Exact-length IO makes the test deterministic and focused on the actual data exchange. ## Scope - Production logic change with matching test rewrite.
…ty (openai#14476) - **Summary** - migrate the code mode handler, service, worker, process, runner, and bridge assets into the `tools/code_mode` module tree - split Execution, protocol, and handler logic into dedicated files and relocate the tool definition into `code_mode/spec.rs` - update core references and tests to stitch the new organization together - **Testing** - Not run (not requested)
Bring over the TUI-side in-process app-server migration and its follow-on fixes while keeping the port aligned with current `main`. The TUI now uses the in-process client flow, replay and approval handling updates, and the associated snapshot and behavior coverage. Adapt the port to current branch APIs instead of reviving dirty-branch shared abstractions. This keeps `SessionSource` explicit, avoids reintroducing `ClientSurface`, and fills newer approval `skill_metadata` fields required on this branch.
Add end-to-end TUI support for `request_permissions` in the in-process app-server path. This wires the new approval request through the adapter, pending replay tracking, inactive-thread routing, and a subset-aware approval overlay. This closes the rebase gap from main where `item/permissions/requestApproval` reached the protocol before the TUI adapter handled the new request and response flow.
Seed a child session with the parent thread's granted turn permissions before submitting its first input. This preserves turn-scoped `request_permissions` behavior across fresh sub-agent spawns without turning the grant into a session-wide elevation.
Keep selected live child threads on the in-process app-server transport in fresh sessions so normal ops continue to flow through the same stream that delivers their events. This restores agent interactions after switching to a live background thread. Also restore shared `AuthManager` workspace overrides when the in-process app-server shuts down so temporary external-auth bridges do not leak session-specific ChatGPT workspace state into later thread-manager reuse.
Track current turn ids and pending interactive request cleanup per thread inside the in-process agent loop. Child-thread shutdown now unsubscribes only that thread instead of tearing down the shared app-server session. This prevents primary turn completion from invalidating child prompts, keeps sub-agent shutdown from killing the whole session, and avoids interrupts targeting a turn from the wrong thread after view switches.
Translate app-server child `thread/closed` notifications into the existing routed shutdown path and keep child turn bookkeeping scoped to the completed thread. This keeps the agent picker and selected-thread state in sync when app-server-managed child threads finish, and prevents stale child turn ids or pending prompts from surviving past completion.
Replace one-shot shared auth mutation with scoped external auth override guards so nested in-process clients restore only their own layer on shutdown. This keeps the latest active refresher and forced ChatGPT workspace bound to the shared `AuthManager` until the owning client exits, instead of reverting to stale state when an older client drops.
Keep forced ChatGPT workspace resolution layered when nested external auth overrides omit their own workspace id. This prevents a newer in-process client from temporarily clearing an inherited workspace pin while it is active, which keeps refreshes on the shared `AuthManager` bound to the expected workspace.
Advance to the next queued `request_user_input` prompt after interrupting the active one, instead of dismissing the entire overlay and stranding other threads behind it. Also render requested permissions from the original profile rather than expanded picker choices so macOS permission prompts only show the permission the server actually requested.
When switching away from an app-server-backed thread, remove the thread from the shared manager and submit `Op::Shutdown` directly to the removed thread. This avoids racing the in-process app-server loop against `remove_thread()`, which could leave the old thread running after a session switch because shutdown never reached the backing thread.
Treat primary `thread/closed` notifications like shutdown completion in the in-process agent loop. This makes externally closed primary sessions tear down through the normal shutdown-finalization path instead of leaving the old client alive on a dead transport.
Backfill in-process listener attachment for threads that already exist in a shared `ThreadManager` when the client initializes. This keeps preloaded or resumed threads observable through the in-process app-server path instead of only auto-subscribing future `thread_created` events.
Skip stale queued `request_user_input` prompts from the interrupted turn instead of advancing into requests that core has already cleared. Also let the permissions picker consume plain `o` for search in the permissions view so cross-thread shortcuts do not steal normal text input.
Capture the originating `thread_id` when interactive prompts are deferred behind the stream controller, and replay them against that stored thread instead of the currently selected widget thread. This keeps approvals and user-input prompts routed to the thread that originally emitted them after view switches, and updates the chatwidget tests to cover the real inbound event paths.
Route `request_user_input` confirmation cells through a thread-scoped app event instead of the global `InsertHistoryCell` path. This stores local confirmation cells with the originating thread so background-thread answers do not write into the visible transcript and still replay when switching back to that thread.
Only the primary live thread should reuse the in-process app-server op sender when switching views. This keeps selected secondary threads on the direct `CodexThread` path until their local response events are routed per thread, which avoids applying skill, prompt, and history UI updates to the primary thread by mistake.
Write exec and permission approval decisions through `InsertThreadHistoryCell` so inactive threads keep the same local confirmation history as the active thread. This preserves deny and partial-grant context after switching threads and updates the TUI tests to accept the thread-scoped history path.
…y in order Send `Op::Shutdown` directly to the primary core thread when the active session is app-server-backed, instead of routing that exit through thread-scoped app-server ops that only unsubscribe the thread stream. Replace the thread-history replay side channel with an ordered replay timeline so local-only history cells interleave with buffered events. This keeps rollback behavior aligned with transcript reconstruction during thread replay.
Use the live `AuthManager` for local ChatGPT token refreshes in app-server-backed sessions and route that path through the configured external auth refresher. This avoids returning stale cached tokens after unauthorized responses and keeps refresh state aligned with the shared thread manager. Treat refreshed ChatGPT account-id mismatches as hard failures during local refresh handling. This preserves the existing re-authentication behavior instead of silently switching a running session to a different account.
Make bottom-pane interrupt cleanup aware of the interrupted thread so turn-scoped approvals and user-input overlays from other live threads stay queued. The active thread still clears its own turn-scoped views, while unscoped views keep the historical interrupt behavior. Thread-owned bottom-pane overlays now report their `thread_id`, and the interrupt path filters on that ownership when dismissing views. This prevents one thread interruption from silently removing actionable prompts belonging to another thread.
Carry the original event `id` through legacy notification decoding when app-server includes one in the wrapper payload. TUI replay and deduplication logic uses non-empty event ids for user-message handling, so dropping them can reintroduce duplicate transcript entries. Keep the existing empty-string fallback for legacy payloads that do not include an event id, and cover both shapes in decoder tests.
Drop the leftover inherited-next-turn permission hooks that no longer exist in `SessionState`, and reconcile the TUI approval code with the upstream permissions picker shape after the rebase. Restore `MessageProcessorArgs.thread_manager` so the in-process `app-server` still reuses the shared `ThreadManager` expected by the TUI and `app-server-client`, keeping the rebased branch compiling without reviving the old branch-specific permissions path.
Adjust the in-process app-server tests to use the current `ThreadManager::new` signature after the rebase cleanup. This keeps the shared-manager test coverage compiling without reviving older constructor arguments that `core` no longer accepts.
Deduplicate in-process permission approvals by `(thread_id, call_id)` and keep the permissions overlay behavior aligned with the current TUI flow, including updated replay and test coverage for the `scope` field. Route shared in-process auth overrides through the existing `AuthManager` guard stack so nested clients unwind correctly, and drop an unused TUI test helper to keep the crate warning-free.
fcoury
force-pushed
the
feat/tui-app-server
branch
from
ea36017 to
f5664bc
Compare
Update the rebased test helpers to match the current `MessageProcessorArgs` and `ThreadManager` interfaces after the latest upstream changes. Tighten the app-server tracing wait predicate so it waits for spans from the request trace, and keep `Cargo.lock` in sync with the rebased crate metadata used by this branch.
Format the rebased app-server tracing test the same way CI's `cargo fmt` expects so the format job passes again. Broaden the macOS seatbelt denial matcher to accept the current bash stderr variant that prefixes denied writes with `line 1:` while keeping the existing sandbox-apply fallback.
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.
In-process app-server for TUI and exec
Problem
This PR routes
codex-tuiandcodex-execthrough app-server in-process so all conversational surfaces share one runtime and one protocol mapping layer. Before this integration, those surfaces mostly used core directly (ThreadManager/CodexThread) and carried parallel lifecycle/routing logic.The in-process path keeps request/response/event flow typed over bounded
mpscchannels, avoiding JSON round-trips inside a single process while preserving the JSON-RPC surface at transport boundaries.Mental model
The app-server runtime is embedded via
codex-app-server-client.codex-tuiandcodex-execdepend directly oncodex-app-server-client(andcodex-app-server-protocol);codex-app-server-clientdepends oncodex-app-server.Communication uses bounded
mpscchannels carrying native Rust types. JSON serialization/deserialization is only used at external boundaries (stdio,websocket).The layering is:
The network server path (
run_main_with_transport) remains in place. The in-process path is a parallel entry point that uses a syntheticConnectionId(0)and the sameroute_outgoing_envelopebroadcast/targeted-routing logic as transport-backed connections.Non-goals
stdioremains a primary integration path; websocket remains experimental/unsupported.InProcessClientHandleowns exactly one synthetic connection (ConnectionId(0)).Tradeoffs
CoreEventvariant inOutgoingMessageinstead of transport-ready pre-serialized notificationsInProcessAppServerClientwrappingInProcessClientHandle)Send-safe facade; automatic lag recovery; auto-reject of undeliverableServerRequestsAgentCommandintent layer in TUIOp; centralizes approval dispatchmemory/dropalongsidethread/memories/drop)Architecture
New crates
codex-app-server-client— Public facade for in-process app-server embedding. ProvidesInProcessAppServerClientwithrequest,request_typed,notify,resolve_server_request,reject_server_request,next_event, andshutdown. Manages a worker task that decouples the caller from runtime internals, tracks dropped events, and emitsLaggedevents when the consumer falls behind.New modules
app-server/src/in_process.rs— Low-level runtime. Spawns a Tokio task containing theMessageProcessorselect loop. Uses channel pairs for client->runtime commands, runtime->client events, and outgoing envelope routing (shared with transport paths viaroute_outgoing_envelope). Request/response correlation usesHashMap<RequestId, oneshot::Sender>.app-server/src/thread_state.rs—ThreadStateManagerand per-threadThreadState. Tracks connection subscriptions, listener generations, turn history builders, and pending interrupt/rollback queues across runtime paths.tui/src/agent_command.rs—AgentCommandenum. Intent-level actions (Interrupt,ExecApproval,PatchApproval,ResolveElicitation) that convert to protocol/core operations.Protocol additions (v2)
Canonical RPCs:
thread/undo,thread/context/update,thread/config/reload,thread/shutdown,thread/memories/drop,thread/memories/update,thread/userShellCommand/run,history/append,history/entry/read,prompt/custom/list,mcp/tools/list.Compatibility aliases (deprecated, still accepted):
thread/turnContext/set,memory/drop,memory/update,thread/shellCommand/run,history/add.Server→client request:
item/mcpToolCall/requestElicitation(MCP elicitation flow).Notifications:
warning,backgroundEvent,turn/streamError,thread/undo/started,thread/undo/completed,thread/shutdown/completed,mcpServer/startup/updated,mcpServer/startup/completed,skills/updated.Enriched thread bootstrap responses:
ThreadStartResponse,ThreadResumeResponse, andThreadForkResponseincludehistory_log_id,history_entry_count,forked_from_thread_id, andnetwork_proxy.Outgoing message refactor
OutgoingMessagecarries typed core events viaOutgoingMessage::CoreEvent { thread_id, event }and keeps app-server-native notifications inOutgoingMessage::AppServerNotification.send_core_event_to_connectionshandles core-event fanout, andlegacy_event_methodis used where transport writers need wire method names.Tracing
Codex::submitcaptures W3C trace context (traceparent/tracestate) into eachSubmission. The submission dispatch loop re-parents spans from this context sorun_turnand deeper core operations appear under the originating app-server request in distributed traces.Observability
tracing::debug!logs on thread listener retention, teardown, and connection disconnect withthread_id,connection_id, andlistener_generationfields.tracing::warn!on dropped in-process events (queue full), unmatched response IDs, and failed envelope sends.tracing::trace!on every outgoing notification with targeted connection count.Tests
in_process.rsunit tests — Initialize/typed-request round-trip, session source mapping, zero-capacity channel clamping.app-server-clientunit tests — Typed request round-trip, JSON-RPC error reporting, surface->session-source mapping, tiny channel capacity behavior.archive_thread,codex_message_processor_flow,config,create_thread,fork_thread,interrupt,list_resume,login,output_schema,send_message,set_default_model,user_agent,user_info.Summary by CodeRabbit
Release Notes
New Features
Improvements
Platform Support