openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-29 19:08:43 +00:00
Code Issues Packages Projects Releases Wiki Activity

Remove detached delivery from review/start (#57667)

Browse Source 
## What changed

`review/start` now supports only inline reviews on the supplied thread and returns that thread's id as `reviewThreadId`. Clients can omit `delivery`, send `null`, or specify `"inline"`; `"detached"` is no longer supported.

For a separate review thread, call `thread/start` followed by `review/start`. To preserve conversation history with custom review instructions, use `thread/fork` followed by `turn/start`.

Remove detached review execution and its app-server dependency on `codex-agent-extension`, and update the protocol schemas and generated clients. Preserve `ReviewDelivery.inline` as a Python enum member.

## Testing

Add a Python regression test verifying that inline review delivery parses as `ReviewDelivery.inline` and round-trips to the original JSON.

GitOrigin-RevId: ed16c5271c3315ae3afcb3ee78883ed421394b4d
This commit is contained in:
yansenzhou-oai
2026-10-28 07:12:32 +00:00
committed by copyberry
parent 154e8736a5
commit 26ad281fd1
26 changed files with 90 additions and 570 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/Cargo.lock
Generated
-1
View File
@@ -2065,7 +2065,6 @@ dependencies = [
"base64 0.22.1",
"chrono",
"clap",
"codex-agent-extension",
"codex-analytics",
"codex-app-server-daemon",
"codex-app-server-protocol",
codex-rs/Cargo.toml
-1
View File
@@ -217,7 +217,6 @@ codex-websocket-auth = { path = "websocket-auth" }
codex-websocket-client = { path = "websocket-client" }
codex-config = { path = "config" }
codex-connectors = { path = "connectors" }
codex-agent-extension = { path = "ext/agent" }
codex-agent-message-board-client = { path = "agent-message-board-client" }
codex-agent-message-board-extension = { path = "ext/agent-message-board" }
codex-message-board = { path = "message-board" }
codex-rs/app-server-protocol/schema/json/ClientRequest.json
Generated
+2 -3
View File
@@ -3960,8 +3960,7 @@
},
"ReviewDelivery": {
"enum": [
"inline",
"detached"
"inline"
],
"type": "string"
},
@@ -3977,7 +3976,7 @@
}
],
"default": null,
"description": "Where to run the review: inline (default) on the current thread or detached on a new thread (returned in `reviewThreadId`). Detached delivery is deprecated and emits `deprecationNotice`. Use `thread/start` followed by an inline review for a separate review thread."
"description": "Reviews run inline on the supplied thread. Omit this field or pass `inline`. To review on a separate thread, start a thread first and use its id."
},
"target": {
"$ref": "#/definitions/ReviewTarget"
codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json
Generated
+3 -4
View File
@@ -20337,8 +20337,7 @@
},
"ReviewDelivery": {
"enum": [
"inline",
"detached"
"inline"
],
"type": "string"
},
@@ -20355,7 +20354,7 @@
}
],
"default": null,
"description": "Where to run the review: inline (default) on the current thread or detached on a new thread (returned in `reviewThreadId`). Detached delivery is deprecated and emits `deprecationNotice`. Use `thread/start` followed by an inline review for a separate review thread."
"description": "Reviews run inline on the supplied thread. Omit this field or pass `inline`. To review on a separate thread, start a thread first and use its id."
},
"target": {
"$ref": "#/definitions/v2/ReviewTarget"
@@ -20375,7 +20374,7 @@
"$schema": "http://json-schema.org/draft-07/schema#",
"properties": {
"reviewThreadId": {
"description": "Identifies the thread where the review runs.\n\nFor inline reviews, this is the original thread id. For detached reviews, this is the id of the new review thread.",
"description": "The thread id supplied to `review/start`.",
"type": "string"
},
"turn": {
codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json
Generated
+3 -4
View File
@@ -16329,8 +16329,7 @@
},
"ReviewDelivery": {
"enum": [
"inline",
"detached"
"inline"
],
"type": "string"
},
@@ -16347,7 +16346,7 @@
}
],
"default": null,
"description": "Where to run the review: inline (default) on the current thread or detached on a new thread (returned in `reviewThreadId`). Detached delivery is deprecated and emits `deprecationNotice`. Use `thread/start` followed by an inline review for a separate review thread."
"description": "Reviews run inline on the supplied thread. Omit this field or pass `inline`. To review on a separate thread, start a thread first and use its id."
},
"target": {
"$ref": "#/definitions/ReviewTarget"
@@ -16367,7 +16366,7 @@
"$schema": "http://json-schema.org/draft-07/schema#",
"properties": {
"reviewThreadId": {
"description": "Identifies the thread where the review runs.\n\nFor inline reviews, this is the original thread id. For detached reviews, this is the id of the new review thread.",
"description": "The thread id supplied to `review/start`.",
"type": "string"
},
"turn": {
codex-rs/app-server-protocol/schema/json/v2/ReviewStartParams.json
Generated
+2 -3
View File
@@ -3,8 +3,7 @@
"definitions": {
"ReviewDelivery": {
"enum": [
"inline",
"detached"
"inline"
],
"type": "string"
},
@@ -111,7 +110,7 @@
}
],
"default": null,
"description": "Where to run the review: inline (default) on the current thread or detached on a new thread (returned in `reviewThreadId`). Detached delivery is deprecated and emits `deprecationNotice`. Use `thread/start` followed by an inline review for a separate review thread."
"description": "Reviews run inline on the supplied thread. Omit this field or pass `inline`. To review on a separate thread, start a thread first and use its id."
},
"target": {
"$ref": "#/definitions/ReviewTarget"
codex-rs/app-server-protocol/schema/json/v2/ReviewStartResponse.json
Generated
+1 -1
View File
@@ -2393,7 +2393,7 @@
},
"properties": {
"reviewThreadId": {
"description": "Identifies the thread where the review runs.\n\nFor inline reviews, this is the original thread id. For detached reviews, this is the id of the new review thread.",
"description": "The thread id supplied to `review/start`.",
"type": "string"
},
"turn": {
codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst
Generated
BIN
View File
Binary file not shown.
codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst
Generated
BIN
View File
Binary file not shown.
codex-rs/app-server-protocol/schema/typescript/v2/ReviewDelivery.ts
Generated
+1 -1
View File
@@ -2,4 +2,4 @@
// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually.
export type ReviewDelivery = "inline" | "detached";
export type ReviewDelivery = "inline";
codex-rs/app-server-protocol/schema/typescript/v2/ReviewStartParams.ts
Generated
+2 -4
View File
@@ -6,9 +6,7 @@ import type { ReviewTarget } from "./ReviewTarget";
export type ReviewStartParams = { threadId: string, target: ReviewTarget,
/**
* Where to run the review: inline (default) on the current thread or
* detached on a new thread (returned in `reviewThreadId`).
* Detached delivery is deprecated and emits `deprecationNotice`.
* Use `thread/start` followed by an inline review for a separate review thread.
* Reviews run inline on the supplied thread. Omit this field or pass `inline`.
* To review on a separate thread, start a thread first and use its id.
*/
delivery?: ReviewDelivery | null, };
codex-rs/app-server-protocol/schema/typescript/v2/ReviewStartResponse.ts
Generated
+1 -4
View File
@@ -5,9 +5,6 @@ import type { Turn } from "./Turn";
export type ReviewStartResponse = { turn: Turn,
/**
* Identifies the thread where the review runs.
*
* For inline reviews, this is the original thread id.
* For detached reviews, this is the id of the new review thread.
* The thread id supplied to `review/start`.
*/
reviewThreadId: string, };
codex-rs/app-server-protocol/src/protocol/v2/review.rs
+9 -14
View File
@@ -1,15 +1,15 @@
use super::Turn;
use super::shared::v2_enum_from_core;
use crate::JsonSchema;
use crate::TS;
use serde::Deserialize;
use serde::Serialize;
v2_enum_from_core!(
pub enum ReviewDelivery from codex_protocol::protocol::ReviewDelivery {
Inline, Detached
}
);
#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)]
#[serde(rename_all = "camelCase")]
#[ts(export_to = "v2/")]
pub enum ReviewDelivery {
Inline,
}
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
#[serde(rename_all = "camelCase")]
@@ -18,10 +18,8 @@ pub struct ReviewStartParams {
pub thread_id: String,
pub target: ReviewTarget,
/// Where to run the review: inline (default) on the current thread or
/// detached on a new thread (returned in `reviewThreadId`).
/// Detached delivery is deprecated and emits `deprecationNotice`.
/// Use `thread/start` followed by an inline review for a separate review thread.
/// Reviews run inline on the supplied thread. Omit this field or pass `inline`.
/// To review on a separate thread, start a thread first and use its id.
#[serde(default)]
#[ts(optional = nullable)]
pub delivery: Option<ReviewDelivery>,
@@ -32,10 +30,7 @@ pub struct ReviewStartParams {
#[ts(export_to = "v2/")]
pub struct ReviewStartResponse {
pub turn: Turn,
/// Identifies the thread where the review runs.
///
/// For inline reviews, this is the original thread id.
/// For detached reviews, this is the id of the new review thread.
/// The thread id supplied to `review/start`.
pub review_thread_id: String,
}
codex-rs/app-server/Cargo.toml
-1
View File
@@ -36,7 +36,6 @@ axum = { workspace = true, default-features = false, features = [
] }
codex-analytics = { workspace = true }
codex-app-server-daemon = { workspace = true }
codex-agent-extension = { workspace = true }
codex-arg0 = { workspace = true }
codex-aws-auth = { workspace = true }
codex-cloud-config = { workspace = true }
codex-rs/app-server/README.md
+8
View File
@@ -179,6 +179,14 @@ discovery finds no project-root marker, Git checkout, or project-local `.codex`
directory. Starting a task there does not preapprove project configuration added
later. Existing trust decisions and permission checks for projects are unchanged.
# Review delivery
`review/start` runs the review on the supplied thread and returns that same
`reviewThreadId`. Clients can omit `delivery`, send `null`, or specify `"inline"`.
Clients that used `"detached"` should call `thread/start` and pass the new thread id
to `review/start`. To keep the original conversation history with custom review
instructions, use `thread/fork` followed by `turn/start`.
# Thread removal
`thread/archive` and `thread/delete` reject attempts to remove a live internal
codex-rs/app-server/src/message_processor.rs
+2 -1
View File
@@ -1746,7 +1746,8 @@ impl MessageProcessor {
self.turn_processor.thread_realtime_list_voices().await
}
ClientRequest::ReviewStart { params, .. } => {
self.turn_processor.review_start(&request_id, params).await
// Keep review setup from inflating the shared request dispatcher's stack frame.
Box::pin(self.turn_processor.review_start(&request_id, params)).await
}
ClientRequest::McpServerOauthLogin { params, .. } => {
// Keep MCP discovery's large future from inflating every request.
codex-rs/app-server/src/request_processors.rs
-3
View File
@@ -172,7 +172,6 @@ use codex_app_server_protocol::RateLimitResetCreditStatus;
use codex_app_server_protocol::RateLimitResetCreditsSummary;
use codex_app_server_protocol::RateLimitResetType;
use codex_app_server_protocol::RequestId;
use codex_app_server_protocol::ReviewDelivery as ApiReviewDelivery;
use codex_app_server_protocol::ReviewStartParams;
use codex_app_server_protocol::ReviewStartResponse;
use codex_app_server_protocol::ReviewTarget as ApiReviewTarget;
@@ -459,7 +458,6 @@ use codex_protocol::protocol::GitInfo as CoreGitInfo;
use codex_protocol::protocol::McpAuthStatus as CoreMcpAuthStatus;
use codex_protocol::protocol::Op;
use codex_protocol::protocol::RealtimeVoicesList;
use codex_protocol::protocol::ReviewDelivery as CoreReviewDelivery;
use codex_protocol::protocol::ReviewRequest;
use codex_protocol::protocol::ReviewTarget as CoreReviewTarget;
use codex_protocol::protocol::SessionConfiguredEvent;
@@ -723,7 +721,6 @@ use self::thread_resume_redaction::*;
use self::thread_summary::*;
pub(crate) use self::thread_lifecycle::populate_thread_turns_from_history;
pub(crate) use self::thread_processor::thread_from_stored_thread;
#[cfg(test)]
pub(crate) use self::thread_summary::read_summary_from_rollout;
#[cfg(test)]
codex-rs/app-server/src/request_processors/thread_processor.rs
+1 -1
View File
@@ -6584,7 +6584,7 @@ fn set_thread_name_from_title(thread: &mut Thread, title: String) {
thread.name = Some(title);
}
pub(crate) fn thread_from_stored_thread(
fn thread_from_stored_thread(
thread: StoredThread,
fallback_provider: &str,
fallback_cwd: &AbsolutePathBuf,
codex-rs/app-server/src/request_processors/turn_processor.rs
+40 -256
View File
@@ -1,8 +1,5 @@
use super::thread_input::ensure_direct_input_allowed;
use super::*;
use codex_agent_extension::AgentInvocation;
use codex_agent_extension::AgentRun;
use codex_agent_extension::AgentRunner;
use codex_app_server_protocol::ImageReference as V2ImageReference;
use codex_app_server_protocol::ThreadItem;
use codex_protocol::error::CodexErrorDetails;
@@ -16,7 +13,6 @@ use codex_protocol::protocol::TurnSettingsUpdate;
use codex_protocol::protocol::TurnSettingsUpdateOutcome;
use codex_protocol::turn_input::AnnotatedResponseItem;
use codex_protocol::turn_input::ResponseItemAnnotations;
use codex_skills::system_cache_root_dir;
use crate::image_url::REMOTE_IMAGE_URL_ERROR;
use crate::image_url::is_remote_image_url;
@@ -81,7 +77,6 @@ fn validate_response_item_image_urls(items: &[ResponseItem]) -> Result<(), JSONR
#[derive(Clone)]
pub(crate) struct TurnRequestProcessor {
agent_runner: AgentRunner,
auth_manager: Arc<AuthManager>,
thread_manager: Arc<ThreadManager>,
outgoing: Arc<OutgoingMessageSender>,
@@ -153,9 +148,7 @@ impl TurnRequestProcessor {
skills_watcher: Arc<SkillsWatcher>,
turn_cost_worker: Option<crate::turn_cost_worker::TurnCostWorkerHandle>,
) -> Self {
let agent_runner = AgentRunner::new(Arc::downgrade(&thread_manager));
Self {
agent_runner,
auth_manager,
thread_manager,
outgoing,
@@ -340,21 +333,25 @@ impl TurnRequestProcessor {
request_id: &ConnectionRequestId,
params: ReviewStartParams,
) -> Result<Option<ClientResponsePayload>, JSONRPCErrorError> {
if matches!(params.delivery, Some(ApiReviewDelivery::Detached)) {
self.outgoing
.send_server_notification_to_connections(
&[request_id.connection_id],
ServerNotification::DeprecationNotice(DeprecationNoticeNotification {
summary: "review/start with delivery \"detached\" is deprecated and will be removed in a future release.".to_string(),
details: Some("Use thread/start followed by review/start with delivery \"inline\" for a separate review thread, or thread/fork followed by turn/start with your own review instructions.".to_string()),
}),
)
.await;
}
// Keep review setup from inflating the shared request dispatcher's stack frame.
Box::pin(self.review_start_inner(request_id, params))
let (_, thread) = self.load_thread(&params.thread_id).await?;
self.ensure_direct_input_allowed(request_id, thread.as_ref())
.await?;
self.config_manager
.check_thread_model_provider(thread.config().await.as_ref())
.await
.map(|()| None)
.map_err(|error| config_load_error(&error))?;
let (review_request, display_text) = Self::review_request_from_target(params.target)?;
let turn_id = self
.submit_core_op(request_id, thread.as_ref(), Op::Review { review_request })
.await
.map_err(|err| internal_error(format!("failed to start review: {err}")))?;
Ok(Some(
ReviewStartResponse {
turn: Self::build_review_turn(turn_id, display_text),
review_thread_id: params.thread_id,
}
.into(),
))
}
fn track_error_response(
@@ -419,66 +416,41 @@ impl TurnRequestProcessor {
fn review_request_from_target(
target: ApiReviewTarget,
) -> Result<(ReviewRequest, String, String), JSONRPCErrorError> {
let cleaned_target = match target {
ApiReviewTarget::UncommittedChanges => ApiReviewTarget::UncommittedChanges,
) -> Result<(ReviewRequest, String), JSONRPCErrorError> {
let core_target = match target {
ApiReviewTarget::UncommittedChanges => CoreReviewTarget::UncommittedChanges,
ApiReviewTarget::BaseBranch { branch } => {
let branch = branch.trim().to_string();
if branch.is_empty() {
return Err(invalid_request("branch must not be empty".to_string()));
return Err(invalid_request("branch must not be empty"));
}
ApiReviewTarget::BaseBranch { branch }
CoreReviewTarget::BaseBranch { branch }
}
ApiReviewTarget::Commit { sha, title } => {
let sha = sha.trim().to_string();
if sha.is_empty() {
return Err(invalid_request("sha must not be empty".to_string()));
return Err(invalid_request("sha must not be empty"));
}
let title = title
.map(|t| t.trim().to_string())
.filter(|t| !t.is_empty());
ApiReviewTarget::Commit { sha, title }
CoreReviewTarget::Commit { sha, title }
}
ApiReviewTarget::Custom { instructions } => {
let trimmed = instructions.trim().to_string();
if trimmed.is_empty() {
return Err(invalid_request(
"instructions must not be empty".to_string(),
));
}
ApiReviewTarget::Custom {
instructions: trimmed,
let instructions = instructions.trim().to_string();
if instructions.is_empty() {
return Err(invalid_request("instructions must not be empty"));
}
CoreReviewTarget::Custom { instructions }
}
};
let core_target = match cleaned_target {
ApiReviewTarget::UncommittedChanges => CoreReviewTarget::UncommittedChanges,
ApiReviewTarget::BaseBranch { branch } => CoreReviewTarget::BaseBranch { branch },
ApiReviewTarget::Commit { sha, title } => CoreReviewTarget::Commit { sha, title },
ApiReviewTarget::Custom { instructions } => CoreReviewTarget::Custom { instructions },
};
let target_prompt = match &core_target {
CoreReviewTarget::UncommittedChanges => {
"Review the current code changes (staged, unstaged, and untracked files)."
.to_string()
}
CoreReviewTarget::BaseBranch { branch } => {
format!("Review the code changes against the base branch {branch:?}.")
}
CoreReviewTarget::Commit { sha, .. } => {
format!("Review the changes introduced by commit {sha:?}.")
}
CoreReviewTarget::Custom { instructions } => instructions.clone(),
};
let hint = codex_core::review_prompts::user_facing_hint(&core_target);
let review_request = ReviewRequest {
target: core_target,
user_facing_hint: Some(hint.clone()),
};
Ok((review_request, hint, target_prompt))
Ok((review_request, hint))
}
async fn request_trace_context(
@@ -1419,25 +1391,20 @@ impl TurnRequestProcessor {
Ok(Some(ThreadRealtimeStopResponse::default()))
}
fn build_review_turn(turn_id: String, display_text: &str) -> Turn {
let items = if display_text.is_empty() {
Vec::new()
} else {
vec![ThreadItem::untimed(ThreadItemPayload::UserMessage {
id: turn_id.clone(),
client_id: None,
content: vec![V2UserInput::Text {
text: display_text.to_string(),
// Review prompt display text is synthesized; no UI element ranges to preserve.
text_elements: Vec::new(),
}],
})]
};
fn build_review_turn(turn_id: String, display_text: String) -> Turn {
let item = ThreadItem::untimed(ThreadItemPayload::UserMessage {
id: turn_id.clone(),
client_id: None,
content: vec![V2UserInput::Text {
text: display_text,
text_elements: Vec::new(),
}],
});
Turn {
id: turn_id,
root_turn_id: None,
items,
items: vec![item],
items_view: TurnItemsView::NotLoaded,
error: None,
status: TurnStatus::InProgress,
@@ -1447,189 +1414,6 @@ impl TurnRequestProcessor {
}
}
async fn emit_review_started(
&self,
request_id: &ConnectionRequestId,
turn: Turn,
review_thread_id: String,
) {
let response = ReviewStartResponse {
turn,
review_thread_id,
};
self.outgoing
.send_response(request_id.clone(), response)
.await;
}
async fn start_inline_review(
&self,
request_id: &ConnectionRequestId,
parent_thread: Arc<CodexThread>,
review_request: ReviewRequest,
display_text: &str,
parent_thread_id: String,
) -> std::result::Result<(), JSONRPCErrorError> {
let turn_id = self
.submit_core_op(
request_id,
parent_thread.as_ref(),
Op::Review { review_request },
)
.await
.map_err(|err| internal_error(format!("failed to start review: {err}")))?;
let turn = Self::build_review_turn(turn_id, display_text);
self.emit_review_started(request_id, turn, parent_thread_id)
.await;
Ok(())
}
async fn start_detached_review(
&self,
request_id: &ConnectionRequestId,
parent_thread: Arc<CodexThread>,
prompt: &str,
) -> std::result::Result<(), JSONRPCErrorError> {
// AgentRunner::start still delegates to spawn_legacy_subagent, which forks from the parent's
// full history. Paginated threads only allow bounded model-context reads, so keep this
// closed until detached review has a bounded fork path.
if matches!(
parent_thread.config_snapshot().await.history_mode,
codex_protocol::protocol::ThreadHistoryMode::Paginated
) {
return Err(invalid_request(
"paginated threads do not support detached review",
));
}
let mut config = parent_thread.config().await.as_ref().clone();
if let Some(review_model) = &config.review_model {
config.model = Some(review_model.clone());
}
let AgentRun {
thread_id,
thread: review_thread,
turn_id,
} = self
.agent_runner
.start(
parent_thread.startup_metadata().thread_id,
AgentInvocation {
config,
prompt: prompt.to_string(),
parent_trace: self.request_trace_context(request_id).await,
},
)
.await
.map_err(|err| internal_error(format!("failed to start detached review: {err}")))?;
let fallback_provider = self.config.model_provider_id.as_str();
let stored_thread = match review_thread
.read_thread(
/*include_archived*/ true, /*include_history*/ false,
)
.await
{
Ok(stored_thread) => {
let (thread, _) =
thread_from_stored_thread(stored_thread, fallback_provider, &self.config.cwd);
Some(thread)
}
Err(err) => {
tracing::warn!("failed to load summary for review thread {thread_id}: {err}");
None
}
};
if let Some(mut thread) = stored_thread {
let config_snapshot = review_thread.config_snapshot().await;
apply_live_thread_settings(&mut thread, &config_snapshot);
thread.session_id = review_thread.startup_metadata().session_id.to_string();
self.thread_watch_manager
.upsert_thread_silently(&thread.id)
.await;
thread.status = resolve_thread_status(
self.thread_watch_manager
.loaded_status_for_thread(&thread.id)
.await,
/*has_in_progress_turn*/ false,
);
let notif = thread_started_notification(thread);
self.outgoing
.send_server_notification(ServerNotification::ThreadStarted(notif))
.await;
}
log_listener_attach_result(
self.ensure_conversation_listener(
thread_id,
request_id.connection_id,
/*raw_events_enabled*/ false,
)
.await,
thread_id,
request_id.connection_id,
"review thread",
);
let turn = Self::build_review_turn(turn_id, prompt);
let review_thread_id = thread_id.to_string();
self.emit_review_started(request_id, turn, review_thread_id)
.await;
Ok(())
}
async fn review_start_inner(
&self,
request_id: &ConnectionRequestId,
params: ReviewStartParams,
) -> Result<(), JSONRPCErrorError> {
let ReviewStartParams {
thread_id,
target,
delivery,
} = params;
let (_, parent_thread) = self.load_thread(&thread_id).await?;
self.ensure_direct_input_allowed(request_id, parent_thread.as_ref())
.await?;
self.config_manager
.check_thread_model_provider(parent_thread.config().await.as_ref())
.await
.map_err(|error| config_load_error(&error))?;
let (review_request, display_text, target_prompt) =
Self::review_request_from_target(target)?;
match delivery.unwrap_or(ApiReviewDelivery::Inline).to_core() {
CoreReviewDelivery::Inline => {
self.start_inline_review(
request_id,
parent_thread,
review_request,
&display_text,
thread_id,
)
.await?;
}
CoreReviewDelivery::Detached => {
let review_skill_path = system_cache_root_dir(&self.config.codex_home)
.join("review-agent")
.join("SKILL.md");
let prompt = format!(
"Use [$review-agent]({}) for this review.\n\n{target_prompt}",
review_skill_path.display()
);
let actual_chars = prompt.chars().count();
if actual_chars > MAX_USER_INPUT_TEXT_CHARS {
return Err(Self::input_too_large_error(actual_chars));
}
self.start_detached_review(request_id, parent_thread, &prompt)
.await?;
}
}
Ok(())
}
async fn turn_interrupt_inner(
&self,
request_id: &ConnectionRequestId,
codex-rs/app-server/tests/suite/v2/model_provider_enforcement_tests.rs
-4
View File
@@ -150,10 +150,6 @@ base_url = "{}/v1"
"review/start",
json!({ "threadId": started.thread.id, "target": { "type": "custom", "instructions": "Review this change." }, "delivery": "inline" }),
),
(
"review/start",
json!({ "threadId": started.thread.id, "target": { "type": "custom", "instructions": "Review this change." }, "delivery": "detached" }),
),
] {
let id = server.send_raw_request(method, Some(params)).await?;
let error = server
codex-rs/app-server/tests/suite/v2/review.rs
-243
View File
@@ -6,32 +6,23 @@ use app_test_support::create_final_assistant_message_sse_response;
use app_test_support::create_mock_responses_server_repeating_assistant;
use app_test_support::create_mock_responses_server_sequence;
use codex_app_server_protocol::ClientRequest;
use codex_app_server_protocol::DeprecationNoticeNotification;
use codex_app_server_protocol::ItemCompletedNotification;
use codex_app_server_protocol::ItemStartedNotification;
use codex_app_server_protocol::JSONRPCError;
use codex_app_server_protocol::JSONRPCMessage;
use codex_app_server_protocol::RequestId;
use codex_app_server_protocol::ReviewDelivery;
use codex_app_server_protocol::ReviewStartParams;
use codex_app_server_protocol::ReviewStartResponse;
use codex_app_server_protocol::ReviewTarget;
use codex_app_server_protocol::ServerRequest;
use codex_app_server_protocol::ThreadHistoryMode;
use codex_app_server_protocol::ThreadItem;
use codex_app_server_protocol::ThreadItemPayload;
use codex_app_server_protocol::ThreadStartParams;
use codex_app_server_protocol::ThreadStartResponse;
use codex_app_server_protocol::ThreadStartedNotification;
use codex_app_server_protocol::ThreadStatusChangedNotification;
use codex_app_server_protocol::TurnItemsView;
use codex_app_server_protocol::TurnStartParams;
use codex_app_server_protocol::TurnStartResponse;
use codex_app_server_protocol::TurnStatus;
use codex_app_server_protocol::UserInput as V2UserInput;
use codex_features::Feature;
use codex_skills::system_cache_root_dir;
use core_test_support::responses;
use pretty_assertions::assert_eq;
use serde_json::json;
use tempfile::TempDir;
@@ -40,57 +31,6 @@ use tokio::time::timeout;
const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10);
const INVALID_REQUEST_ERROR_CODE: i64 = -32600;
const COLLIDING_REVIEW_SKILL_MARKER: &str = "COLLIDING_REVIEW_SKILL_MARKER";
#[tokio::test]
async fn review_start_rejects_detached_delivery_for_paginated_parent() -> Result<()> {
let server = create_mock_responses_server_repeating_assistant("Done").await;
let codex_home = TempDir::new()?;
create_config_toml(codex_home.path(), &server.uri())?;
let mut mcp = TestAppServer::builder()
.with_codex_home(codex_home.path())
.without_auto_env()
.build_initialized()
.await?;
let ThreadStartResponse { thread, .. } = mcp
.request(|request_id| ClientRequest::ThreadStart {
request_id,
params: ThreadStartParams {
history_mode: Some(ThreadHistoryMode::Paginated),
..Default::default()
},
})
.await?;
let review_id = mcp
.send_review_start_request(ReviewStartParams {
thread_id: thread.id,
delivery: Some(ReviewDelivery::Detached),
target: ReviewTarget::Custom {
instructions: "detached review".to_string(),
},
})
.await?;
let review_err: JSONRPCError = timeout(
DEFAULT_READ_TIMEOUT,
mcp.read_stream_until_error_message(RequestId::Integer(review_id)),
)
.await??;
assert_eq!(review_err.error.code, -32600);
assert_eq!(
review_err.error.message,
"paginated threads do not support detached review"
);
assert!(
mcp.pending_notification_methods()
.iter()
.any(|method| method == "deprecationNotice"),
"rejected detached requests should still emit a deprecation notice"
);
Ok(())
}
#[test_case(None; "omitted_delivery")]
#[test_case(Some(json!(null)); "null_delivery")]
@@ -341,166 +281,6 @@ async fn review_start_rejects_empty_base_branch() -> Result<()> {
Ok(())
}
#[cfg_attr(target_os = "windows", ignore = "flaky on windows CI")]
#[tokio::test]
async fn review_start_with_detached_delivery_returns_new_thread_id() -> Result<()> {
let server = responses::start_mock_server().await;
let response_mock = responses::mount_sse_sequence(
&server,
vec![
responses::sse(vec![
responses::ev_response_created("materialize-response"),
responses::ev_assistant_message("materialize-message", "materialized"),
responses::ev_completed("materialize-response"),
]),
responses::sse(vec![
responses::ev_response_created("review-response"),
responses::ev_assistant_message("review-message", "No findings."),
responses::ev_completed("review-response"),
]),
],
)
.await;
let codex_home = TempDir::new()?;
let startup_provider = responses::start_mock_server().await;
create_config_toml(codex_home.path(), &startup_provider.uri())?;
let colliding_skill_dir = codex_home.path().join("skills/review-agent-collision");
std::fs::create_dir_all(&colliding_skill_dir)?;
std::fs::write(
colliding_skill_dir.join("SKILL.md"),
format!(
"---\nname: review-agent\ndescription: Colliding user review skill.\n---\n\n{COLLIDING_REVIEW_SKILL_MARKER}\n"
),
)?;
let canonical_codex_home = std::fs::canonicalize(codex_home.path())?.try_into()?;
let review_skill_path = system_cache_root_dir(&canonical_codex_home)
.join("review-agent")
.join("SKILL.md");
let expected_prompt = format!(
"Use [$review-agent]({}) for this review.\n\ndetached review",
review_skill_path.display()
);
let mut mcp = TestAppServer::builder()
.with_codex_home(codex_home.path())
.build_initialized()
.await?;
// New threads use the refreshed route; detached review must inherit that route.
std::fs::write(
codex_home.path().join("requirements.toml"),
format!(
"model_provider = 'review-gateway'\n[model_providers.review-gateway]\nname = 'Review Gateway'\nbase_url = '{}/v1'\n",
server.uri()
),
)?;
let ThreadStartResponse { thread, .. } = mcp
.start_thread(ThreadStartParams {
model: Some("mock-model".to_string()),
history_mode: Some(ThreadHistoryMode::Legacy),
..Default::default()
})
.await?;
timeout(
DEFAULT_READ_TIMEOUT,
mcp.read_stream_until_notification_message("thread/started"),
)
.await??;
let thread_id = thread.id;
materialize_thread_rollout(&mut mcp, &thread_id).await?;
let review_id = mcp
.send_review_start_request(ReviewStartParams {
thread_id: thread_id.clone(),
delivery: Some(ReviewDelivery::Detached),
target: ReviewTarget::Custom {
instructions: "detached review".to_string(),
},
})
.await?;
let notice: DeprecationNoticeNotification = timeout(
DEFAULT_READ_TIMEOUT,
mcp.read_notification("deprecationNotice"),
)
.await??;
assert_eq!(
notice,
DeprecationNoticeNotification {
summary: "review/start with delivery \"detached\" is deprecated and will be removed in a future release.".to_string(),
details: Some("Use thread/start followed by review/start with delivery \"inline\" for a separate review thread, or thread/fork followed by turn/start with your own review instructions.".to_string()),
}
);
let ReviewStartResponse {
turn,
review_thread_id,
} = timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(review_id)).await??;
assert_eq!(turn.status, TurnStatus::InProgress);
assert_eq!(turn.items_view, TurnItemsView::NotLoaded);
assert_eq!(
turn.items,
[ThreadItem::untimed(ThreadItemPayload::UserMessage {
id: turn.id.clone(),
client_id: None,
content: vec![V2UserInput::Text {
text: expected_prompt.clone(),
text_elements: Vec::new(),
}],
})]
);
assert_ne!(
review_thread_id, thread_id,
"detached review should run on a different thread"
);
let deadline = tokio::time::Instant::now() + DEFAULT_READ_TIMEOUT;
let notification = loop {
let remaining = deadline.saturating_duration_since(tokio::time::Instant::now());
let message = timeout(remaining, mcp.read_next_message()).await??;
let JSONRPCMessage::Notification(notification) = message else {
continue;
};
if notification.method == "thread/status/changed" {
let status_changed: ThreadStatusChangedNotification =
serde_json::from_value(notification.params.expect("params must be present"))?;
if status_changed.thread_id == review_thread_id {
anyhow::bail!(
"detached review threads should be introduced without a preceding thread/status/changed"
);
}
continue;
}
if notification.method == "thread/started" {
break notification;
}
};
let started: ThreadStartedNotification =
serde_json::from_value(notification.params.expect("params must be present"))?;
assert_eq!(started.thread.id, review_thread_id);
assert_eq!(started.thread.session_id, review_thread_id);
timeout(
DEFAULT_READ_TIMEOUT,
mcp.read_stream_until_notification_message("turn/completed"),
)
.await??;
let requests = response_mock.requests();
assert_eq!(requests.len(), 2);
let review_request = &requests[1];
assert_eq!(review_request.header("x-openai-subagent"), None);
assert!(review_request.body_contains_text("Colliding user review skill."));
let user_messages = review_request.message_input_texts("user");
assert!(user_messages.iter().any(|text| text == &expected_prompt));
assert!(user_messages.iter().any(|text| {
text.starts_with("<skill>")
&& text.contains("<name>review-agent</name>")
&& text.contains("Do not modify files")
}));
assert!(!review_request.body_contains_text(COLLIDING_REVIEW_SKILL_MARKER));
Ok(())
}
#[tokio::test]
async fn review_start_rejects_empty_commit_sha() -> Result<()> {
let server = create_mock_responses_server_repeating_assistant("Done").await;
@@ -592,29 +372,6 @@ async fn start_default_thread(mcp: &mut TestAppServer) -> Result<String> {
Ok(thread.id)
}
async fn materialize_thread_rollout(mcp: &mut TestAppServer, thread_id: &str) -> Result<()> {
let _: TurnStartResponse = mcp
.request(|request_id| ClientRequest::TurnStart {
request_id,
params: TurnStartParams {
thread_id: thread_id.to_string(),
client_user_message_id: None,
input: vec![V2UserInput::Text {
text: "materialize rollout".to_string(),
text_elements: Vec::new(),
}],
..Default::default()
},
})
.await?;
timeout(
DEFAULT_READ_TIMEOUT,
mcp.read_stream_until_notification_message("turn/completed"),
)
.await??;
Ok(())
}
fn create_config_toml(codex_home: &std::path::Path, server_uri: &str) -> std::io::Result<()> {
MockResponsesConfig::new(server_uri)
.with_provider_name("Mock provider")
codex-rs/app-server/tests/suite/v2/turn_start.rs
-7
View File
@@ -4892,13 +4892,6 @@ async fn direct_input_to_multi_agent_v2_subagent_is_rejected(
"delivery": "inline",
}),
),
(
"review/start",
json!({
"target": {"type": "custom", "instructions": "Start a detached task."},
"delivery": "detached",
}),
),
(
"thread/realtime/start",
json!({
codex-rs/protocol/src/protocol.rs
-7
View File
@@ -3682,13 +3682,6 @@ pub struct GitInfo {
pub repository_url: Option<SanitizedGitUrl>,
}
#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)]
#[serde(rename_all = "snake_case")]
pub enum ReviewDelivery {
Inline,
Detached,
}
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema, TS)]
#[serde(tag = "type", rename_all = "camelCase")]
#[ts(tag = "type")]
sdk/python/scripts/update_sdk_artifacts.py
+3
View File
@@ -547,6 +547,9 @@ def _normalized_schema_bundle_text(schema_dir: Path) -> str:
required.remove(field)
for name, definition in definitions.items():
if isinstance(definition, dict):
if name == "ReviewDelivery":
# Preserve ReviewDelivery.inline when only one delivery mode remains.
definition["x-enum-field-as-literal"] = False
if name == "CodexErrorInfo":
_normalize_open_ended_enum(definition)
_flatten_string_enum_one_of(definition)
sdk/python/src/openai_codex/generated/v2_all.py
+2 -7
View File
@@ -4135,7 +4135,6 @@ class ResponsesApiWebSearchAction(
class ReviewDelivery(Enum):
inline = "inline"
detached = "detached"
class UncommittedChangesReviewTarget(BaseModel):
@@ -9460,7 +9459,7 @@ class ReviewStartParams(BaseModel):
delivery: Annotated[
ReviewDelivery | None,
Field(
description="Where to run the review: inline (default) on the current thread or detached on a new thread (returned in `reviewThreadId`). Detached delivery is deprecated and emits `deprecationNotice`. Use `thread/start` followed by an inline review for a separate review thread."
description="Reviews run inline on the supplied thread. Omit this field or pass `inline`. To review on a separate thread, start a thread first and use its id."
),
] = None
target: ReviewTarget
@@ -12565,11 +12564,7 @@ class ReviewStartResponse(BaseModel):
populate_by_name=True,
)
review_thread_id: Annotated[
str,
Field(
alias="reviewThreadId",
description="Identifies the thread where the review runs.\n\nFor inline reviews, this is the original thread id. For detached reviews, this is the id of the new review thread.",
),
str, Field(alias="reviewThreadId", description="The thread id supplied to `review/start`.")
]
turn: Turn
sdk/python/tests/test_client_rpc_methods.py
+10
View File
@@ -23,6 +23,8 @@ from openai_codex.generated.v2_all import (
PlanType,
ReasoningEffort,
ReasoningEffortOption,
ReviewDelivery,
ReviewStartParams,
ThreadForkParams,
ThreadListParams,
ThreadQueueChangedNotification,
@@ -39,6 +41,14 @@ from openai_codex.types import ThreadSource
ROOT = Path(__file__).resolve().parents[1]
def test_inline_review_delivery_round_trips_as_an_enum() -> None:
wire = {"threadId": "thread-1", "target": {"type": "uncommittedChanges"}, "delivery": "inline"}
params = ReviewStartParams.model_validate(wire)
assert params.delivery is ReviewDelivery.inline
assert params.model_dump(mode="json", by_alias=True) == wire
@pytest.mark.parametrize(
("model", "fields"),
[