mirror of
https://github.com/openai/codex.git
synced 2026-02-02 06:57:03 +00:00
Compare commits
21 Commits
blocking-s
...
owen/jb_pa
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
67b5ef9cb0 | ||
|
|
ce7fc27f89 | ||
|
|
8532876ad8 | ||
|
|
44d92675eb | ||
|
|
a421eba31f | ||
|
|
40006808a3 | ||
|
|
ba58184349 | ||
|
|
14df5c9492 | ||
|
|
cb85a7b96e | ||
|
|
3f12f1140f | ||
|
|
c22cd2e953 | ||
|
|
ebd485b1a0 | ||
|
|
457c9fdb87 | ||
|
|
6eeaf46ac1 | ||
|
|
aaec8abf58 | ||
|
|
cbd7d0d543 | ||
|
|
fabdbfef9c | ||
|
|
8b314e2d04 | ||
|
|
963009737f | ||
|
|
e953092949 | ||
|
|
28ff364c3a |
5
.github/workflows/cla.yml
vendored
5
.github/workflows/cla.yml
vendored
@@ -46,7 +46,4 @@ jobs:
|
||||
path-to-document: https://github.com/openai/codex/blob/main/docs/CLA.md
|
||||
path-to-signatures: signatures/cla.json
|
||||
branch: cla-signatures
|
||||
allowlist: |
|
||||
codex
|
||||
dependabot
|
||||
dependabot[bot]
|
||||
allowlist: codex,dependabot,dependabot[bot],github-actions[bot]
|
||||
|
||||
71
codex-rs/Cargo.lock
generated
71
codex-rs/Cargo.lock
generated
@@ -198,9 +198,9 @@ dependencies = [
|
||||
|
||||
[[package]]
|
||||
name = "arboard"
|
||||
version = "3.6.0"
|
||||
version = "3.6.1"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "55f533f8e0af236ffe5eb979b99381df3258853f00ba2e44b6e1955292c75227"
|
||||
checksum = "0348a1c054491f4bfe6ab86a7b6ab1e44e45d899005de92f58b3df180b36ddaf"
|
||||
dependencies = [
|
||||
"clipboard-win",
|
||||
"image",
|
||||
@@ -212,7 +212,7 @@ dependencies = [
|
||||
"objc2-foundation",
|
||||
"parking_lot",
|
||||
"percent-encoding",
|
||||
"windows-sys 0.52.0",
|
||||
"windows-sys 0.60.2",
|
||||
"wl-clipboard-rs",
|
||||
"x11rb",
|
||||
]
|
||||
@@ -1144,6 +1144,7 @@ dependencies = [
|
||||
"codex-apply-patch",
|
||||
"codex-arg0",
|
||||
"codex-async-utils",
|
||||
"codex-core",
|
||||
"codex-execpolicy",
|
||||
"codex-file-search",
|
||||
"codex-git",
|
||||
@@ -2535,7 +2536,7 @@ checksum = "0ce92ff622d6dadf7349484f42c93271a0d49b7cc4d466a936405bacbe10aa78"
|
||||
dependencies = [
|
||||
"cfg-if",
|
||||
"rustix 1.0.8",
|
||||
"windows-sys 0.52.0",
|
||||
"windows-sys 0.59.0",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -3292,9 +3293,9 @@ dependencies = [
|
||||
|
||||
[[package]]
|
||||
name = "image"
|
||||
version = "0.25.8"
|
||||
version = "0.25.9"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "529feb3e6769d234375c4cf1ee2ce713682b8e76538cb13f9fc23e1400a591e7"
|
||||
checksum = "e6506c6c10786659413faa717ceebcb8f70731c0a60cbae39795fdf114519c1a"
|
||||
dependencies = [
|
||||
"bytemuck",
|
||||
"byteorder-lite",
|
||||
@@ -3302,8 +3303,8 @@ dependencies = [
|
||||
"num-traits",
|
||||
"png",
|
||||
"tiff",
|
||||
"zune-core",
|
||||
"zune-jpeg",
|
||||
"zune-core 0.5.0",
|
||||
"zune-jpeg 0.5.5",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -3439,7 +3440,7 @@ checksum = "e04d7f318608d35d4b61ddd75cbdaee86b023ebe2bd5a66ee0915f0bf93095a9"
|
||||
dependencies = [
|
||||
"hermit-abi",
|
||||
"libc",
|
||||
"windows-sys 0.52.0",
|
||||
"windows-sys 0.59.0",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -5078,9 +5079,9 @@ checksum = "2b15c43186be67a4fd63bee50d0303afffcef381492ebe2c5d87f324e1b8815c"
|
||||
|
||||
[[package]]
|
||||
name = "reqwest"
|
||||
version = "0.12.23"
|
||||
version = "0.12.24"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "d429f34c8092b2d42c7c93cec323bb4adeb7c67698f70839adec842ec10c7ceb"
|
||||
checksum = "9d0946410b9f7b082a427e4ef5c8ff541a88b357bc6c637c40db3a68ac70a36f"
|
||||
dependencies = [
|
||||
"base64",
|
||||
"bytes",
|
||||
@@ -5216,7 +5217,7 @@ dependencies = [
|
||||
"errno",
|
||||
"libc",
|
||||
"linux-raw-sys 0.4.15",
|
||||
"windows-sys 0.52.0",
|
||||
"windows-sys 0.59.0",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -5735,9 +5736,9 @@ dependencies = [
|
||||
|
||||
[[package]]
|
||||
name = "serde_with"
|
||||
version = "3.14.0"
|
||||
version = "3.16.1"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "f2c45cd61fefa9db6f254525d46e392b852e0e61d9a1fd36e5bd183450a556d5"
|
||||
checksum = "4fa237f2807440d238e0364a218270b98f767a00d3dada77b1c53ae88940e2e7"
|
||||
dependencies = [
|
||||
"base64",
|
||||
"chrono",
|
||||
@@ -5746,8 +5747,7 @@ dependencies = [
|
||||
"indexmap 2.12.0",
|
||||
"schemars 0.9.0",
|
||||
"schemars 1.0.4",
|
||||
"serde",
|
||||
"serde_derive",
|
||||
"serde_core",
|
||||
"serde_json",
|
||||
"serde_with_macros",
|
||||
"time",
|
||||
@@ -5755,11 +5755,11 @@ dependencies = [
|
||||
|
||||
[[package]]
|
||||
name = "serde_with_macros"
|
||||
version = "3.14.0"
|
||||
version = "3.16.1"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "de90945e6565ce0d9a25098082ed4ee4002e047cb59892c318d66821e14bb30f"
|
||||
checksum = "52a8e3ca0ca629121f70ab50f95249e5a6f925cc0f6ffe8256c45b728875706c"
|
||||
dependencies = [
|
||||
"darling 0.20.11",
|
||||
"darling 0.21.3",
|
||||
"proc-macro2",
|
||||
"quote",
|
||||
"syn 2.0.104",
|
||||
@@ -6408,7 +6408,7 @@ dependencies = [
|
||||
"half",
|
||||
"quick-error",
|
||||
"weezl",
|
||||
"zune-jpeg",
|
||||
"zune-jpeg 0.4.19",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -6713,9 +6713,9 @@ checksum = "8df9b6e13f2d32c91b9bd719c00d1958837bc7dec474d94952798cc8e69eeec3"
|
||||
|
||||
[[package]]
|
||||
name = "tracing"
|
||||
version = "0.1.41"
|
||||
version = "0.1.43"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "784e0ac535deb450455cbfa28a6f0df145ea1bb7ae51b821cf5e7927fdcfbdd0"
|
||||
checksum = "2d15d90a0b5c19378952d479dc858407149d7bb45a14de0142f6c534b16fc647"
|
||||
dependencies = [
|
||||
"log",
|
||||
"pin-project-lite",
|
||||
@@ -6737,9 +6737,9 @@ dependencies = [
|
||||
|
||||
[[package]]
|
||||
name = "tracing-attributes"
|
||||
version = "0.1.30"
|
||||
version = "0.1.31"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "81383ab64e72a7a8b8e13130c49e3dab29def6d0c7d76a03087b3cf71c5c6903"
|
||||
checksum = "7490cfa5ec963746568740651ac6781f701c9c5ea257c58e057f3ba8cf69e8da"
|
||||
dependencies = [
|
||||
"proc-macro2",
|
||||
"quote",
|
||||
@@ -6748,9 +6748,9 @@ dependencies = [
|
||||
|
||||
[[package]]
|
||||
name = "tracing-core"
|
||||
version = "0.1.34"
|
||||
version = "0.1.35"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "b9d12581f227e93f094d3af2ae690a574abb8a2b9b7a96e7cfe9647b2b617678"
|
||||
checksum = "7a04e24fab5c89c6a36eb8558c9656f30d81de51dfa4d3b45f26b21d61fa0a6c"
|
||||
dependencies = [
|
||||
"once_cell",
|
||||
"valuable",
|
||||
@@ -7368,7 +7368,7 @@ version = "0.1.9"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "cf221c93e13a30d793f7645a0e7762c55d169dbb0a49671918a2319d289b10bb"
|
||||
dependencies = [
|
||||
"windows-sys 0.52.0",
|
||||
"windows-sys 0.59.0",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
@@ -8093,13 +8093,28 @@ version = "0.4.12"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "3f423a2c17029964870cfaabb1f13dfab7d092a62a29a89264f4d36990ca414a"
|
||||
|
||||
[[package]]
|
||||
name = "zune-core"
|
||||
version = "0.5.0"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "111f7d9820f05fd715df3144e254d6fc02ee4088b0644c0ffd0efc9e6d9d2773"
|
||||
|
||||
[[package]]
|
||||
name = "zune-jpeg"
|
||||
version = "0.4.19"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "2c9e525af0a6a658e031e95f14b7f889976b74a11ba0eca5a5fc9ac8a1c43a6a"
|
||||
dependencies = [
|
||||
"zune-core",
|
||||
"zune-core 0.4.12",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
name = "zune-jpeg"
|
||||
version = "0.5.5"
|
||||
source = "registry+https://github.com/rust-lang/crates.io-index"
|
||||
checksum = "dc6fb7703e32e9a07fb3f757360338b3a567a5054f21b5f52a666752e333d58e"
|
||||
dependencies = [
|
||||
"zune-core 0.5.0",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
|
||||
@@ -136,7 +136,7 @@ icu_decimal = "2.1"
|
||||
icu_locale_core = "2.1"
|
||||
icu_provider = { version = "2.1", features = ["sync"] }
|
||||
ignore = "0.4.23"
|
||||
image = { version = "^0.25.8", default-features = false }
|
||||
image = { version = "^0.25.9", default-features = false }
|
||||
indexmap = "2.12.0"
|
||||
insta = "1.43.2"
|
||||
itertools = "0.14.0"
|
||||
@@ -178,7 +178,7 @@ seccompiler = "0.5.0"
|
||||
sentry = "0.34.0"
|
||||
serde = "1"
|
||||
serde_json = "1"
|
||||
serde_with = "3.14"
|
||||
serde_with = "3.16"
|
||||
serial_test = "3.2.0"
|
||||
sha1 = "0.10.6"
|
||||
sha2 = "0.10"
|
||||
@@ -203,7 +203,7 @@ tokio-util = "0.7.16"
|
||||
toml = "0.9.5"
|
||||
toml_edit = "0.23.5"
|
||||
tonic = "0.13.1"
|
||||
tracing = "0.1.41"
|
||||
tracing = "0.1.43"
|
||||
tracing-appender = "0.2.3"
|
||||
tracing-subscriber = "0.3.20"
|
||||
tracing-test = "0.2.5"
|
||||
|
||||
@@ -131,7 +131,7 @@ client_request_definitions! {
|
||||
},
|
||||
ReviewStart => "review/start" {
|
||||
params: v2::ReviewStartParams,
|
||||
response: v2::TurnStartResponse,
|
||||
response: v2::ReviewStartResponse,
|
||||
},
|
||||
|
||||
ModelList => "model/list" {
|
||||
@@ -506,10 +506,12 @@ server_notification_definitions! {
|
||||
TurnStarted => "turn/started" (v2::TurnStartedNotification),
|
||||
TurnCompleted => "turn/completed" (v2::TurnCompletedNotification),
|
||||
TurnDiffUpdated => "turn/diff/updated" (v2::TurnDiffUpdatedNotification),
|
||||
TurnPlanUpdated => "turn/plan/updated" (v2::TurnPlanUpdatedNotification),
|
||||
ItemStarted => "item/started" (v2::ItemStartedNotification),
|
||||
ItemCompleted => "item/completed" (v2::ItemCompletedNotification),
|
||||
AgentMessageDelta => "item/agentMessage/delta" (v2::AgentMessageDeltaNotification),
|
||||
CommandExecutionOutputDelta => "item/commandExecution/outputDelta" (v2::CommandExecutionOutputDeltaNotification),
|
||||
FileChangeOutputDelta => "item/fileChange/outputDelta" (v2::FileChangeOutputDeltaNotification),
|
||||
McpToolCallProgress => "item/mcpToolCall/progress" (v2::McpToolCallProgressNotification),
|
||||
AccountUpdated => "account/updated" (v2::AccountUpdatedNotification),
|
||||
AccountRateLimitsUpdated => "account/rateLimits/updated" (v2::AccountRateLimitsUpdatedNotification),
|
||||
|
||||
@@ -11,6 +11,8 @@ use codex_protocol::items::AgentMessageContent as CoreAgentMessageContent;
|
||||
use codex_protocol::items::TurnItem as CoreTurnItem;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use codex_protocol::parse_command::ParsedCommand as CoreParsedCommand;
|
||||
use codex_protocol::plan_tool::PlanItemArg as CorePlanItemArg;
|
||||
use codex_protocol::plan_tool::StepStatus as CorePlanStepStatus;
|
||||
use codex_protocol::protocol::CodexErrorInfo as CoreCodexErrorInfo;
|
||||
use codex_protocol::protocol::CreditsSnapshot as CoreCreditsSnapshot;
|
||||
use codex_protocol::protocol::RateLimitSnapshot as CoreRateLimitSnapshot;
|
||||
@@ -130,6 +132,12 @@ v2_enum_from_core!(
|
||||
}
|
||||
);
|
||||
|
||||
v2_enum_from_core!(
|
||||
pub enum ReviewDelivery from codex_protocol::protocol::ReviewDelivery {
|
||||
Inline, Detached
|
||||
}
|
||||
);
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
@@ -908,9 +916,22 @@ pub struct ReviewStartParams {
|
||||
pub thread_id: String,
|
||||
pub target: ReviewTarget,
|
||||
|
||||
/// When true, also append the final review message to the original thread.
|
||||
/// Where to run the review: inline (default) on the current thread or
|
||||
/// detached on a new thread (returned in `reviewThreadId`).
|
||||
#[serde(default)]
|
||||
pub append_to_original_thread: bool,
|
||||
pub delivery: Option<ReviewDelivery>,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[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.
|
||||
pub review_thread_id: String,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
@@ -1020,6 +1041,8 @@ pub enum ThreadItem {
|
||||
command: String,
|
||||
/// The command's working directory.
|
||||
cwd: PathBuf,
|
||||
/// Identifier for the underlying PTY process (when available).
|
||||
process_id: Option<String>,
|
||||
status: CommandExecutionStatus,
|
||||
/// A best-effort parsing of the command to understand the action(s) it will perform.
|
||||
/// This returns a list of CommandAction objects because a single shell command may
|
||||
@@ -1061,7 +1084,10 @@ pub enum ThreadItem {
|
||||
ImageView { id: String, path: String },
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(rename_all = "camelCase")]
|
||||
CodeReview { id: String, review: String },
|
||||
EnteredReviewMode { id: String, review: String },
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(rename_all = "camelCase")]
|
||||
ExitedReviewMode { id: String, review: String },
|
||||
}
|
||||
|
||||
impl From<CoreTurnItem> for ThreadItem {
|
||||
@@ -1120,7 +1146,11 @@ pub struct FileUpdateChange {
|
||||
pub enum PatchChangeKind {
|
||||
Add,
|
||||
Delete,
|
||||
Update { move_path: Option<PathBuf> },
|
||||
Update {
|
||||
move_path: Option<PathBuf>,
|
||||
old_content: String,
|
||||
new_content: String,
|
||||
},
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
@@ -1206,10 +1236,56 @@ pub struct TurnCompletedNotification {
|
||||
/// Notification that the turn-level unified diff has changed.
|
||||
/// Contains the latest aggregated diff across all file changes in the turn.
|
||||
pub struct TurnDiffUpdatedNotification {
|
||||
pub thread_id: String,
|
||||
pub turn_id: String,
|
||||
pub diff: String,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub struct TurnPlanUpdatedNotification {
|
||||
pub turn_id: String,
|
||||
pub explanation: Option<String>,
|
||||
pub plan: Vec<TurnPlanStep>,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub struct TurnPlanStep {
|
||||
pub step: String,
|
||||
pub status: TurnPlanStepStatus,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub enum TurnPlanStepStatus {
|
||||
Pending,
|
||||
InProgress,
|
||||
Completed,
|
||||
}
|
||||
|
||||
impl From<CorePlanItemArg> for TurnPlanStep {
|
||||
fn from(value: CorePlanItemArg) -> Self {
|
||||
Self {
|
||||
step: value.step,
|
||||
status: value.status.into(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl From<CorePlanStepStatus> for TurnPlanStepStatus {
|
||||
fn from(value: CorePlanStepStatus) -> Self {
|
||||
match value {
|
||||
CorePlanStepStatus::Pending => Self::Pending,
|
||||
CorePlanStepStatus::InProgress => Self::InProgress,
|
||||
CorePlanStepStatus::Completed => Self::Completed,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
@@ -1233,6 +1309,8 @@ pub struct ItemCompletedNotification {
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub struct AgentMessageDeltaNotification {
|
||||
pub thread_id: String,
|
||||
pub turn_id: String,
|
||||
pub item_id: String,
|
||||
pub delta: String,
|
||||
}
|
||||
@@ -1241,6 +1319,8 @@ pub struct AgentMessageDeltaNotification {
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub struct ReasoningSummaryTextDeltaNotification {
|
||||
pub thread_id: String,
|
||||
pub turn_id: String,
|
||||
pub item_id: String,
|
||||
pub delta: String,
|
||||
pub summary_index: i64,
|
||||
@@ -1250,6 +1330,8 @@ pub struct ReasoningSummaryTextDeltaNotification {
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub struct ReasoningSummaryPartAddedNotification {
|
||||
pub thread_id: String,
|
||||
pub turn_id: String,
|
||||
pub item_id: String,
|
||||
pub summary_index: i64,
|
||||
}
|
||||
@@ -1258,6 +1340,8 @@ pub struct ReasoningSummaryPartAddedNotification {
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub struct ReasoningTextDeltaNotification {
|
||||
pub thread_id: String,
|
||||
pub turn_id: String,
|
||||
pub item_id: String,
|
||||
pub delta: String,
|
||||
pub content_index: i64,
|
||||
@@ -1267,6 +1351,18 @@ pub struct ReasoningTextDeltaNotification {
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub struct CommandExecutionOutputDeltaNotification {
|
||||
pub thread_id: String,
|
||||
pub turn_id: String,
|
||||
pub item_id: String,
|
||||
pub delta: String,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub struct FileChangeOutputDeltaNotification {
|
||||
pub thread_id: String,
|
||||
pub turn_id: String,
|
||||
pub item_id: String,
|
||||
pub delta: String,
|
||||
}
|
||||
@@ -1275,6 +1371,8 @@ pub struct CommandExecutionOutputDeltaNotification {
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub struct McpToolCallProgressNotification {
|
||||
pub thread_id: String,
|
||||
pub turn_id: String,
|
||||
pub item_id: String,
|
||||
pub message: String,
|
||||
}
|
||||
|
||||
@@ -65,7 +65,7 @@ The JSON-RPC API exposes dedicated methods for managing Codex conversations. Thr
|
||||
- `thread/archive` — move a thread’s rollout file into the archived directory; returns `{}` on success.
|
||||
- `turn/start` — add user input to a thread and begin Codex generation; responds with the initial `turn` object and streams `turn/started`, `item/*`, and `turn/completed` notifications.
|
||||
- `turn/interrupt` — request cancellation of an in-flight turn by `(thread_id, turn_id)`; success is an empty `{}` response and the turn finishes with `status: "interrupted"`.
|
||||
- `review/start` — kick off Codex’s automated reviewer for a thread; responds like `turn/start` and emits a `item/completed` notification with a `codeReview` item when results are ready.
|
||||
- `review/start` — kick off Codex’s automated reviewer for a thread; responds like `turn/start` and emits `item/started`/`item/completed` notifications with `enteredReviewMode` and `exitedReviewMode` items, plus a final assistant `agentMessage` containing the review.
|
||||
|
||||
### 1) Start or resume a thread
|
||||
|
||||
@@ -190,49 +190,56 @@ Use `review/start` to run Codex’s reviewer on the currently checked-out projec
|
||||
- `{"type":"baseBranch","branch":"main"}` — diff against the provided branch’s upstream (see prompt for the exact `git merge-base`/`git diff` instructions Codex will run).
|
||||
- `{"type":"commit","sha":"abc1234","title":"Optional subject"}` — review a specific commit.
|
||||
- `{"type":"custom","instructions":"Free-form reviewer instructions"}` — fallback prompt equivalent to the legacy manual review request.
|
||||
- `appendToOriginalThread` (bool, default `false`) — when `true`, Codex also records a final assistant-style message with the review summary in the original thread. When `false`, only the `codeReview` item is emitted for the review run and no extra message is added to the original thread.
|
||||
- `delivery` (`"inline"` or `"detached"`, default `"inline"`) — where the review runs:
|
||||
- `"inline"`: run the review as a new turn on the existing thread. The response’s `reviewThreadId` equals the original `threadId`, and no new `thread/started` notification is emitted.
|
||||
- `"detached"`: fork a new review thread from the parent conversation and run the review there. The response’s `reviewThreadId` is the id of this new review thread, and the server emits a `thread/started` notification for it before streaming review items.
|
||||
|
||||
Example request/response:
|
||||
|
||||
```json
|
||||
{ "method": "review/start", "id": 40, "params": {
|
||||
"threadId": "thr_123",
|
||||
"appendToOriginalThread": true,
|
||||
"delivery": "inline",
|
||||
"target": { "type": "commit", "sha": "1234567deadbeef", "title": "Polish tui colors" }
|
||||
} }
|
||||
{ "id": 40, "result": { "turn": {
|
||||
"id": "turn_900",
|
||||
"status": "inProgress",
|
||||
"items": [
|
||||
{ "type": "userMessage", "id": "turn_900", "content": [ { "type": "text", "text": "Review commit 1234567: Polish tui colors" } ] }
|
||||
],
|
||||
"error": null
|
||||
} } }
|
||||
{ "id": 40, "result": {
|
||||
"turn": {
|
||||
"id": "turn_900",
|
||||
"status": "inProgress",
|
||||
"items": [
|
||||
{ "type": "userMessage", "id": "turn_900", "content": [ { "type": "text", "text": "Review commit 1234567: Polish tui colors" } ] }
|
||||
],
|
||||
"error": null
|
||||
},
|
||||
"reviewThreadId": "thr_123"
|
||||
} }
|
||||
```
|
||||
|
||||
For a detached review, use `"delivery": "detached"`. The response is the same shape, but `reviewThreadId` will be the id of the new review thread (different from the original `threadId`). The server also emits a `thread/started` notification for that new thread before streaming the review turn.
|
||||
|
||||
Codex streams the usual `turn/started` notification followed by an `item/started`
|
||||
with the same `codeReview` item id so clients can show progress:
|
||||
with an `enteredReviewMode` item so clients can show progress:
|
||||
|
||||
```json
|
||||
{ "method": "item/started", "params": { "item": {
|
||||
"type": "codeReview",
|
||||
"type": "enteredReviewMode",
|
||||
"id": "turn_900",
|
||||
"review": "current changes"
|
||||
} } }
|
||||
```
|
||||
|
||||
When the reviewer finishes, the server emits `item/completed` containing the same
|
||||
`codeReview` item with the final review text:
|
||||
When the reviewer finishes, the server emits `item/started` and `item/completed`
|
||||
containing an `exitedReviewMode` item with the final review text:
|
||||
|
||||
```json
|
||||
{ "method": "item/completed", "params": { "item": {
|
||||
"type": "codeReview",
|
||||
"type": "exitedReviewMode",
|
||||
"id": "turn_900",
|
||||
"review": "Looks solid overall...\n\n- Prefer Stylize helpers — app.rs:10-20\n ..."
|
||||
} } }
|
||||
```
|
||||
|
||||
The `review` string is plain text that already bundles the overall explanation plus a bullet list for each structured finding (matching `ThreadItem::CodeReview` in the generated schema). Use this notification to render the reviewer output in your client.
|
||||
The `review` string is plain text that already bundles the overall explanation plus a bullet list for each structured finding (matching `ThreadItem::ExitedReviewMode` in the generated schema). Use this notification to render the reviewer output in your client.
|
||||
|
||||
## Events (work-in-progress)
|
||||
|
||||
@@ -244,6 +251,7 @@ The app-server streams JSON-RPC notifications while a turn is running. Each turn
|
||||
|
||||
- `turn/started` — `{ turn }` with the turn id, empty `items`, and `status: "inProgress"`.
|
||||
- `turn/completed` — `{ turn }` where `turn.status` is `completed`, `interrupted`, or `failed`; failures carry `{ error: { message, codexErrorInfo? } }`.
|
||||
- `turn/plan/updated` — `{ turnId, explanation?, plan }` whenever the agent shares or changes its plan; each `plan` entry is `{ step, status }` with `status` in `pending`, `inProgress`, or `completed`.
|
||||
|
||||
Today both notifications carry an empty `items` array even when item events were streamed; rely on `item/*` notifications for the canonical item list until this is fixed.
|
||||
|
||||
|
||||
@@ -18,6 +18,7 @@ use codex_app_server_protocol::ContextCompactedNotification;
|
||||
use codex_app_server_protocol::ErrorNotification;
|
||||
use codex_app_server_protocol::ExecCommandApprovalParams;
|
||||
use codex_app_server_protocol::ExecCommandApprovalResponse;
|
||||
use codex_app_server_protocol::FileChangeOutputDeltaNotification;
|
||||
use codex_app_server_protocol::FileChangeRequestApprovalParams;
|
||||
use codex_app_server_protocol::FileChangeRequestApprovalResponse;
|
||||
use codex_app_server_protocol::FileUpdateChange;
|
||||
@@ -43,6 +44,8 @@ use codex_app_server_protocol::TurnCompletedNotification;
|
||||
use codex_app_server_protocol::TurnDiffUpdatedNotification;
|
||||
use codex_app_server_protocol::TurnError;
|
||||
use codex_app_server_protocol::TurnInterruptResponse;
|
||||
use codex_app_server_protocol::TurnPlanStep;
|
||||
use codex_app_server_protocol::TurnPlanUpdatedNotification;
|
||||
use codex_app_server_protocol::TurnStatus;
|
||||
use codex_core::CodexConversation;
|
||||
use codex_core::parse_command::shlex_join;
|
||||
@@ -60,6 +63,7 @@ use codex_core::protocol::TokenCountEvent;
|
||||
use codex_core::protocol::TurnDiffEvent;
|
||||
use codex_core::review_format::format_review_findings_block;
|
||||
use codex_protocol::ConversationId;
|
||||
use codex_protocol::plan_tool::UpdatePlanArgs;
|
||||
use codex_protocol::protocol::ReviewOutputEvent;
|
||||
use std::collections::HashMap;
|
||||
use std::convert::TryFrom;
|
||||
@@ -257,6 +261,8 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
}
|
||||
EventMsg::AgentMessageContentDelta(event) => {
|
||||
let notification = AgentMessageDeltaNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: event_turn_id.clone(),
|
||||
item_id: event.item_id,
|
||||
delta: event.delta,
|
||||
};
|
||||
@@ -275,6 +281,8 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
}
|
||||
EventMsg::ReasoningContentDelta(event) => {
|
||||
let notification = ReasoningSummaryTextDeltaNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: event_turn_id.clone(),
|
||||
item_id: event.item_id,
|
||||
delta: event.delta,
|
||||
summary_index: event.summary_index,
|
||||
@@ -287,6 +295,8 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
}
|
||||
EventMsg::ReasoningRawContentDelta(event) => {
|
||||
let notification = ReasoningTextDeltaNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: event_turn_id.clone(),
|
||||
item_id: event.item_id,
|
||||
delta: event.delta,
|
||||
content_index: event.content_index,
|
||||
@@ -297,6 +307,8 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
}
|
||||
EventMsg::AgentReasoningSectionBreak(event) => {
|
||||
let notification = ReasoningSummaryPartAddedNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: event_turn_id.clone(),
|
||||
item_id: event.item_id,
|
||||
summary_index: event.summary_index,
|
||||
};
|
||||
@@ -340,16 +352,26 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
.await;
|
||||
}
|
||||
EventMsg::EnteredReviewMode(review_request) => {
|
||||
let notification = ItemStartedNotification {
|
||||
let review = review_request.user_facing_hint;
|
||||
let item = ThreadItem::EnteredReviewMode {
|
||||
id: event_turn_id.clone(),
|
||||
review,
|
||||
};
|
||||
let started = ItemStartedNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: event_turn_id.clone(),
|
||||
item: ThreadItem::CodeReview {
|
||||
id: event_turn_id.clone(),
|
||||
review: review_request.user_facing_hint,
|
||||
},
|
||||
item: item.clone(),
|
||||
};
|
||||
outgoing
|
||||
.send_server_notification(ServerNotification::ItemStarted(notification))
|
||||
.send_server_notification(ServerNotification::ItemStarted(started))
|
||||
.await;
|
||||
let completed = ItemCompletedNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: event_turn_id.clone(),
|
||||
item,
|
||||
};
|
||||
outgoing
|
||||
.send_server_notification(ServerNotification::ItemCompleted(completed))
|
||||
.await;
|
||||
}
|
||||
EventMsg::ItemStarted(item_started_event) => {
|
||||
@@ -375,21 +397,29 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
.await;
|
||||
}
|
||||
EventMsg::ExitedReviewMode(review_event) => {
|
||||
let review_text = match review_event.review_output {
|
||||
let review = match review_event.review_output {
|
||||
Some(output) => render_review_output_text(&output),
|
||||
None => REVIEW_FALLBACK_MESSAGE.to_string(),
|
||||
};
|
||||
let review_item_id = event_turn_id.clone();
|
||||
let notification = ItemCompletedNotification {
|
||||
let item = ThreadItem::ExitedReviewMode {
|
||||
id: event_turn_id.clone(),
|
||||
review,
|
||||
};
|
||||
let started = ItemStartedNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: event_turn_id.clone(),
|
||||
item: ThreadItem::CodeReview {
|
||||
id: review_item_id,
|
||||
review: review_text,
|
||||
},
|
||||
item: item.clone(),
|
||||
};
|
||||
outgoing
|
||||
.send_server_notification(ServerNotification::ItemCompleted(notification))
|
||||
.send_server_notification(ServerNotification::ItemStarted(started))
|
||||
.await;
|
||||
let completed = ItemCompletedNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: event_turn_id.clone(),
|
||||
item,
|
||||
};
|
||||
outgoing
|
||||
.send_server_notification(ServerNotification::ItemCompleted(completed))
|
||||
.await;
|
||||
}
|
||||
EventMsg::PatchApplyBegin(patch_begin_event) => {
|
||||
@@ -449,11 +479,13 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
.collect::<Vec<_>>();
|
||||
let command = shlex_join(&exec_command_begin_event.command);
|
||||
let cwd = exec_command_begin_event.cwd;
|
||||
let process_id = exec_command_begin_event.process_id;
|
||||
|
||||
let item = ThreadItem::CommandExecution {
|
||||
id: item_id,
|
||||
command,
|
||||
cwd,
|
||||
process_id,
|
||||
status: CommandExecutionStatus::InProgress,
|
||||
command_actions,
|
||||
aggregated_output: None,
|
||||
@@ -470,15 +502,44 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
.await;
|
||||
}
|
||||
EventMsg::ExecCommandOutputDelta(exec_command_output_delta_event) => {
|
||||
let notification = CommandExecutionOutputDeltaNotification {
|
||||
item_id: exec_command_output_delta_event.call_id.clone(),
|
||||
delta: String::from_utf8_lossy(&exec_command_output_delta_event.chunk).to_string(),
|
||||
let item_id = exec_command_output_delta_event.call_id.clone();
|
||||
let delta = String::from_utf8_lossy(&exec_command_output_delta_event.chunk).to_string();
|
||||
// The underlying EventMsg::ExecCommandOutputDelta is used for shell, unified_exec,
|
||||
// and apply_patch tool calls. We represent apply_patch with the FileChange item, and
|
||||
// everything else with the CommandExecution item.
|
||||
//
|
||||
// We need to detect which item type it is so we can emit the right notification.
|
||||
// We already have state tracking FileChange items on item/started, so let's use that.
|
||||
let is_file_change = {
|
||||
let map = turn_summary_store.lock().await;
|
||||
map.get(&conversation_id)
|
||||
.is_some_and(|summary| summary.file_change_started.contains(&item_id))
|
||||
};
|
||||
outgoing
|
||||
.send_server_notification(ServerNotification::CommandExecutionOutputDelta(
|
||||
notification,
|
||||
))
|
||||
.await;
|
||||
if is_file_change {
|
||||
let notification = FileChangeOutputDeltaNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: event_turn_id.clone(),
|
||||
item_id,
|
||||
delta,
|
||||
};
|
||||
outgoing
|
||||
.send_server_notification(ServerNotification::FileChangeOutputDelta(
|
||||
notification,
|
||||
))
|
||||
.await;
|
||||
} else {
|
||||
let notification = CommandExecutionOutputDeltaNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: event_turn_id.clone(),
|
||||
item_id,
|
||||
delta,
|
||||
};
|
||||
outgoing
|
||||
.send_server_notification(ServerNotification::CommandExecutionOutputDelta(
|
||||
notification,
|
||||
))
|
||||
.await;
|
||||
}
|
||||
}
|
||||
EventMsg::ExecCommandEnd(exec_command_end_event) => {
|
||||
let ExecCommandEndEvent {
|
||||
@@ -486,6 +547,7 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
command,
|
||||
cwd,
|
||||
parsed_cmd,
|
||||
process_id,
|
||||
aggregated_output,
|
||||
exit_code,
|
||||
duration,
|
||||
@@ -514,6 +576,7 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
id: call_id,
|
||||
command: shlex_join(&command),
|
||||
cwd,
|
||||
process_id,
|
||||
status,
|
||||
command_actions,
|
||||
aggregated_output,
|
||||
@@ -563,6 +626,7 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
}
|
||||
EventMsg::TurnDiff(turn_diff_event) => {
|
||||
handle_turn_diff(
|
||||
conversation_id,
|
||||
&event_turn_id,
|
||||
turn_diff_event,
|
||||
api_version,
|
||||
@@ -570,12 +634,22 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
)
|
||||
.await;
|
||||
}
|
||||
EventMsg::PlanUpdate(plan_update_event) => {
|
||||
handle_turn_plan_update(
|
||||
&event_turn_id,
|
||||
plan_update_event,
|
||||
api_version,
|
||||
outgoing.as_ref(),
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
|
||||
async fn handle_turn_diff(
|
||||
conversation_id: ConversationId,
|
||||
event_turn_id: &str,
|
||||
turn_diff_event: TurnDiffEvent,
|
||||
api_version: ApiVersion,
|
||||
@@ -583,6 +657,7 @@ async fn handle_turn_diff(
|
||||
) {
|
||||
if let ApiVersion::V2 = api_version {
|
||||
let notification = TurnDiffUpdatedNotification {
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: event_turn_id.to_string(),
|
||||
diff: turn_diff_event.unified_diff,
|
||||
};
|
||||
@@ -592,6 +667,28 @@ async fn handle_turn_diff(
|
||||
}
|
||||
}
|
||||
|
||||
async fn handle_turn_plan_update(
|
||||
event_turn_id: &str,
|
||||
plan_update_event: UpdatePlanArgs,
|
||||
api_version: ApiVersion,
|
||||
outgoing: &OutgoingMessageSender,
|
||||
) {
|
||||
if let ApiVersion::V2 = api_version {
|
||||
let notification = TurnPlanUpdatedNotification {
|
||||
turn_id: event_turn_id.to_string(),
|
||||
explanation: plan_update_event.explanation,
|
||||
plan: plan_update_event
|
||||
.plan
|
||||
.into_iter()
|
||||
.map(TurnPlanStep::from)
|
||||
.collect(),
|
||||
};
|
||||
outgoing
|
||||
.send_server_notification(ServerNotification::TurnPlanUpdated(notification))
|
||||
.await;
|
||||
}
|
||||
}
|
||||
|
||||
async fn emit_turn_completed_with_status(
|
||||
conversation_id: ConversationId,
|
||||
event_turn_id: String,
|
||||
@@ -649,6 +746,7 @@ async fn complete_command_execution_item(
|
||||
item_id: String,
|
||||
command: String,
|
||||
cwd: PathBuf,
|
||||
process_id: Option<String>,
|
||||
command_actions: Vec<V2ParsedCommand>,
|
||||
status: CommandExecutionStatus,
|
||||
outgoing: &OutgoingMessageSender,
|
||||
@@ -657,6 +755,7 @@ async fn complete_command_execution_item(
|
||||
id: item_id,
|
||||
command,
|
||||
cwd,
|
||||
process_id,
|
||||
status,
|
||||
command_actions,
|
||||
aggregated_output: None,
|
||||
@@ -869,8 +968,15 @@ fn map_patch_change_kind(change: &CoreFileChange) -> V2PatchChangeKind {
|
||||
match change {
|
||||
CoreFileChange::Add { .. } => V2PatchChangeKind::Add,
|
||||
CoreFileChange::Delete { .. } => V2PatchChangeKind::Delete,
|
||||
CoreFileChange::Update { move_path, .. } => V2PatchChangeKind::Update {
|
||||
CoreFileChange::Update {
|
||||
move_path,
|
||||
old_content,
|
||||
new_content,
|
||||
..
|
||||
} => V2PatchChangeKind::Update {
|
||||
move_path: move_path.clone(),
|
||||
old_content: old_content.clone(),
|
||||
new_content: new_content.clone(),
|
||||
},
|
||||
}
|
||||
}
|
||||
@@ -882,6 +988,7 @@ fn format_file_change_diff(change: &CoreFileChange) -> String {
|
||||
CoreFileChange::Update {
|
||||
unified_diff,
|
||||
move_path,
|
||||
..
|
||||
} => {
|
||||
if let Some(path) = move_path {
|
||||
format!("{unified_diff}\n\nMoved to: {}", path.display())
|
||||
@@ -1015,6 +1122,7 @@ async fn on_command_execution_request_approval_response(
|
||||
item_id.clone(),
|
||||
command.clone(),
|
||||
cwd.clone(),
|
||||
None,
|
||||
command_actions.clone(),
|
||||
status,
|
||||
outgoing.as_ref(),
|
||||
@@ -1108,12 +1216,15 @@ mod tests {
|
||||
use anyhow::Result;
|
||||
use anyhow::anyhow;
|
||||
use anyhow::bail;
|
||||
use codex_app_server_protocol::TurnPlanStepStatus;
|
||||
use codex_core::protocol::CreditsSnapshot;
|
||||
use codex_core::protocol::McpInvocation;
|
||||
use codex_core::protocol::RateLimitSnapshot;
|
||||
use codex_core::protocol::RateLimitWindow;
|
||||
use codex_core::protocol::TokenUsage;
|
||||
use codex_core::protocol::TokenUsageInfo;
|
||||
use codex_protocol::plan_tool::PlanItemArg;
|
||||
use codex_protocol::plan_tool::StepStatus;
|
||||
use mcp_types::CallToolResult;
|
||||
use mcp_types::ContentBlock;
|
||||
use mcp_types::TextContent;
|
||||
@@ -1273,6 +1384,46 @@ mod tests {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_handle_turn_plan_update_emits_notification_for_v2() -> Result<()> {
|
||||
let (tx, mut rx) = mpsc::channel(CHANNEL_CAPACITY);
|
||||
let outgoing = OutgoingMessageSender::new(tx);
|
||||
let update = UpdatePlanArgs {
|
||||
explanation: Some("need plan".to_string()),
|
||||
plan: vec![
|
||||
PlanItemArg {
|
||||
step: "first".to_string(),
|
||||
status: StepStatus::Pending,
|
||||
},
|
||||
PlanItemArg {
|
||||
step: "second".to_string(),
|
||||
status: StepStatus::Completed,
|
||||
},
|
||||
],
|
||||
};
|
||||
|
||||
handle_turn_plan_update("turn-123", update, ApiVersion::V2, &outgoing).await;
|
||||
|
||||
let msg = rx
|
||||
.recv()
|
||||
.await
|
||||
.ok_or_else(|| anyhow!("should send one notification"))?;
|
||||
match msg {
|
||||
OutgoingMessage::AppServerNotification(ServerNotification::TurnPlanUpdated(n)) => {
|
||||
assert_eq!(n.turn_id, "turn-123");
|
||||
assert_eq!(n.explanation.as_deref(), Some("need plan"));
|
||||
assert_eq!(n.plan.len(), 2);
|
||||
assert_eq!(n.plan[0].step, "first");
|
||||
assert_eq!(n.plan[0].status, TurnPlanStepStatus::Pending);
|
||||
assert_eq!(n.plan[1].step, "second");
|
||||
assert_eq!(n.plan[1].status, TurnPlanStepStatus::Completed);
|
||||
}
|
||||
other => bail!("unexpected message: {other:?}"),
|
||||
}
|
||||
assert!(rx.try_recv().is_err(), "no extra messages expected");
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_handle_token_count_event_emits_usage_and_rate_limits() -> Result<()> {
|
||||
let conversation_id = ConversationId::new();
|
||||
@@ -1672,8 +1823,10 @@ mod tests {
|
||||
let (tx, mut rx) = mpsc::channel(CHANNEL_CAPACITY);
|
||||
let outgoing = OutgoingMessageSender::new(tx);
|
||||
let unified_diff = "--- a\n+++ b\n".to_string();
|
||||
let conversation_id = ConversationId::new();
|
||||
|
||||
handle_turn_diff(
|
||||
conversation_id,
|
||||
"turn-1",
|
||||
TurnDiffEvent {
|
||||
unified_diff: unified_diff.clone(),
|
||||
@@ -1691,6 +1844,7 @@ mod tests {
|
||||
OutgoingMessage::AppServerNotification(ServerNotification::TurnDiffUpdated(
|
||||
notification,
|
||||
)) => {
|
||||
assert_eq!(notification.thread_id, conversation_id.to_string());
|
||||
assert_eq!(notification.turn_id, "turn-1");
|
||||
assert_eq!(notification.diff, unified_diff);
|
||||
}
|
||||
@@ -1704,8 +1858,10 @@ mod tests {
|
||||
async fn test_handle_turn_diff_is_noop_for_v1() -> Result<()> {
|
||||
let (tx, mut rx) = mpsc::channel(CHANNEL_CAPACITY);
|
||||
let outgoing = OutgoingMessageSender::new(tx);
|
||||
let conversation_id = ConversationId::new();
|
||||
|
||||
handle_turn_diff(
|
||||
conversation_id,
|
||||
"turn-1",
|
||||
TurnDiffEvent {
|
||||
unified_diff: "diff".to_string(),
|
||||
|
||||
@@ -61,7 +61,9 @@ use codex_app_server_protocol::RemoveConversationSubscriptionResponse;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::ResumeConversationParams;
|
||||
use codex_app_server_protocol::ResumeConversationResponse;
|
||||
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;
|
||||
use codex_app_server_protocol::SandboxMode;
|
||||
use codex_app_server_protocol::SendUserMessageParams;
|
||||
@@ -120,6 +122,7 @@ use codex_core::git_info::git_diff_to_remote;
|
||||
use codex_core::parse_cursor;
|
||||
use codex_core::protocol::EventMsg;
|
||||
use codex_core::protocol::Op;
|
||||
use codex_core::protocol::ReviewDelivery as CoreReviewDelivery;
|
||||
use codex_core::protocol::ReviewRequest;
|
||||
use codex_core::protocol::SessionConfiguredEvent;
|
||||
use codex_core::read_head_for_summary;
|
||||
@@ -253,7 +256,6 @@ impl CodexMessageProcessor {
|
||||
|
||||
fn review_request_from_target(
|
||||
target: ReviewTarget,
|
||||
append_to_original_thread: bool,
|
||||
) -> Result<(ReviewRequest, String), JSONRPCErrorError> {
|
||||
fn invalid_request(message: String) -> JSONRPCErrorError {
|
||||
JSONRPCErrorError {
|
||||
@@ -269,7 +271,6 @@ impl CodexMessageProcessor {
|
||||
ReviewRequest {
|
||||
prompt: "Review the current code changes (staged, unstaged, and untracked files) and provide prioritized findings.".to_string(),
|
||||
user_facing_hint: "current changes".to_string(),
|
||||
append_to_original_thread,
|
||||
},
|
||||
"Review uncommitted changes".to_string(),
|
||||
)),
|
||||
@@ -285,7 +286,6 @@ impl CodexMessageProcessor {
|
||||
ReviewRequest {
|
||||
prompt,
|
||||
user_facing_hint: hint,
|
||||
append_to_original_thread,
|
||||
},
|
||||
display,
|
||||
))
|
||||
@@ -314,7 +314,6 @@ impl CodexMessageProcessor {
|
||||
ReviewRequest {
|
||||
prompt,
|
||||
user_facing_hint: hint,
|
||||
append_to_original_thread,
|
||||
},
|
||||
display,
|
||||
))
|
||||
@@ -328,7 +327,6 @@ impl CodexMessageProcessor {
|
||||
ReviewRequest {
|
||||
prompt: trimmed.clone(),
|
||||
user_facing_hint: trimmed.clone(),
|
||||
append_to_original_thread,
|
||||
},
|
||||
trimmed,
|
||||
))
|
||||
@@ -2497,60 +2495,220 @@ impl CodexMessageProcessor {
|
||||
}
|
||||
}
|
||||
|
||||
async fn review_start(&self, request_id: RequestId, params: ReviewStartParams) {
|
||||
fn build_review_turn(turn_id: String, display_text: &str) -> Turn {
|
||||
let items = if display_text.is_empty() {
|
||||
Vec::new()
|
||||
} else {
|
||||
vec![ThreadItem::UserMessage {
|
||||
id: turn_id.clone(),
|
||||
content: vec![V2UserInput::Text {
|
||||
text: display_text.to_string(),
|
||||
}],
|
||||
}]
|
||||
};
|
||||
|
||||
Turn {
|
||||
id: turn_id,
|
||||
items,
|
||||
status: TurnStatus::InProgress,
|
||||
}
|
||||
}
|
||||
|
||||
async fn emit_review_started(
|
||||
&self,
|
||||
request_id: &RequestId,
|
||||
turn: Turn,
|
||||
parent_thread_id: String,
|
||||
review_thread_id: String,
|
||||
) {
|
||||
let response = ReviewStartResponse {
|
||||
turn: turn.clone(),
|
||||
review_thread_id,
|
||||
};
|
||||
self.outgoing
|
||||
.send_response(request_id.clone(), response)
|
||||
.await;
|
||||
|
||||
let notif = TurnStartedNotification {
|
||||
thread_id: parent_thread_id,
|
||||
turn,
|
||||
};
|
||||
self.outgoing
|
||||
.send_server_notification(ServerNotification::TurnStarted(notif))
|
||||
.await;
|
||||
}
|
||||
|
||||
async fn start_inline_review(
|
||||
&self,
|
||||
request_id: &RequestId,
|
||||
parent_conversation: Arc<CodexConversation>,
|
||||
review_request: ReviewRequest,
|
||||
display_text: &str,
|
||||
parent_thread_id: String,
|
||||
) -> std::result::Result<(), JSONRPCErrorError> {
|
||||
let turn_id = parent_conversation
|
||||
.submit(Op::Review { review_request })
|
||||
.await;
|
||||
|
||||
match turn_id {
|
||||
Ok(turn_id) => {
|
||||
let turn = Self::build_review_turn(turn_id, display_text);
|
||||
self.emit_review_started(
|
||||
request_id,
|
||||
turn,
|
||||
parent_thread_id.clone(),
|
||||
parent_thread_id,
|
||||
)
|
||||
.await;
|
||||
Ok(())
|
||||
}
|
||||
Err(err) => Err(JSONRPCErrorError {
|
||||
code: INTERNAL_ERROR_CODE,
|
||||
message: format!("failed to start review: {err}"),
|
||||
data: None,
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
async fn start_detached_review(
|
||||
&mut self,
|
||||
request_id: &RequestId,
|
||||
parent_conversation_id: ConversationId,
|
||||
review_request: ReviewRequest,
|
||||
display_text: &str,
|
||||
) -> std::result::Result<(), JSONRPCErrorError> {
|
||||
let rollout_path = find_conversation_path_by_id_str(
|
||||
&self.config.codex_home,
|
||||
&parent_conversation_id.to_string(),
|
||||
)
|
||||
.await
|
||||
.map_err(|err| JSONRPCErrorError {
|
||||
code: INTERNAL_ERROR_CODE,
|
||||
message: format!("failed to locate conversation id {parent_conversation_id}: {err}"),
|
||||
data: None,
|
||||
})?
|
||||
.ok_or_else(|| JSONRPCErrorError {
|
||||
code: INVALID_REQUEST_ERROR_CODE,
|
||||
message: format!("no rollout found for conversation id {parent_conversation_id}"),
|
||||
data: None,
|
||||
})?;
|
||||
|
||||
let mut config = self.config.as_ref().clone();
|
||||
config.model = self.config.review_model.clone();
|
||||
|
||||
let NewConversation {
|
||||
conversation_id,
|
||||
conversation,
|
||||
session_configured,
|
||||
..
|
||||
} = self
|
||||
.conversation_manager
|
||||
.fork_conversation(usize::MAX, config, rollout_path)
|
||||
.await
|
||||
.map_err(|err| JSONRPCErrorError {
|
||||
code: INTERNAL_ERROR_CODE,
|
||||
message: format!("error creating detached review conversation: {err}"),
|
||||
data: None,
|
||||
})?;
|
||||
|
||||
if let Err(err) = self
|
||||
.attach_conversation_listener(conversation_id, false, ApiVersion::V2)
|
||||
.await
|
||||
{
|
||||
tracing::warn!(
|
||||
"failed to attach listener for review conversation {}: {}",
|
||||
conversation_id,
|
||||
err.message
|
||||
);
|
||||
}
|
||||
|
||||
let rollout_path = conversation.rollout_path();
|
||||
let fallback_provider = self.config.model_provider_id.as_str();
|
||||
match read_summary_from_rollout(rollout_path.as_path(), fallback_provider).await {
|
||||
Ok(summary) => {
|
||||
let thread = summary_to_thread(summary);
|
||||
let notif = ThreadStartedNotification { thread };
|
||||
self.outgoing
|
||||
.send_server_notification(ServerNotification::ThreadStarted(notif))
|
||||
.await;
|
||||
}
|
||||
Err(err) => {
|
||||
tracing::warn!(
|
||||
"failed to load summary for review conversation {}: {}",
|
||||
session_configured.session_id,
|
||||
err
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
let turn_id = conversation
|
||||
.submit(Op::Review { review_request })
|
||||
.await
|
||||
.map_err(|err| JSONRPCErrorError {
|
||||
code: INTERNAL_ERROR_CODE,
|
||||
message: format!("failed to start detached review turn: {err}"),
|
||||
data: None,
|
||||
})?;
|
||||
|
||||
let turn = Self::build_review_turn(turn_id, display_text);
|
||||
let review_thread_id = conversation_id.to_string();
|
||||
self.emit_review_started(request_id, turn, review_thread_id.clone(), review_thread_id)
|
||||
.await;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn review_start(&mut self, request_id: RequestId, params: ReviewStartParams) {
|
||||
let ReviewStartParams {
|
||||
thread_id,
|
||||
target,
|
||||
append_to_original_thread,
|
||||
delivery,
|
||||
} = params;
|
||||
let (_, conversation) = match self.conversation_from_thread_id(&thread_id).await {
|
||||
Ok(v) => v,
|
||||
Err(error) => {
|
||||
self.outgoing.send_error(request_id, error).await;
|
||||
return;
|
||||
}
|
||||
};
|
||||
|
||||
let (review_request, display_text) =
|
||||
match Self::review_request_from_target(target, append_to_original_thread) {
|
||||
Ok(value) => value,
|
||||
Err(err) => {
|
||||
self.outgoing.send_error(request_id, err).await;
|
||||
let (parent_conversation_id, parent_conversation) =
|
||||
match self.conversation_from_thread_id(&thread_id).await {
|
||||
Ok(v) => v,
|
||||
Err(error) => {
|
||||
self.outgoing.send_error(request_id, error).await;
|
||||
return;
|
||||
}
|
||||
};
|
||||
|
||||
let turn_id = conversation.submit(Op::Review { review_request }).await;
|
||||
|
||||
match turn_id {
|
||||
Ok(turn_id) => {
|
||||
let mut items = Vec::new();
|
||||
if !display_text.is_empty() {
|
||||
items.push(ThreadItem::UserMessage {
|
||||
id: turn_id.clone(),
|
||||
content: vec![V2UserInput::Text { text: display_text }],
|
||||
});
|
||||
}
|
||||
let turn = Turn {
|
||||
id: turn_id.clone(),
|
||||
items,
|
||||
status: TurnStatus::InProgress,
|
||||
};
|
||||
let response = TurnStartResponse { turn: turn.clone() };
|
||||
self.outgoing.send_response(request_id, response).await;
|
||||
|
||||
let notif = TurnStartedNotification { thread_id, turn };
|
||||
self.outgoing
|
||||
.send_server_notification(ServerNotification::TurnStarted(notif))
|
||||
.await;
|
||||
}
|
||||
let (review_request, display_text) = match Self::review_request_from_target(target) {
|
||||
Ok(value) => value,
|
||||
Err(err) => {
|
||||
let error = JSONRPCErrorError {
|
||||
code: INTERNAL_ERROR_CODE,
|
||||
message: format!("failed to start review: {err}"),
|
||||
data: None,
|
||||
};
|
||||
self.outgoing.send_error(request_id, error).await;
|
||||
self.outgoing.send_error(request_id, err).await;
|
||||
return;
|
||||
}
|
||||
};
|
||||
|
||||
let delivery = delivery.unwrap_or(ApiReviewDelivery::Inline).to_core();
|
||||
match delivery {
|
||||
CoreReviewDelivery::Inline => {
|
||||
if let Err(err) = self
|
||||
.start_inline_review(
|
||||
&request_id,
|
||||
parent_conversation,
|
||||
review_request,
|
||||
display_text.as_str(),
|
||||
thread_id.clone(),
|
||||
)
|
||||
.await
|
||||
{
|
||||
self.outgoing.send_error(request_id, err).await;
|
||||
}
|
||||
}
|
||||
CoreReviewDelivery::Detached => {
|
||||
if let Err(err) = self
|
||||
.start_detached_review(
|
||||
&request_id,
|
||||
parent_conversation_id,
|
||||
review_request,
|
||||
display_text.as_str(),
|
||||
)
|
||||
.await
|
||||
{
|
||||
self.outgoing.send_error(request_id, err).await;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -15,6 +15,7 @@ pub use mcp_process::McpProcess;
|
||||
pub use mock_model_server::create_mock_chat_completions_server;
|
||||
pub use mock_model_server::create_mock_chat_completions_server_unchecked;
|
||||
pub use responses::create_apply_patch_sse_response;
|
||||
pub use responses::create_exec_command_sse_response;
|
||||
pub use responses::create_final_assistant_message_sse_response;
|
||||
pub use responses::create_shell_command_sse_response;
|
||||
pub use rollout::create_fake_rollout;
|
||||
|
||||
@@ -94,3 +94,42 @@ pub fn create_apply_patch_sse_response(
|
||||
);
|
||||
Ok(sse)
|
||||
}
|
||||
|
||||
pub fn create_exec_command_sse_response(call_id: &str) -> anyhow::Result<String> {
|
||||
let (cmd, args) = if cfg!(windows) {
|
||||
("cmd.exe", vec!["/d", "/c", "echo hi"])
|
||||
} else {
|
||||
("/bin/sh", vec!["-c", "echo hi"])
|
||||
};
|
||||
let command = std::iter::once(cmd.to_string())
|
||||
.chain(args.into_iter().map(str::to_string))
|
||||
.collect::<Vec<_>>();
|
||||
let tool_call_arguments = serde_json::to_string(&json!({
|
||||
"cmd": command.join(" "),
|
||||
"yield_time_ms": 500
|
||||
}))?;
|
||||
let tool_call = json!({
|
||||
"choices": [
|
||||
{
|
||||
"delta": {
|
||||
"tool_calls": [
|
||||
{
|
||||
"id": call_id,
|
||||
"function": {
|
||||
"name": "exec_command",
|
||||
"arguments": tool_call_arguments
|
||||
}
|
||||
}
|
||||
]
|
||||
},
|
||||
"finish_reason": "tool_calls"
|
||||
}
|
||||
]
|
||||
});
|
||||
|
||||
let sse = format!(
|
||||
"data: {}\n\ndata: DONE\n\n",
|
||||
serde_json::to_string(&tool_call)?
|
||||
);
|
||||
Ok(sse)
|
||||
}
|
||||
|
||||
@@ -9,12 +9,13 @@ use codex_app_server_protocol::JSONRPCError;
|
||||
use codex_app_server_protocol::JSONRPCNotification;
|
||||
use codex_app_server_protocol::JSONRPCResponse;
|
||||
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::ThreadItem;
|
||||
use codex_app_server_protocol::ThreadStartParams;
|
||||
use codex_app_server_protocol::ThreadStartResponse;
|
||||
use codex_app_server_protocol::TurnStartResponse;
|
||||
use codex_app_server_protocol::TurnStatus;
|
||||
use serde_json::json;
|
||||
use tempfile::TempDir;
|
||||
@@ -59,7 +60,7 @@ async fn review_start_runs_review_turn_and_emits_code_review_item() -> Result<()
|
||||
let review_req = mcp
|
||||
.send_review_start_request(ReviewStartParams {
|
||||
thread_id: thread_id.clone(),
|
||||
append_to_original_thread: true,
|
||||
delivery: Some(ReviewDelivery::Inline),
|
||||
target: ReviewTarget::Commit {
|
||||
sha: "1234567deadbeef".to_string(),
|
||||
title: Some("Tidy UI colors".to_string()),
|
||||
@@ -71,43 +72,43 @@ async fn review_start_runs_review_turn_and_emits_code_review_item() -> Result<()
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(review_req)),
|
||||
)
|
||||
.await??;
|
||||
let TurnStartResponse { turn } = to_response::<TurnStartResponse>(review_resp)?;
|
||||
let ReviewStartResponse {
|
||||
turn,
|
||||
review_thread_id,
|
||||
} = to_response::<ReviewStartResponse>(review_resp)?;
|
||||
assert_eq!(review_thread_id, thread_id.clone());
|
||||
let turn_id = turn.id.clone();
|
||||
assert_eq!(turn.status, TurnStatus::InProgress);
|
||||
assert_eq!(turn.items.len(), 1);
|
||||
match &turn.items[0] {
|
||||
ThreadItem::UserMessage { content, .. } => {
|
||||
assert_eq!(content.len(), 1);
|
||||
assert!(matches!(
|
||||
&content[0],
|
||||
codex_app_server_protocol::UserInput::Text { .. }
|
||||
));
|
||||
}
|
||||
other => panic!("expected user message, got {other:?}"),
|
||||
}
|
||||
|
||||
let _started: JSONRPCNotification = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("turn/started"),
|
||||
)
|
||||
.await??;
|
||||
let item_started: JSONRPCNotification = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("item/started"),
|
||||
)
|
||||
.await??;
|
||||
let started: ItemStartedNotification =
|
||||
serde_json::from_value(item_started.params.expect("params must be present"))?;
|
||||
match started.item {
|
||||
ThreadItem::CodeReview { id, review } => {
|
||||
assert_eq!(id, turn_id);
|
||||
assert_eq!(review, "commit 1234567");
|
||||
// Confirm we see the EnteredReviewMode marker on the main thread.
|
||||
let mut saw_entered_review_mode = false;
|
||||
for _ in 0..10 {
|
||||
let item_started: JSONRPCNotification = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("item/started"),
|
||||
)
|
||||
.await??;
|
||||
let started: ItemStartedNotification =
|
||||
serde_json::from_value(item_started.params.expect("params must be present"))?;
|
||||
match started.item {
|
||||
ThreadItem::EnteredReviewMode { id, review } => {
|
||||
assert_eq!(id, turn_id);
|
||||
assert_eq!(review, "commit 1234567");
|
||||
saw_entered_review_mode = true;
|
||||
break;
|
||||
}
|
||||
_ => continue,
|
||||
}
|
||||
other => panic!("expected code review item, got {other:?}"),
|
||||
}
|
||||
assert!(
|
||||
saw_entered_review_mode,
|
||||
"did not observe enteredReviewMode item"
|
||||
);
|
||||
|
||||
// Confirm we see the ExitedReviewMode marker (with review text)
|
||||
// on the same turn. Ignore any other items the stream surfaces.
|
||||
let mut review_body: Option<String> = None;
|
||||
for _ in 0..5 {
|
||||
for _ in 0..10 {
|
||||
let review_notif: JSONRPCNotification = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("item/completed"),
|
||||
@@ -116,13 +117,12 @@ async fn review_start_runs_review_turn_and_emits_code_review_item() -> Result<()
|
||||
let completed: ItemCompletedNotification =
|
||||
serde_json::from_value(review_notif.params.expect("params must be present"))?;
|
||||
match completed.item {
|
||||
ThreadItem::CodeReview { id, review } => {
|
||||
ThreadItem::ExitedReviewMode { id, review } => {
|
||||
assert_eq!(id, turn_id);
|
||||
review_body = Some(review);
|
||||
break;
|
||||
}
|
||||
ThreadItem::UserMessage { .. } => continue,
|
||||
other => panic!("unexpected item/completed payload: {other:?}"),
|
||||
_ => continue,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -146,7 +146,7 @@ async fn review_start_rejects_empty_base_branch() -> Result<()> {
|
||||
let request_id = mcp
|
||||
.send_review_start_request(ReviewStartParams {
|
||||
thread_id,
|
||||
append_to_original_thread: true,
|
||||
delivery: Some(ReviewDelivery::Inline),
|
||||
target: ReviewTarget::BaseBranch {
|
||||
branch: " ".to_string(),
|
||||
},
|
||||
@@ -167,6 +167,56 @@ async fn review_start_rejects_empty_base_branch() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn review_start_with_detached_delivery_returns_new_thread_id() -> Result<()> {
|
||||
let review_payload = json!({
|
||||
"findings": [],
|
||||
"overall_correctness": "ok",
|
||||
"overall_explanation": "detached review",
|
||||
"overall_confidence_score": 0.5
|
||||
})
|
||||
.to_string();
|
||||
let responses = vec![create_final_assistant_message_sse_response(
|
||||
&review_payload,
|
||||
)?];
|
||||
let server = create_mock_chat_completions_server_unchecked(responses).await;
|
||||
|
||||
let codex_home = TempDir::new()?;
|
||||
create_config_toml(codex_home.path(), &server.uri())?;
|
||||
|
||||
let mut mcp = McpProcess::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let thread_id = start_default_thread(&mut mcp).await?;
|
||||
|
||||
let review_req = 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 review_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(review_req)),
|
||||
)
|
||||
.await??;
|
||||
let ReviewStartResponse {
|
||||
turn,
|
||||
review_thread_id,
|
||||
} = to_response::<ReviewStartResponse>(review_resp)?;
|
||||
|
||||
assert_eq!(turn.status, TurnStatus::InProgress);
|
||||
assert_ne!(
|
||||
review_thread_id, thread_id,
|
||||
"detached review should run on a different thread"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn review_start_rejects_empty_commit_sha() -> Result<()> {
|
||||
let server = create_mock_chat_completions_server_unchecked(vec![]).await;
|
||||
@@ -180,7 +230,7 @@ async fn review_start_rejects_empty_commit_sha() -> Result<()> {
|
||||
let request_id = mcp
|
||||
.send_review_start_request(ReviewStartParams {
|
||||
thread_id,
|
||||
append_to_original_thread: true,
|
||||
delivery: Some(ReviewDelivery::Inline),
|
||||
target: ReviewTarget::Commit {
|
||||
sha: "\t".to_string(),
|
||||
title: None,
|
||||
@@ -215,7 +265,7 @@ async fn review_start_rejects_empty_custom_instructions() -> Result<()> {
|
||||
let request_id = mcp
|
||||
.send_review_start_request(ReviewStartParams {
|
||||
thread_id,
|
||||
append_to_original_thread: true,
|
||||
delivery: Some(ReviewDelivery::Inline),
|
||||
target: ReviewTarget::Custom {
|
||||
instructions: "\n\n".to_string(),
|
||||
},
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
use anyhow::Result;
|
||||
use app_test_support::McpProcess;
|
||||
use app_test_support::create_apply_patch_sse_response;
|
||||
use app_test_support::create_exec_command_sse_response;
|
||||
use app_test_support::create_final_assistant_message_sse_response;
|
||||
use app_test_support::create_mock_chat_completions_server;
|
||||
use app_test_support::create_mock_chat_completions_server_unchecked;
|
||||
@@ -10,6 +11,7 @@ use app_test_support::to_response;
|
||||
use codex_app_server_protocol::ApprovalDecision;
|
||||
use codex_app_server_protocol::CommandExecutionRequestApprovalResponse;
|
||||
use codex_app_server_protocol::CommandExecutionStatus;
|
||||
use codex_app_server_protocol::FileChangeOutputDeltaNotification;
|
||||
use codex_app_server_protocol::FileChangeRequestApprovalResponse;
|
||||
use codex_app_server_protocol::ItemCompletedNotification;
|
||||
use codex_app_server_protocol::ItemStartedNotification;
|
||||
@@ -627,10 +629,19 @@ async fn turn_start_file_change_approval_v2() -> Result<()> {
|
||||
*** Add File: README.md
|
||||
+new line
|
||||
*** End Patch
|
||||
"#;
|
||||
let update_patch = r#"*** Begin Patch
|
||||
*** Update File: README.md
|
||||
@@
|
||||
-new line
|
||||
+updated line
|
||||
*** End Patch
|
||||
"#;
|
||||
let responses = vec![
|
||||
create_apply_patch_sse_response(patch, "patch-call")?,
|
||||
create_final_assistant_message_sse_response("patch applied")?,
|
||||
create_apply_patch_sse_response(update_patch, "patch-call-2")?,
|
||||
create_final_assistant_message_sse_response("patch updated")?,
|
||||
];
|
||||
let server = create_mock_chat_completions_server(responses).await;
|
||||
create_config_toml(&codex_home, &server.uri(), "untrusted")?;
|
||||
@@ -705,8 +716,8 @@ async fn turn_start_file_change_approval_v2() -> Result<()> {
|
||||
assert_eq!(params.item_id, "patch-call");
|
||||
assert_eq!(params.thread_id, thread.id);
|
||||
assert_eq!(params.turn_id, turn.id);
|
||||
let expected_readme_path = workspace.join("README.md");
|
||||
let expected_readme_path = expected_readme_path.to_string_lossy().into_owned();
|
||||
let readme_path = workspace.join("README.md");
|
||||
let expected_readme_path = readme_path.to_string_lossy().into_owned();
|
||||
pretty_assertions::assert_eq!(
|
||||
started_changes,
|
||||
vec![codex_app_server_protocol::FileUpdateChange {
|
||||
@@ -724,6 +735,26 @@ async fn turn_start_file_change_approval_v2() -> Result<()> {
|
||||
)
|
||||
.await?;
|
||||
|
||||
let output_delta_notif = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("item/fileChange/outputDelta"),
|
||||
)
|
||||
.await??;
|
||||
let output_delta: FileChangeOutputDeltaNotification = serde_json::from_value(
|
||||
output_delta_notif
|
||||
.params
|
||||
.clone()
|
||||
.expect("item/fileChange/outputDelta params"),
|
||||
)?;
|
||||
assert_eq!(output_delta.thread_id, thread.id);
|
||||
assert_eq!(output_delta.turn_id, turn.id);
|
||||
assert_eq!(output_delta.item_id, "patch-call");
|
||||
assert!(
|
||||
!output_delta.delta.is_empty(),
|
||||
"expected delta to be non-empty, got: {}",
|
||||
output_delta.delta
|
||||
);
|
||||
|
||||
let completed_file_change = timeout(DEFAULT_READ_TIMEOUT, async {
|
||||
loop {
|
||||
let completed_notif = mcp
|
||||
@@ -753,9 +784,134 @@ async fn turn_start_file_change_approval_v2() -> Result<()> {
|
||||
)
|
||||
.await??;
|
||||
|
||||
let readme_contents = std::fs::read_to_string(expected_readme_path)?;
|
||||
let readme_contents = std::fs::read_to_string(&readme_path)?;
|
||||
assert_eq!(readme_contents, "new line\n");
|
||||
|
||||
let second_turn_req = mcp
|
||||
.send_turn_start_request(TurnStartParams {
|
||||
thread_id: thread.id.clone(),
|
||||
input: vec![V2UserInput::Text {
|
||||
text: "update patch".into(),
|
||||
}],
|
||||
approval_policy: Some(codex_app_server_protocol::AskForApproval::Never),
|
||||
sandbox_policy: Some(codex_app_server_protocol::SandboxPolicy::DangerFullAccess),
|
||||
model: Some("mock-model".to_string()),
|
||||
effort: Some(ReasoningEffort::Medium),
|
||||
summary: Some(ReasoningSummary::Auto),
|
||||
cwd: Some(workspace.clone()),
|
||||
})
|
||||
.await?;
|
||||
let second_turn_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(second_turn_req)),
|
||||
)
|
||||
.await??;
|
||||
let TurnStartResponse { turn: second_turn } =
|
||||
to_response::<TurnStartResponse>(second_turn_resp)?;
|
||||
|
||||
let started_update_file_change = timeout(DEFAULT_READ_TIMEOUT, async {
|
||||
loop {
|
||||
let started_notif = mcp
|
||||
.read_stream_until_notification_message("item/started")
|
||||
.await?;
|
||||
let started: ItemStartedNotification =
|
||||
serde_json::from_value(started_notif.params.clone().expect("item/started params"))?;
|
||||
if let ThreadItem::FileChange { .. } = started.item {
|
||||
return Ok::<ThreadItem, anyhow::Error>(started.item);
|
||||
}
|
||||
}
|
||||
})
|
||||
.await??;
|
||||
let ThreadItem::FileChange {
|
||||
ref id,
|
||||
status,
|
||||
ref changes,
|
||||
} = started_update_file_change
|
||||
else {
|
||||
unreachable!("loop ensures we break on file change items");
|
||||
};
|
||||
assert_eq!(id, "patch-call-2");
|
||||
assert_eq!(status, PatchApplyStatus::InProgress);
|
||||
let update_changes = changes.clone();
|
||||
let update_change = update_changes
|
||||
.first()
|
||||
.expect("expected a single update change");
|
||||
assert_eq!(update_change.path, expected_readme_path);
|
||||
match &update_change.kind {
|
||||
PatchChangeKind::Update {
|
||||
move_path,
|
||||
old_content,
|
||||
new_content,
|
||||
} => {
|
||||
assert!(move_path.is_none());
|
||||
assert_eq!(old_content, "new line\n");
|
||||
assert_eq!(new_content, "updated line\n");
|
||||
}
|
||||
other => panic!("expected PatchChangeKind::Update, got: {other:?}"),
|
||||
}
|
||||
assert!(
|
||||
update_change.diff.contains("-new line"),
|
||||
"expected diff to include removal of original line, got: {}",
|
||||
update_change.diff
|
||||
);
|
||||
assert!(
|
||||
update_change.diff.contains("+updated line"),
|
||||
"expected diff to include addition of updated line, got: {}",
|
||||
update_change.diff
|
||||
);
|
||||
|
||||
let output_delta_notif = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("item/fileChange/outputDelta"),
|
||||
)
|
||||
.await??;
|
||||
let output_delta: FileChangeOutputDeltaNotification = serde_json::from_value(
|
||||
output_delta_notif
|
||||
.params
|
||||
.clone()
|
||||
.expect("item/fileChange/outputDelta params"),
|
||||
)?;
|
||||
assert_eq!(output_delta.thread_id, thread.id);
|
||||
assert_eq!(output_delta.turn_id, second_turn.id);
|
||||
assert_eq!(output_delta.item_id, "patch-call-2");
|
||||
assert!(
|
||||
!output_delta.delta.is_empty(),
|
||||
"expected delta to be non-empty, got: {}",
|
||||
output_delta.delta
|
||||
);
|
||||
|
||||
let completed_file_change = timeout(DEFAULT_READ_TIMEOUT, async {
|
||||
loop {
|
||||
let completed_notif = mcp
|
||||
.read_stream_until_notification_message("item/completed")
|
||||
.await?;
|
||||
let completed: ItemCompletedNotification = serde_json::from_value(
|
||||
completed_notif
|
||||
.params
|
||||
.clone()
|
||||
.expect("item/completed params"),
|
||||
)?;
|
||||
if let ThreadItem::FileChange { .. } = completed.item {
|
||||
return Ok::<ThreadItem, anyhow::Error>(completed.item);
|
||||
}
|
||||
}
|
||||
})
|
||||
.await??;
|
||||
let ThreadItem::FileChange { ref id, status, .. } = completed_file_change else {
|
||||
unreachable!("loop ensures we break on file change items");
|
||||
};
|
||||
assert_eq!(id, "patch-call-2");
|
||||
assert_eq!(status, PatchApplyStatus::Completed);
|
||||
|
||||
timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("codex/event/task_complete"),
|
||||
)
|
||||
.await??;
|
||||
|
||||
let updated_readme_contents = std::fs::read_to_string(&readme_path)?;
|
||||
assert_eq!(updated_readme_contents, "updated line\n");
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -907,6 +1063,134 @@ async fn turn_start_file_change_approval_decline_v2() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[cfg_attr(windows, ignore = "process id reporting differs on Windows")]
|
||||
async fn command_execution_notifications_include_process_id() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let responses = vec![
|
||||
create_exec_command_sse_response("uexec-1")?,
|
||||
create_final_assistant_message_sse_response("done")?,
|
||||
];
|
||||
let server = create_mock_chat_completions_server(responses).await;
|
||||
let codex_home = TempDir::new()?;
|
||||
create_config_toml(codex_home.path(), &server.uri(), "never")?;
|
||||
let config_toml = codex_home.path().join("config.toml");
|
||||
let mut config_contents = std::fs::read_to_string(&config_toml)?;
|
||||
config_contents.push_str(
|
||||
r#"
|
||||
[features]
|
||||
unified_exec = true
|
||||
"#,
|
||||
);
|
||||
std::fs::write(&config_toml, config_contents)?;
|
||||
|
||||
let mut mcp = McpProcess::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let start_id = mcp
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
model: Some("mock-model".to_string()),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let start_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(start_id)),
|
||||
)
|
||||
.await??;
|
||||
let ThreadStartResponse { thread, .. } = to_response::<ThreadStartResponse>(start_resp)?;
|
||||
|
||||
let turn_id = mcp
|
||||
.send_turn_start_request(TurnStartParams {
|
||||
thread_id: thread.id.clone(),
|
||||
input: vec![V2UserInput::Text {
|
||||
text: "run a command".to_string(),
|
||||
}],
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let turn_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(turn_id)),
|
||||
)
|
||||
.await??;
|
||||
let TurnStartResponse { turn: _turn } = to_response::<TurnStartResponse>(turn_resp)?;
|
||||
|
||||
let started_command = timeout(DEFAULT_READ_TIMEOUT, async {
|
||||
loop {
|
||||
let notif = mcp
|
||||
.read_stream_until_notification_message("item/started")
|
||||
.await?;
|
||||
let started: ItemStartedNotification = serde_json::from_value(
|
||||
notif
|
||||
.params
|
||||
.clone()
|
||||
.expect("item/started should include params"),
|
||||
)?;
|
||||
if let ThreadItem::CommandExecution { .. } = started.item {
|
||||
return Ok::<ThreadItem, anyhow::Error>(started.item);
|
||||
}
|
||||
}
|
||||
})
|
||||
.await??;
|
||||
let ThreadItem::CommandExecution {
|
||||
id,
|
||||
process_id: started_process_id,
|
||||
status,
|
||||
..
|
||||
} = started_command
|
||||
else {
|
||||
unreachable!("loop ensures we break on command execution items");
|
||||
};
|
||||
assert_eq!(id, "uexec-1");
|
||||
assert_eq!(status, CommandExecutionStatus::InProgress);
|
||||
let started_process_id = started_process_id.expect("process id should be present");
|
||||
|
||||
let completed_command = timeout(DEFAULT_READ_TIMEOUT, async {
|
||||
loop {
|
||||
let notif = mcp
|
||||
.read_stream_until_notification_message("item/completed")
|
||||
.await?;
|
||||
let completed: ItemCompletedNotification = serde_json::from_value(
|
||||
notif
|
||||
.params
|
||||
.clone()
|
||||
.expect("item/completed should include params"),
|
||||
)?;
|
||||
if let ThreadItem::CommandExecution { .. } = completed.item {
|
||||
return Ok::<ThreadItem, anyhow::Error>(completed.item);
|
||||
}
|
||||
}
|
||||
})
|
||||
.await??;
|
||||
let ThreadItem::CommandExecution {
|
||||
id: completed_id,
|
||||
process_id: completed_process_id,
|
||||
status: completed_status,
|
||||
exit_code,
|
||||
..
|
||||
} = completed_command
|
||||
else {
|
||||
unreachable!("loop ensures we break on command execution items");
|
||||
};
|
||||
assert_eq!(completed_id, "uexec-1");
|
||||
assert_eq!(completed_status, CommandExecutionStatus::Completed);
|
||||
assert_eq!(exit_code, Some(0));
|
||||
assert_eq!(
|
||||
completed_process_id.as_deref(),
|
||||
Some(started_process_id.as_str())
|
||||
);
|
||||
|
||||
timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("turn/completed"),
|
||||
)
|
||||
.await??;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
// Helper to create a config.toml pointing at the mock model server.
|
||||
fn create_config_toml(
|
||||
codex_home: &Path,
|
||||
|
||||
@@ -194,6 +194,8 @@ pub enum ApplyPatchFileChange {
|
||||
move_path: Option<PathBuf>,
|
||||
/// new_content that will result after the unified_diff is applied.
|
||||
new_content: String,
|
||||
/// original content before the diff was applied.
|
||||
old_content: String,
|
||||
},
|
||||
}
|
||||
|
||||
@@ -329,6 +331,7 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp
|
||||
let ApplyPatchFileUpdate {
|
||||
unified_diff,
|
||||
content: contents,
|
||||
old_content,
|
||||
} = match unified_diff_from_chunks(&path, &chunks) {
|
||||
Ok(diff) => diff,
|
||||
Err(e) => {
|
||||
@@ -341,6 +344,7 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp
|
||||
unified_diff,
|
||||
move_path: move_path.map(|p| effective_cwd.join(p)),
|
||||
new_content: contents,
|
||||
old_content,
|
||||
},
|
||||
);
|
||||
}
|
||||
@@ -846,6 +850,7 @@ fn apply_replacements(
|
||||
pub struct ApplyPatchFileUpdate {
|
||||
unified_diff: String,
|
||||
content: String,
|
||||
old_content: String,
|
||||
}
|
||||
|
||||
pub fn unified_diff_from_chunks(
|
||||
@@ -869,6 +874,7 @@ pub fn unified_diff_from_chunks_with_context(
|
||||
Ok(ApplyPatchFileUpdate {
|
||||
unified_diff,
|
||||
content: new_contents,
|
||||
old_content: original_contents,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -1467,6 +1473,7 @@ PATCH"#,
|
||||
let expected = ApplyPatchFileUpdate {
|
||||
unified_diff: expected_diff.to_string(),
|
||||
content: "foo\nBAR\nbaz\nQUX\n".to_string(),
|
||||
old_content: "foo\nbar\nbaz\nqux\n".to_string(),
|
||||
};
|
||||
assert_eq!(expected, diff);
|
||||
}
|
||||
@@ -1503,6 +1510,7 @@ PATCH"#,
|
||||
let expected = ApplyPatchFileUpdate {
|
||||
unified_diff: expected_diff.to_string(),
|
||||
content: "FOO\nbar\nbaz\n".to_string(),
|
||||
old_content: "foo\nbar\nbaz\n".to_string(),
|
||||
};
|
||||
assert_eq!(expected, diff);
|
||||
}
|
||||
@@ -1540,6 +1548,7 @@ PATCH"#,
|
||||
let expected = ApplyPatchFileUpdate {
|
||||
unified_diff: expected_diff.to_string(),
|
||||
content: "foo\nbar\nBAZ\n".to_string(),
|
||||
old_content: "foo\nbar\nbaz\n".to_string(),
|
||||
};
|
||||
assert_eq!(expected, diff);
|
||||
}
|
||||
@@ -1574,6 +1583,7 @@ PATCH"#,
|
||||
let expected = ApplyPatchFileUpdate {
|
||||
unified_diff: expected_diff.to_string(),
|
||||
content: "foo\nbar\nbaz\nquux\n".to_string(),
|
||||
old_content: "foo\nbar\nbaz\n".to_string(),
|
||||
};
|
||||
assert_eq!(expected, diff);
|
||||
}
|
||||
@@ -1629,6 +1639,7 @@ PATCH"#,
|
||||
let expected = ApplyPatchFileUpdate {
|
||||
unified_diff: expected_diff.to_string(),
|
||||
content: "a\nB\nc\nd\nE\nf\ng\n".to_string(),
|
||||
old_content: "a\nb\nc\nd\ne\nf\n".to_string(),
|
||||
};
|
||||
|
||||
assert_eq!(expected, diff);
|
||||
@@ -1688,6 +1699,7 @@ g
|
||||
.to_string(),
|
||||
move_path: None,
|
||||
new_content: "updated session directory content\n".to_string(),
|
||||
old_content: "session directory content\n".to_string(),
|
||||
},
|
||||
)]),
|
||||
patch: argv[1].clone(),
|
||||
|
||||
@@ -1,206 +0,0 @@
|
||||
# Client Extraction Plan
|
||||
|
||||
## Goals
|
||||
- Split the HTTP transport/client code out of `codex-core` into a reusable crate that is agnostic of Codex/OpenAI business logic and API schemas.
|
||||
- Create a separate API library crate that houses typed requests/responses for well-known APIs (Responses, Chat Completions, Compact) and plugs into the transport crate via minimal traits.
|
||||
- Preserve current behaviour (auth headers, retries, SSE handling, rate-limit parsing, compaction, fixtures) while making the APIs symmetric and avoiding code duplication.
|
||||
- Keep existing consumers (`codex-core`, tests, and tools) stable by providing a small compatibility layer during the transition.
|
||||
|
||||
## Snapshot of Today
|
||||
- `core/src/client.rs (ModelClient)` owns config/auth/session state, chooses wire API, builds payloads, drives retries, parses SSE, compaction, and rate-limit headers.
|
||||
- `core/src/chat_completions.rs` implements the Chat Completions call + SSE parser + aggregation helper.
|
||||
- `core/src/client_common.rs` holds `Prompt`, tool specs, shared request structs (`ResponsesApiRequest`, `TextControls`), and `ResponseEvent`/`ResponseStream`.
|
||||
- `core/src/default_client.rs` wraps `reqwest` with Codex UA/originator defaults.
|
||||
- `core/src/model_provider_info.rs` models providers (base URL, headers, env keys, retry/timeout tuning) and builds `CodexRequestBuilder`s.
|
||||
- Current retry logic is co-located with API handling; streaming SSE parsing is duplicated across Responses/Chat.
|
||||
|
||||
## Target Crates (with interfaces)
|
||||
|
||||
- `codex-client` (generic transport)
|
||||
- Owns the generic HTTP machinery: a `CodexHttpClient`/`CodexRequestBuilder`-style wrapper, retry/backoff hooks, streaming connector (SSE framing + idle timeout), header injection, and optional telemetry callbacks.
|
||||
- Does **not** know about OpenAI/Codex-specific paths, headers, or error codes; it only exposes HTTP-level concepts (status, headers, bodies, connection errors).
|
||||
- Minimal surface:
|
||||
```rust
|
||||
pub trait HttpTransport {
|
||||
fn execute(&self, req: Request) -> Result<Response, TransportError>;
|
||||
fn stream(&self, req: Request) -> Result<ByteStream, TransportError>;
|
||||
}
|
||||
|
||||
pub struct Request {
|
||||
pub method: Method,
|
||||
pub url: String,
|
||||
pub headers: HeaderMap,
|
||||
pub body: Option<serde_json::Value>,
|
||||
pub timeout: Option<Duration>,
|
||||
}
|
||||
```
|
||||
- Generic client traits (request/response/chunk are abstract over the transport):
|
||||
```rust
|
||||
#[async_trait::async_trait]
|
||||
pub trait UnaryClient<Req, Resp> {
|
||||
async fn run(&self, req: Req) -> Result<Resp, TransportError>;
|
||||
}
|
||||
|
||||
#[async_trait::async_trait]
|
||||
pub trait StreamClient<Req, Chunk> {
|
||||
async fn run(&self, req: Req) -> Result<ResponseStream<Chunk>, TransportError>;
|
||||
}
|
||||
|
||||
pub struct RetryPolicy {
|
||||
pub max_attempts: u64,
|
||||
pub base_delay: Duration,
|
||||
pub retry_on: RetryOn, // e.g., transport errors + 429/5xx
|
||||
}
|
||||
```
|
||||
- `RetryOn` lives in `codex-client` and captures HTTP status classes and transport failures that qualify for retry.
|
||||
- Implementations in `codex-api` plug in their own request types, parsers, and retry policies while reusing the transport’s backoff and error types.
|
||||
- Planned runtime helper:
|
||||
```rust
|
||||
pub async fn run_with_retry<T, F, Fut>(
|
||||
policy: RetryPolicy,
|
||||
make_req: impl Fn() -> Request,
|
||||
op: F,
|
||||
) -> Result<T, TransportError>
|
||||
where
|
||||
F: Fn(Request) -> Fut,
|
||||
Fut: Future<Output = Result<T, TransportError>>,
|
||||
{
|
||||
for attempt in 0..=policy.max_attempts {
|
||||
let req = make_req();
|
||||
match op(req).await {
|
||||
Ok(resp) => return Ok(resp),
|
||||
Err(err) if policy.retry_on.should_retry(&err, attempt) => {
|
||||
tokio::time::sleep(backoff(policy.base_delay, attempt + 1)).await;
|
||||
}
|
||||
Err(err) => return Err(err),
|
||||
}
|
||||
}
|
||||
Err(TransportError::RetryLimit)
|
||||
}
|
||||
```
|
||||
- Unary clients wrap `transport.execute` with this helper and then deserialize.
|
||||
- Stream clients wrap the **initial** `transport.stream` call with this helper. Mid-stream disconnects are surfaced as `StreamError`s; automatic resume/reconnect can be added later on top of this primitive if we introduce cursor support.
|
||||
- Common helpers: `retry::backoff(attempt)`, `errors::{TransportError, StreamError}`.
|
||||
- Streaming utility (SSE framing only):
|
||||
```rust
|
||||
pub fn sse_stream<S>(
|
||||
bytes: S,
|
||||
idle_timeout: Duration,
|
||||
tx: mpsc::Sender<Result<String, StreamError>>,
|
||||
telemetry: Option<Box<dyn Telemetry>>,
|
||||
)
|
||||
where
|
||||
S: Stream<Item = Result<Bytes, TransportError>> + Unpin + Send + 'static;
|
||||
```
|
||||
- `sse_stream` is responsible for timeouts, connection-level errors, and emitting raw `data:` chunks as UTF-8 strings; parsing those strings into structured events is done in `codex-api`.
|
||||
|
||||
- `codex-api` (OpenAI/Codex API library)
|
||||
- Owns typed models for Responses/Chat/Compact plus shared helpers (`Prompt`, tool specs, text controls, `ResponsesApiRequest`, etc.).
|
||||
- Knows about OpenAI/Codex semantics:
|
||||
- URL shapes (`/v1/responses`, `/v1/chat/completions`, `/responses/compact`).
|
||||
- Provider configuration (`WireApi`, base URLs, query params, per-provider retry knobs).
|
||||
- Rate-limit headers (`x-codex-*`) and their mapping into `RateLimitSnapshot` / `CreditsSnapshot`.
|
||||
- Error body formats (`{ error: { type, code, message, plan_type, resets_at } }`) and how they become API errors (context window exceeded, quota/usage limit, etc.).
|
||||
- SSE event names (`response.output_item.done`, `response.completed`, `response.failed`, etc.) and their mapping into high-level events.
|
||||
- Provides a provider abstraction (conceptually similar to `ModelProviderInfo`):
|
||||
```rust
|
||||
pub struct Provider {
|
||||
pub name: String,
|
||||
pub base_url: String,
|
||||
pub wire: WireApi, // Responses | Chat
|
||||
pub headers: HeaderMap,
|
||||
pub retry: RetryConfig,
|
||||
pub stream_idle_timeout: Duration,
|
||||
}
|
||||
|
||||
pub trait AuthProvider {
|
||||
/// Returns a bearer token to use for this request (if any).
|
||||
/// Implementations are expected to be cheap and to surface already-refreshed tokens;
|
||||
/// higher layers (`codex-core`) remain responsible for token refresh flows.
|
||||
fn bearer_token(&self) -> Option<String>;
|
||||
|
||||
/// Optional ChatGPT account id header for Chat mode.
|
||||
fn account_id(&self) -> Option<String>;
|
||||
}
|
||||
```
|
||||
- Ready-made clients built on `HttpTransport`:
|
||||
```rust
|
||||
pub struct ResponsesClient<T: HttpTransport, A: AuthProvider> { /* ... */ }
|
||||
impl<T, A> ResponsesClient<T, A> {
|
||||
pub async fn stream(&self, prompt: &Prompt) -> ApiResult<ResponseStream<ApiEvent>>;
|
||||
pub async fn compact(&self, prompt: &Prompt) -> ApiResult<Vec<ResponseItem>>;
|
||||
}
|
||||
|
||||
pub struct ChatClient<T: HttpTransport, A: AuthProvider> { /* ... */ }
|
||||
impl<T, A> ChatClient<T, A> {
|
||||
pub async fn stream(&self, prompt: &Prompt) -> ApiResult<ResponseStream<ApiEvent>>;
|
||||
}
|
||||
|
||||
pub struct CompactClient<T: HttpTransport, A: AuthProvider> { /* ... */ }
|
||||
impl<T, A> CompactClient<T, A> {
|
||||
pub async fn compact(&self, prompt: &Prompt) -> ApiResult<Vec<ResponseItem>>;
|
||||
}
|
||||
```
|
||||
- Streaming events unified across wire APIs (this can closely mirror `ResponseEvent` today, and we may type-alias one to the other during migration):
|
||||
```rust
|
||||
pub enum ApiEvent {
|
||||
Created,
|
||||
OutputItemAdded(ResponseItem),
|
||||
OutputItemDone(ResponseItem),
|
||||
OutputTextDelta(String),
|
||||
ReasoningContentDelta { delta: String, content_index: i64 },
|
||||
ReasoningSummaryDelta { delta: String, summary_index: i64 },
|
||||
RateLimits(RateLimitSnapshot),
|
||||
Completed { response_id: String, token_usage: Option<TokenUsage> },
|
||||
}
|
||||
```
|
||||
- Error layering:
|
||||
- `codex-client`: defines `TransportError` / `StreamError` (status codes, IO, timeouts).
|
||||
- `codex-api`: defines `ApiError` that wraps `TransportError` plus API-specific errors parsed from bodies and headers.
|
||||
- `codex-core`: maps `ApiError` into existing `CodexErr` variants so downstream callers remain unchanged.
|
||||
- Aggregation strategies (today’s `AggregateStreamExt`) live here as adapters (`Aggregated`, `Streaming`) that transform `ResponseStream<ApiEvent>` into the higher-level views used by `codex-core`.
|
||||
|
||||
## Implementation Steps
|
||||
|
||||
1. **Create crates**: add `codex-client` and `codex-api` (names keep the `codex-` prefix). Stub lib files with feature flags/tests wired into the workspace; wire them into `Cargo.toml`.
|
||||
2. **Extract API-level SSE + rate limits into `codex-api`**:
|
||||
- Move the Responses SSE parser (`process_sse`), rate-limit parsing, and related tests from `core/src/client.rs` into `codex-api`, keeping the behavior identical.
|
||||
- Introduce `ApiEvent` (initially equivalent to `ResponseEvent`) and `ApiError`, and adjust the parser to emit those.
|
||||
- Provide test-only helpers for fixture streams (replacement for `CODEX_RS_SSE_FIXTURE`) in `codex-api`.
|
||||
3. **Lift transport layer into `codex-client`**:
|
||||
- Move `CodexHttpClient`/`CodexRequestBuilder`, UA/originator plumbing, and backoff helpers from `core/src/default_client.rs` into `codex-client` (or a thin wrapper on top of it).
|
||||
- Introduce `HttpTransport`, `Request`, `RetryPolicy`, `RetryOn`, and `run_with_retry` as described above.
|
||||
- Keep sandbox/no-proxy toggles behind injected configuration so `codex-client` stays generic and does not depend on Codex-specific env vars.
|
||||
4. **Model provider abstraction in `codex-api`**:
|
||||
- Relocate `ModelProviderInfo` (base URL, env/header resolution, retry knobs, wire API enum) into `codex-api`, expressed in terms of `Provider` and `AuthProvider`.
|
||||
- Ensure provider logic handles:
|
||||
- URL building for Responses/Chat/Compact (including Azure special cases).
|
||||
- Static and env-based headers.
|
||||
- Per-provider retry and idle-timeout settings that map cleanly into `RetryPolicy`/`RetryOn`.
|
||||
5. **API crate wiring**:
|
||||
- Move `Prompt`, tool specs, `ResponsesApiRequest`, `TextControls`, and `ResponseEvent/ResponseStream` into `codex-api` under modules (`common`, `responses`, `chat`, `compact`), keeping public types stable or re-exported through `codex-core` as needed.
|
||||
- Rebuild Responses and Chat clients on top of `HttpTransport` + `StreamClient`, reusing shared retry + SSE helpers; keep aggregation adapters as reusable strategies instead of `ModelClient`-local logic.
|
||||
- Implement Compact on top of `UnaryClient` and the unary `execute` path with JSON deserialization, sharing the same retry policy.
|
||||
- Keep request builders symmetric: each client prepares a `Request<serde_json::Value>`, attaches headers/auth via `AuthProvider`, and plugs in its parser (streaming clients) or deserializer (unary) while sharing retry/backoff configuration derived from `Provider`.
|
||||
6. **Core integration layer**:
|
||||
- Replace `core::ModelClient` internals with thin adapters that construct `codex-api` clients using `Config`, `AuthManager`, and `OtelEventManager`.
|
||||
- Keep the public `ModelClient` API and `ResponseEvent`/`ResponseStream` types stable by re-exporting `codex-api` types or providing type aliases.
|
||||
- Preserve existing auth flows (including ChatGPT token refresh) inside `codex-core` or a thin adapter, using `AuthProvider` to surface bearer tokens to `codex-api` and handling 401/refresh semantics at this layer.
|
||||
7. **Tests/migration**:
|
||||
- Move unit tests for SSE parsing, retry/backoff decisions, and provider/header behavior into the new crates; keep integration tests in `core` using the compatibility layer.
|
||||
- Update fixtures to be consumed via test-only adapters in `codex-api`.
|
||||
- Run targeted `just fmt`, `just fix -p` for the touched crates, and scoped `cargo test -p codex-client`, `-p codex-api`, and existing `codex-core` suites.
|
||||
|
||||
## Design Decisions
|
||||
|
||||
- **UA construction**
|
||||
- `codex-client` exposes an optional UA suffix/provider hook (tiny feature) and remains unaware of the CLI; `codex-core` / the CLI compute the full UA (including `terminal::user_agent()`) and pass the suffix or builder down.
|
||||
- **Config vs provider**
|
||||
- Most configuration stays in `codex-core`. `codex-api::Provider` only contains what is strictly required for HTTP (base URLs, query params, retry/timeout knobs, wire API), while higher-level knobs (reasoning defaults, verbosity flags, etc.) remain core concerns.
|
||||
- **Auth flow ownership**
|
||||
- Auth flows (including ChatGPT token refresh) remain in `codex-core`. `AuthProvider` simply exposes already-fresh tokens/account IDs; 401 handling and refresh retries stay in the existing auth layer.
|
||||
- **Error enums**
|
||||
- `codex-client` continues to define `TransportError` / `StreamError`. `codex-api` defines an `ApiError` (deriving `thiserror::Error`) that wraps `TransportError` and API-specific failures, and `codex-core` maps `ApiError` into existing `CodexErr` variants for callers.
|
||||
- **Streaming reconnection semantics**
|
||||
- For now, mid-stream SSE failures are surfaced as errors and only the initial connection is retried via `run_with_retry`. We will revisit mid-stream reconnect/resume once the underlying APIs support cursor/idempotent event semantics.
|
||||
|
||||
@@ -87,6 +87,9 @@ uuid = { workspace = true, features = ["serde", "v4", "v5"] }
|
||||
which = { workspace = true }
|
||||
wildmatch = { workspace = true }
|
||||
|
||||
[features]
|
||||
deterministic_process_ids = []
|
||||
|
||||
|
||||
[target.'cfg(target_os = "linux")'.dependencies]
|
||||
landlock = { workspace = true }
|
||||
@@ -115,6 +118,7 @@ keyring = { workspace = true, features = ["sync-secret-service"] }
|
||||
assert_cmd = { workspace = true }
|
||||
assert_matches = { workspace = true }
|
||||
codex-arg0 = { workspace = true }
|
||||
codex-core = { path = ".", features = ["deterministic_process_ids"] }
|
||||
core_test_support = { workspace = true }
|
||||
ctor = { workspace = true }
|
||||
escargot = { workspace = true }
|
||||
|
||||
@@ -105,10 +105,13 @@ pub(crate) fn convert_apply_patch_to_protocol(
|
||||
ApplyPatchFileChange::Update {
|
||||
unified_diff,
|
||||
move_path,
|
||||
new_content: _new_content,
|
||||
new_content,
|
||||
old_content,
|
||||
} => FileChange::Update {
|
||||
unified_diff: unified_diff.clone(),
|
||||
move_path: move_path.clone(),
|
||||
old_content: old_content.clone(),
|
||||
new_content: new_content.clone(),
|
||||
},
|
||||
};
|
||||
result.insert(path.clone(), protocol_change);
|
||||
|
||||
@@ -102,6 +102,7 @@ use crate::protocol::TurnDiffEvent;
|
||||
use crate::protocol::WarningEvent;
|
||||
use crate::rollout::RolloutRecorder;
|
||||
use crate::rollout::RolloutRecorderParams;
|
||||
use crate::rollout::map_session_init_error;
|
||||
use crate::shell;
|
||||
use crate::state::ActiveTurn;
|
||||
use crate::state::SessionServices;
|
||||
@@ -206,7 +207,7 @@ impl Codex {
|
||||
.await
|
||||
.map_err(|e| {
|
||||
error!("Failed to create session: {e:#}");
|
||||
CodexErr::InternalAgentDied
|
||||
map_session_init_error(&e, &config.codex_home)
|
||||
})?;
|
||||
let conversation_id = session.conversation_id;
|
||||
|
||||
@@ -508,7 +509,7 @@ impl Session {
|
||||
|
||||
let rollout_recorder = rollout_recorder.map_err(|e| {
|
||||
error!("failed to initialize rollout recorder: {e:#}");
|
||||
anyhow::anyhow!("failed to initialize rollout recorder: {e:#}")
|
||||
anyhow::Error::from(e)
|
||||
})?;
|
||||
let rollout_path = rollout_recorder.rollout_path.clone();
|
||||
|
||||
@@ -1189,22 +1190,17 @@ impl Session {
|
||||
}
|
||||
}
|
||||
|
||||
/// Record a user input item to conversation history and also persist a
|
||||
/// corresponding UserMessage EventMsg to rollout.
|
||||
async fn record_input_and_rollout_usermsg(
|
||||
pub(crate) async fn record_response_item_and_emit_turn_item(
|
||||
&self,
|
||||
turn_context: &TurnContext,
|
||||
response_input: &ResponseInputItem,
|
||||
response_item: ResponseItem,
|
||||
) {
|
||||
let response_item: ResponseItem = response_input.clone().into();
|
||||
// Add to conversation history and persist response item to rollout
|
||||
// Add to conversation history and persist response item to rollout.
|
||||
self.record_conversation_items(turn_context, std::slice::from_ref(&response_item))
|
||||
.await;
|
||||
|
||||
// Derive user message events and persist only UserMessage to rollout
|
||||
let turn_item = parse_turn_item(&response_item);
|
||||
|
||||
if let Some(item @ TurnItem::UserMessage(_)) = turn_item {
|
||||
// Derive a turn item and emit lifecycle events if applicable.
|
||||
if let Some(item) = parse_turn_item(&response_item) {
|
||||
self.emit_turn_item_started(turn_context, &item).await;
|
||||
self.emit_turn_item_completed(turn_context, item).await;
|
||||
}
|
||||
@@ -1880,12 +1876,7 @@ async fn spawn_review_thread(
|
||||
text: review_prompt,
|
||||
}];
|
||||
let tc = Arc::new(review_turn_context);
|
||||
sess.spawn_task(
|
||||
tc.clone(),
|
||||
input,
|
||||
ReviewTask::new(review_request.append_to_original_thread),
|
||||
)
|
||||
.await;
|
||||
sess.spawn_task(tc.clone(), input, ReviewTask::new()).await;
|
||||
|
||||
// Announce entering review mode so UIs can switch modes.
|
||||
sess.send_event(&tc, EventMsg::EnteredReviewMode(review_request))
|
||||
@@ -1921,7 +1912,8 @@ pub(crate) async fn run_task(
|
||||
sess.send_event(&turn_context, event).await;
|
||||
|
||||
let initial_input_for_turn: ResponseInputItem = ResponseInputItem::from(input);
|
||||
sess.record_input_and_rollout_usermsg(turn_context.as_ref(), &initial_input_for_turn)
|
||||
let response_item: ResponseItem = initial_input_for_turn.clone().into();
|
||||
sess.record_response_item_and_emit_turn_item(turn_context.as_ref(), response_item)
|
||||
.await;
|
||||
|
||||
sess.maybe_start_ghost_snapshot(Arc::clone(&turn_context), cancellation_token.child_token())
|
||||
@@ -2886,7 +2878,7 @@ mod tests {
|
||||
let input = vec![UserInput::Text {
|
||||
text: "start review".to_string(),
|
||||
}];
|
||||
sess.spawn_task(Arc::clone(&tc), input, ReviewTask::new(true))
|
||||
sess.spawn_task(Arc::clone(&tc), input, ReviewTask::new())
|
||||
.await;
|
||||
|
||||
sess.abort_all_tasks(TurnAbortReason::Interrupted).await;
|
||||
@@ -2914,6 +2906,8 @@ mod tests {
|
||||
.expect("event");
|
||||
match evt.msg {
|
||||
EventMsg::RawResponseItem(_) => continue,
|
||||
EventMsg::ItemStarted(_) | EventMsg::ItemCompleted(_) => continue,
|
||||
EventMsg::AgentMessage(_) => continue,
|
||||
EventMsg::TurnAborted(e) => {
|
||||
assert_eq!(TurnAbortReason::Interrupted, e.reason);
|
||||
break;
|
||||
@@ -2923,23 +2917,7 @@ mod tests {
|
||||
}
|
||||
|
||||
let history = sess.clone_history().await.get_history();
|
||||
let found = history.iter().any(|item| match item {
|
||||
ResponseItem::Message { role, content, .. } if role == "user" => {
|
||||
content.iter().any(|ci| match ci {
|
||||
ContentItem::InputText { text } => {
|
||||
text.contains("<user_action>")
|
||||
&& text.contains("review")
|
||||
&& text.contains("interrupted")
|
||||
}
|
||||
_ => false,
|
||||
})
|
||||
}
|
||||
_ => false,
|
||||
});
|
||||
assert!(
|
||||
found,
|
||||
"synthetic review interruption not recorded in history"
|
||||
);
|
||||
let _ = history;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
use crate::protocol::ReviewFinding;
|
||||
use crate::protocol::ReviewOutputEvent;
|
||||
|
||||
// Note: We keep this module UI-agnostic. It returns plain strings that
|
||||
// higher layers (e.g., TUI) may style as needed.
|
||||
@@ -10,6 +11,8 @@ fn format_location(item: &ReviewFinding) -> String {
|
||||
format!("{path}:{start}-{end}")
|
||||
}
|
||||
|
||||
const REVIEW_FALLBACK_MESSAGE: &str = "Reviewer failed to output a response.";
|
||||
|
||||
/// Format a full review findings block as plain text lines.
|
||||
///
|
||||
/// - When `selection` is `Some`, each item line includes a checkbox marker:
|
||||
@@ -53,3 +56,27 @@ pub fn format_review_findings_block(
|
||||
|
||||
lines.join("\n")
|
||||
}
|
||||
|
||||
/// Render a human-readable review summary suitable for a user-facing message.
|
||||
///
|
||||
/// Returns either the explanation, the formatted findings block, or both
|
||||
/// separated by a blank line. If neither is present, emits a fallback message.
|
||||
pub fn render_review_output_text(output: &ReviewOutputEvent) -> String {
|
||||
let mut sections = Vec::new();
|
||||
let explanation = output.overall_explanation.trim();
|
||||
if !explanation.is_empty() {
|
||||
sections.push(explanation.to_string());
|
||||
}
|
||||
if !output.findings.is_empty() {
|
||||
let findings = format_review_findings_block(&output.findings, None);
|
||||
let trimmed = findings.trim();
|
||||
if !trimmed.is_empty() {
|
||||
sections.push(trimmed.to_string());
|
||||
}
|
||||
}
|
||||
if sections.is_empty() {
|
||||
REVIEW_FALLBACK_MESSAGE.to_string()
|
||||
} else {
|
||||
sections.join("\n\n")
|
||||
}
|
||||
}
|
||||
|
||||
49
codex-rs/core/src/rollout/error.rs
Normal file
49
codex-rs/core/src/rollout/error.rs
Normal file
@@ -0,0 +1,49 @@
|
||||
use std::io::ErrorKind;
|
||||
use std::path::Path;
|
||||
|
||||
use crate::error::CodexErr;
|
||||
use crate::rollout::SESSIONS_SUBDIR;
|
||||
|
||||
pub(crate) fn map_session_init_error(err: &anyhow::Error, codex_home: &Path) -> CodexErr {
|
||||
if let Some(mapped) = err
|
||||
.chain()
|
||||
.filter_map(|cause| cause.downcast_ref::<std::io::Error>())
|
||||
.find_map(|io_err| map_rollout_io_error(io_err, codex_home))
|
||||
{
|
||||
return mapped;
|
||||
}
|
||||
|
||||
CodexErr::Fatal(format!("Failed to initialize session: {err:#}"))
|
||||
}
|
||||
|
||||
fn map_rollout_io_error(io_err: &std::io::Error, codex_home: &Path) -> Option<CodexErr> {
|
||||
let sessions_dir = codex_home.join(SESSIONS_SUBDIR);
|
||||
let hint = match io_err.kind() {
|
||||
ErrorKind::PermissionDenied => format!(
|
||||
"Codex cannot access session files at {} (permission denied). If sessions were created using sudo, fix ownership: sudo chown -R $(whoami) {}",
|
||||
sessions_dir.display(),
|
||||
codex_home.display()
|
||||
),
|
||||
ErrorKind::NotFound => format!(
|
||||
"Session storage missing at {}. Create the directory or choose a different Codex home.",
|
||||
sessions_dir.display()
|
||||
),
|
||||
ErrorKind::AlreadyExists => format!(
|
||||
"Session storage path {} is blocked by an existing file. Remove or rename it so Codex can create sessions.",
|
||||
sessions_dir.display()
|
||||
),
|
||||
ErrorKind::InvalidData | ErrorKind::InvalidInput => format!(
|
||||
"Session data under {} looks corrupt or unreadable. Clearing the sessions directory may help (this will remove saved conversations).",
|
||||
sessions_dir.display()
|
||||
),
|
||||
ErrorKind::IsADirectory | ErrorKind::NotADirectory => format!(
|
||||
"Session storage path {} has an unexpected type. Ensure it is a directory Codex can use for session files.",
|
||||
sessions_dir.display()
|
||||
),
|
||||
_ => return None,
|
||||
};
|
||||
|
||||
Some(CodexErr::Fatal(format!(
|
||||
"{hint} (underlying error: {io_err})"
|
||||
)))
|
||||
}
|
||||
@@ -7,11 +7,13 @@ pub const ARCHIVED_SESSIONS_SUBDIR: &str = "archived_sessions";
|
||||
pub const INTERACTIVE_SESSION_SOURCES: &[SessionSource] =
|
||||
&[SessionSource::Cli, SessionSource::VSCode];
|
||||
|
||||
pub(crate) mod error;
|
||||
pub mod list;
|
||||
pub(crate) mod policy;
|
||||
pub mod recorder;
|
||||
|
||||
pub use codex_protocol::protocol::SessionMeta;
|
||||
pub(crate) use error::map_session_init_error;
|
||||
pub use list::find_conversation_path_by_id_str;
|
||||
pub use recorder::RolloutRecorder;
|
||||
pub use recorder::RolloutRecorderParams;
|
||||
|
||||
@@ -14,6 +14,7 @@ use crate::protocol::SandboxPolicy;
|
||||
use askama::Template;
|
||||
use codex_otel::otel_event_manager::OtelEventManager;
|
||||
use codex_protocol::ConversationId;
|
||||
use codex_protocol::config_types::ReasoningEffort as ReasoningEffortConfig;
|
||||
use codex_protocol::models::ContentItem;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use codex_protocol::protocol::SandboxCommandAssessment;
|
||||
@@ -23,7 +24,8 @@ use serde_json::json;
|
||||
use tokio::time::timeout;
|
||||
use tracing::warn;
|
||||
|
||||
const SANDBOX_ASSESSMENT_TIMEOUT: Duration = Duration::from_secs(5);
|
||||
const SANDBOX_ASSESSMENT_TIMEOUT: Duration = Duration::from_secs(15);
|
||||
const SANDBOX_ASSESSMENT_REASONING_EFFORT: ReasoningEffortConfig = ReasoningEffortConfig::Medium;
|
||||
|
||||
#[derive(Template)]
|
||||
#[template(path = "sandboxing/assessment_prompt.md", escape = "none")]
|
||||
@@ -130,7 +132,7 @@ pub(crate) async fn assess_command(
|
||||
Some(auth_manager),
|
||||
child_otel,
|
||||
provider,
|
||||
config.model_reasoning_effort,
|
||||
Some(SANDBOX_ASSESSMENT_REASONING_EFFORT),
|
||||
config.model_reasoning_summary,
|
||||
conversation_id,
|
||||
session_source,
|
||||
|
||||
@@ -17,6 +17,7 @@ use crate::codex::Session;
|
||||
use crate::codex::TurnContext;
|
||||
use crate::codex_delegate::run_codex_conversation_one_shot;
|
||||
use crate::review_format::format_review_findings_block;
|
||||
use crate::review_format::render_review_output_text;
|
||||
use crate::state::TaskKind;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
|
||||
@@ -24,15 +25,11 @@ use super::SessionTask;
|
||||
use super::SessionTaskContext;
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
pub(crate) struct ReviewTask {
|
||||
append_to_original_thread: bool,
|
||||
}
|
||||
pub(crate) struct ReviewTask;
|
||||
|
||||
impl ReviewTask {
|
||||
pub(crate) fn new(append_to_original_thread: bool) -> Self {
|
||||
Self {
|
||||
append_to_original_thread,
|
||||
}
|
||||
pub(crate) fn new() -> Self {
|
||||
Self
|
||||
}
|
||||
}
|
||||
|
||||
@@ -62,25 +59,13 @@ impl SessionTask for ReviewTask {
|
||||
None => None,
|
||||
};
|
||||
if !cancellation_token.is_cancelled() {
|
||||
exit_review_mode(
|
||||
session.clone_session(),
|
||||
output.clone(),
|
||||
ctx.clone(),
|
||||
self.append_to_original_thread,
|
||||
)
|
||||
.await;
|
||||
exit_review_mode(session.clone_session(), output.clone(), ctx.clone()).await;
|
||||
}
|
||||
None
|
||||
}
|
||||
|
||||
async fn abort(&self, session: Arc<SessionTaskContext>, ctx: Arc<TurnContext>) {
|
||||
exit_review_mode(
|
||||
session.clone_session(),
|
||||
None,
|
||||
ctx,
|
||||
self.append_to_original_thread,
|
||||
)
|
||||
.await;
|
||||
exit_review_mode(session.clone_session(), None, ctx).await;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -197,39 +182,57 @@ pub(crate) async fn exit_review_mode(
|
||||
session: Arc<Session>,
|
||||
review_output: Option<ReviewOutputEvent>,
|
||||
ctx: Arc<TurnContext>,
|
||||
append_to_original_thread: bool,
|
||||
) {
|
||||
if append_to_original_thread {
|
||||
let user_message = if let Some(out) = review_output.clone() {
|
||||
let mut findings_str = String::new();
|
||||
let text = out.overall_explanation.trim();
|
||||
if !text.is_empty() {
|
||||
findings_str.push_str(text);
|
||||
}
|
||||
if !out.findings.is_empty() {
|
||||
let block = format_review_findings_block(&out.findings, None);
|
||||
findings_str.push_str(&format!("\n{block}"));
|
||||
}
|
||||
crate::client_common::REVIEW_EXIT_SUCCESS_TMPL.replace("{results}", &findings_str)
|
||||
} else {
|
||||
crate::client_common::REVIEW_EXIT_INTERRUPTED_TMPL.to_string()
|
||||
};
|
||||
const REVIEW_USER_MESSAGE_ID: &str = "review:rollout:user";
|
||||
const REVIEW_ASSISTANT_MESSAGE_ID: &str = "review:rollout:assistant";
|
||||
let (user_message, assistant_message) = if let Some(out) = review_output.clone() {
|
||||
let mut findings_str = String::new();
|
||||
let text = out.overall_explanation.trim();
|
||||
if !text.is_empty() {
|
||||
findings_str.push_str(text);
|
||||
}
|
||||
if !out.findings.is_empty() {
|
||||
let block = format_review_findings_block(&out.findings, None);
|
||||
findings_str.push_str(&format!("\n{block}"));
|
||||
}
|
||||
let rendered =
|
||||
crate::client_common::REVIEW_EXIT_SUCCESS_TMPL.replace("{results}", &findings_str);
|
||||
let assistant_message = render_review_output_text(&out);
|
||||
(rendered, assistant_message)
|
||||
} else {
|
||||
let rendered = crate::client_common::REVIEW_EXIT_INTERRUPTED_TMPL.to_string();
|
||||
let assistant_message =
|
||||
"Review was interrupted. Please re-run /review and wait for it to complete."
|
||||
.to_string();
|
||||
(rendered, assistant_message)
|
||||
};
|
||||
|
||||
session
|
||||
.record_conversation_items(
|
||||
&ctx,
|
||||
&[ResponseItem::Message {
|
||||
id: None,
|
||||
role: "user".to_string(),
|
||||
content: vec![ContentItem::InputText { text: user_message }],
|
||||
}],
|
||||
)
|
||||
.await;
|
||||
}
|
||||
session
|
||||
.record_conversation_items(
|
||||
&ctx,
|
||||
&[ResponseItem::Message {
|
||||
id: Some(REVIEW_USER_MESSAGE_ID.to_string()),
|
||||
role: "user".to_string(),
|
||||
content: vec![ContentItem::InputText { text: user_message }],
|
||||
}],
|
||||
)
|
||||
.await;
|
||||
session
|
||||
.send_event(
|
||||
ctx.as_ref(),
|
||||
EventMsg::ExitedReviewMode(ExitedReviewModeEvent { review_output }),
|
||||
)
|
||||
.await;
|
||||
session
|
||||
.record_response_item_and_emit_turn_item(
|
||||
ctx.as_ref(),
|
||||
ResponseItem::Message {
|
||||
id: Some(REVIEW_ASSISTANT_MESSAGE_ID.to_string()),
|
||||
role: "assistant".to_string(),
|
||||
content: vec![ContentItem::OutputText {
|
||||
text: assistant_message,
|
||||
}],
|
||||
},
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
@@ -81,6 +81,7 @@ impl SessionTask for UserShellCommandTask {
|
||||
turn_context.as_ref(),
|
||||
EventMsg::ExecCommandBegin(ExecCommandBeginEvent {
|
||||
call_id: call_id.clone(),
|
||||
process_id: None,
|
||||
turn_id: turn_context.sub_id.clone(),
|
||||
command: command.clone(),
|
||||
cwd: cwd.clone(),
|
||||
@@ -139,6 +140,7 @@ impl SessionTask for UserShellCommandTask {
|
||||
turn_context.as_ref(),
|
||||
EventMsg::ExecCommandEnd(ExecCommandEndEvent {
|
||||
call_id,
|
||||
process_id: None,
|
||||
turn_id: turn_context.sub_id.clone(),
|
||||
command: command.clone(),
|
||||
cwd: cwd.clone(),
|
||||
@@ -161,6 +163,7 @@ impl SessionTask for UserShellCommandTask {
|
||||
turn_context.as_ref(),
|
||||
EventMsg::ExecCommandEnd(ExecCommandEndEvent {
|
||||
call_id: call_id.clone(),
|
||||
process_id: None,
|
||||
turn_id: turn_context.sub_id.clone(),
|
||||
command: command.clone(),
|
||||
cwd: cwd.clone(),
|
||||
@@ -205,6 +208,7 @@ impl SessionTask for UserShellCommandTask {
|
||||
turn_context.as_ref(),
|
||||
EventMsg::ExecCommandEnd(ExecCommandEndEvent {
|
||||
call_id,
|
||||
process_id: None,
|
||||
turn_id: turn_context.sub_id.clone(),
|
||||
command,
|
||||
cwd,
|
||||
|
||||
@@ -65,12 +65,14 @@ pub(crate) async fn emit_exec_command_begin(
|
||||
parsed_cmd: &[ParsedCommand],
|
||||
source: ExecCommandSource,
|
||||
interaction_input: Option<String>,
|
||||
process_id: Option<&str>,
|
||||
) {
|
||||
ctx.session
|
||||
.send_event(
|
||||
ctx.turn,
|
||||
EventMsg::ExecCommandBegin(ExecCommandBeginEvent {
|
||||
call_id: ctx.call_id.to_string(),
|
||||
process_id: process_id.map(str::to_owned),
|
||||
turn_id: ctx.turn.sub_id.clone(),
|
||||
command: command.to_vec(),
|
||||
cwd: cwd.to_path_buf(),
|
||||
@@ -100,6 +102,7 @@ pub(crate) enum ToolEmitter {
|
||||
source: ExecCommandSource,
|
||||
interaction_input: Option<String>,
|
||||
parsed_cmd: Vec<ParsedCommand>,
|
||||
process_id: Option<String>,
|
||||
},
|
||||
}
|
||||
|
||||
@@ -132,6 +135,7 @@ impl ToolEmitter {
|
||||
cwd: PathBuf,
|
||||
source: ExecCommandSource,
|
||||
interaction_input: Option<String>,
|
||||
process_id: Option<String>,
|
||||
) -> Self {
|
||||
let parsed_cmd = parse_command(command);
|
||||
Self::UnifiedExec {
|
||||
@@ -140,6 +144,7 @@ impl ToolEmitter {
|
||||
source,
|
||||
interaction_input,
|
||||
parsed_cmd,
|
||||
process_id,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -157,7 +162,7 @@ impl ToolEmitter {
|
||||
) => {
|
||||
emit_exec_stage(
|
||||
ctx,
|
||||
ExecCommandInput::new(command, cwd.as_path(), parsed_cmd, *source, None),
|
||||
ExecCommandInput::new(command, cwd.as_path(), parsed_cmd, *source, None, None),
|
||||
stage,
|
||||
)
|
||||
.await;
|
||||
@@ -229,6 +234,7 @@ impl ToolEmitter {
|
||||
source,
|
||||
interaction_input,
|
||||
parsed_cmd,
|
||||
process_id,
|
||||
},
|
||||
stage,
|
||||
) => {
|
||||
@@ -240,6 +246,7 @@ impl ToolEmitter {
|
||||
parsed_cmd,
|
||||
*source,
|
||||
interaction_input.as_deref(),
|
||||
process_id.as_deref(),
|
||||
),
|
||||
stage,
|
||||
)
|
||||
@@ -319,6 +326,7 @@ struct ExecCommandInput<'a> {
|
||||
parsed_cmd: &'a [ParsedCommand],
|
||||
source: ExecCommandSource,
|
||||
interaction_input: Option<&'a str>,
|
||||
process_id: Option<&'a str>,
|
||||
}
|
||||
|
||||
impl<'a> ExecCommandInput<'a> {
|
||||
@@ -328,6 +336,7 @@ impl<'a> ExecCommandInput<'a> {
|
||||
parsed_cmd: &'a [ParsedCommand],
|
||||
source: ExecCommandSource,
|
||||
interaction_input: Option<&'a str>,
|
||||
process_id: Option<&'a str>,
|
||||
) -> Self {
|
||||
Self {
|
||||
command,
|
||||
@@ -335,6 +344,7 @@ impl<'a> ExecCommandInput<'a> {
|
||||
parsed_cmd,
|
||||
source,
|
||||
interaction_input,
|
||||
process_id,
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -362,6 +372,7 @@ async fn emit_exec_stage(
|
||||
exec_input.parsed_cmd,
|
||||
exec_input.source,
|
||||
exec_input.interaction_input.map(str::to_owned),
|
||||
exec_input.process_id,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
@@ -402,6 +413,7 @@ async fn emit_exec_end(
|
||||
ctx.turn,
|
||||
EventMsg::ExecCommandEnd(ExecCommandEndEvent {
|
||||
call_id: ctx.call_id.to_string(),
|
||||
process_id: exec_input.process_id.map(str::to_owned),
|
||||
turn_id: ctx.turn.sub_id.clone(),
|
||||
command: exec_input.command.to_vec(),
|
||||
cwd: exec_input.cwd.to_path_buf(),
|
||||
|
||||
@@ -46,6 +46,7 @@ struct ExecCommandArgs {
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
struct WriteStdinArgs {
|
||||
// The model is trained on `session_id`.
|
||||
session_id: i32,
|
||||
#[serde(default)]
|
||||
chars: String,
|
||||
@@ -128,6 +129,7 @@ impl ToolHandler for UnifiedExecHandler {
|
||||
"failed to parse exec_command arguments: {err:?}"
|
||||
))
|
||||
})?;
|
||||
let process_id = manager.allocate_process_id().await;
|
||||
|
||||
let command = get_command(&args);
|
||||
let ExecCommandArgs {
|
||||
@@ -168,6 +170,7 @@ impl ToolHandler for UnifiedExecHandler {
|
||||
cwd.clone(),
|
||||
ExecCommandSource::UnifiedExecStartup,
|
||||
None,
|
||||
Some(process_id.clone()),
|
||||
);
|
||||
emitter.emit(event_ctx, ToolEventStage::Begin).await;
|
||||
|
||||
@@ -175,6 +178,7 @@ impl ToolHandler for UnifiedExecHandler {
|
||||
.exec_command(
|
||||
ExecCommandRequest {
|
||||
command,
|
||||
process_id,
|
||||
yield_time_ms,
|
||||
max_output_tokens,
|
||||
workdir,
|
||||
@@ -197,7 +201,7 @@ impl ToolHandler for UnifiedExecHandler {
|
||||
manager
|
||||
.write_stdin(WriteStdinRequest {
|
||||
call_id: &call_id,
|
||||
session_id: args.session_id,
|
||||
process_id: &args.session_id.to_string(),
|
||||
input: &args.chars,
|
||||
yield_time_ms: args.yield_time_ms,
|
||||
max_output_tokens: args.max_output_tokens,
|
||||
@@ -255,8 +259,9 @@ fn format_response(response: &UnifiedExecResponse) -> String {
|
||||
sections.push(format!("Process exited with code {exit_code}"));
|
||||
}
|
||||
|
||||
if let Some(session_id) = response.session_id {
|
||||
sections.push(format!("Process running with session ID {session_id}"));
|
||||
if let Some(process_id) = &response.process_id {
|
||||
// Training still uses "session ID".
|
||||
sections.push(format!("Process running with session ID {process_id}"));
|
||||
}
|
||||
|
||||
if let Some(original_token_count) = response.original_token_count {
|
||||
|
||||
@@ -545,6 +545,8 @@ index {ZERO_OID}..{right_oid}
|
||||
FileChange::Update {
|
||||
unified_diff: "".to_owned(),
|
||||
move_path: None,
|
||||
old_content: "foo\n".to_owned(),
|
||||
new_content: "foo\nbar\n".to_owned(),
|
||||
},
|
||||
)]);
|
||||
acc.on_patch_begin(&update_changes);
|
||||
@@ -620,6 +622,8 @@ index {left_oid}..{ZERO_OID}
|
||||
FileChange::Update {
|
||||
unified_diff: "".to_owned(),
|
||||
move_path: Some(dest.clone()),
|
||||
old_content: "line\n".to_owned(),
|
||||
new_content: "line2\n".to_owned(),
|
||||
},
|
||||
)]);
|
||||
acc.on_patch_begin(&mv_changes);
|
||||
@@ -660,6 +664,8 @@ index {left_oid}..{right_oid}
|
||||
FileChange::Update {
|
||||
unified_diff: "".to_owned(),
|
||||
move_path: Some(dest.clone()),
|
||||
old_content: "same\n".to_owned(),
|
||||
new_content: "same\n".to_owned(),
|
||||
},
|
||||
)]);
|
||||
acc.on_patch_begin(&mv_changes);
|
||||
@@ -682,6 +688,8 @@ index {left_oid}..{right_oid}
|
||||
FileChange::Update {
|
||||
unified_diff: "".into(),
|
||||
move_path: Some(dest.clone()),
|
||||
old_content: String::new(),
|
||||
new_content: "hello\n".to_owned(),
|
||||
},
|
||||
)]);
|
||||
acc.on_patch_begin(&mv);
|
||||
@@ -722,6 +730,8 @@ index {ZERO_OID}..{right_oid}
|
||||
FileChange::Update {
|
||||
unified_diff: "".to_owned(),
|
||||
move_path: None,
|
||||
old_content: "foo\n".to_owned(),
|
||||
new_content: "foo\nbar\n".to_owned(),
|
||||
},
|
||||
)]);
|
||||
acc.on_patch_begin(&update_a);
|
||||
@@ -802,6 +812,8 @@ index {left_oid_b}..{ZERO_OID}
|
||||
FileChange::Update {
|
||||
unified_diff: "".to_owned(),
|
||||
move_path: None,
|
||||
old_content: String::new(),
|
||||
new_content: String::new(),
|
||||
},
|
||||
)]);
|
||||
acc.on_patch_begin(&update_changes);
|
||||
@@ -868,6 +880,8 @@ index {ZERO_OID}..{right_oid}
|
||||
FileChange::Update {
|
||||
unified_diff: "".to_owned(),
|
||||
move_path: None,
|
||||
old_content: "foo\n".to_owned(),
|
||||
new_content: "foo\nbar\n".to_owned(),
|
||||
},
|
||||
)]);
|
||||
acc.on_patch_begin(&update_changes);
|
||||
|
||||
@@ -5,8 +5,9 @@ use thiserror::Error;
|
||||
pub(crate) enum UnifiedExecError {
|
||||
#[error("Failed to create unified exec session: {message}")]
|
||||
CreateSession { message: String },
|
||||
#[error("Unknown session id {session_id}")]
|
||||
UnknownSessionId { session_id: i32 },
|
||||
// Called "session" in the model's training.
|
||||
#[error("Unknown session id {process_id}")]
|
||||
UnknownSessionId { process_id: String },
|
||||
#[error("failed to write to stdin")]
|
||||
WriteToStdin,
|
||||
#[error("missing command line for unified exec request")]
|
||||
|
||||
@@ -22,9 +22,9 @@
|
||||
//! - `session_manager.rs`: orchestration (approvals, sandboxing, reuse) and request handling.
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
use std::sync::atomic::AtomicI32;
|
||||
use std::time::Duration;
|
||||
|
||||
use rand::Rng;
|
||||
@@ -67,6 +67,7 @@ impl UnifiedExecContext {
|
||||
#[derive(Debug)]
|
||||
pub(crate) struct ExecCommandRequest {
|
||||
pub command: Vec<String>,
|
||||
pub process_id: String,
|
||||
pub yield_time_ms: u64,
|
||||
pub max_output_tokens: Option<usize>,
|
||||
pub workdir: Option<PathBuf>,
|
||||
@@ -77,7 +78,7 @@ pub(crate) struct ExecCommandRequest {
|
||||
#[derive(Debug)]
|
||||
pub(crate) struct WriteStdinRequest<'a> {
|
||||
pub call_id: &'a str,
|
||||
pub session_id: i32,
|
||||
pub process_id: &'a str,
|
||||
pub input: &'a str,
|
||||
pub yield_time_ms: u64,
|
||||
pub max_output_tokens: Option<usize>,
|
||||
@@ -89,7 +90,7 @@ pub(crate) struct UnifiedExecResponse {
|
||||
pub chunk_id: String,
|
||||
pub wall_time: Duration,
|
||||
pub output: String,
|
||||
pub session_id: Option<i32>,
|
||||
pub process_id: Option<String>,
|
||||
pub exit_code: Option<i32>,
|
||||
pub original_token_count: Option<usize>,
|
||||
pub session_command: Option<Vec<String>>,
|
||||
@@ -97,15 +98,34 @@ pub(crate) struct UnifiedExecResponse {
|
||||
|
||||
#[derive(Default)]
|
||||
pub(crate) struct UnifiedExecSessionManager {
|
||||
next_session_id: AtomicI32,
|
||||
sessions: Mutex<HashMap<i32, SessionEntry>>,
|
||||
session_store: Mutex<SessionStore>,
|
||||
}
|
||||
|
||||
// Required for mutex sharing.
|
||||
#[derive(Default)]
|
||||
pub(crate) struct SessionStore {
|
||||
sessions: HashMap<String, SessionEntry>,
|
||||
reserved_sessions_id: HashSet<String>,
|
||||
}
|
||||
|
||||
impl SessionStore {
|
||||
fn remove(&mut self, session_id: &str) -> Option<SessionEntry> {
|
||||
self.reserved_sessions_id.remove(session_id);
|
||||
self.sessions.remove(session_id)
|
||||
}
|
||||
|
||||
pub(crate) fn clear(&mut self) {
|
||||
self.reserved_sessions_id.clear();
|
||||
self.sessions.clear();
|
||||
}
|
||||
}
|
||||
|
||||
struct SessionEntry {
|
||||
session: session::UnifiedExecSession,
|
||||
session: UnifiedExecSession,
|
||||
session_ref: Arc<Session>,
|
||||
turn_ref: Arc<TurnContext>,
|
||||
call_id: String,
|
||||
process_id: String,
|
||||
command: Vec<String>,
|
||||
cwd: PathBuf,
|
||||
started_at: tokio::time::Instant,
|
||||
@@ -159,6 +179,11 @@ mod tests {
|
||||
) -> Result<UnifiedExecResponse, UnifiedExecError> {
|
||||
let context =
|
||||
UnifiedExecContext::new(Arc::clone(session), Arc::clone(turn), "call".to_string());
|
||||
let process_id = session
|
||||
.services
|
||||
.unified_exec_manager
|
||||
.allocate_process_id()
|
||||
.await;
|
||||
|
||||
session
|
||||
.services
|
||||
@@ -166,6 +191,7 @@ mod tests {
|
||||
.exec_command(
|
||||
ExecCommandRequest {
|
||||
command: vec!["bash".to_string(), "-lc".to_string(), cmd.to_string()],
|
||||
process_id,
|
||||
yield_time_ms,
|
||||
max_output_tokens: None,
|
||||
workdir: None,
|
||||
@@ -179,7 +205,7 @@ mod tests {
|
||||
|
||||
async fn write_stdin(
|
||||
session: &Arc<Session>,
|
||||
session_id: i32,
|
||||
process_id: &str,
|
||||
input: &str,
|
||||
yield_time_ms: u64,
|
||||
) -> Result<UnifiedExecResponse, UnifiedExecError> {
|
||||
@@ -188,7 +214,7 @@ mod tests {
|
||||
.unified_exec_manager
|
||||
.write_stdin(WriteStdinRequest {
|
||||
call_id: "write-stdin",
|
||||
session_id,
|
||||
process_id,
|
||||
input,
|
||||
yield_time_ms,
|
||||
max_output_tokens: None,
|
||||
@@ -221,11 +247,15 @@ mod tests {
|
||||
let (session, turn) = test_session_and_turn();
|
||||
|
||||
let open_shell = exec_command(&session, &turn, "bash -i", 2_500).await?;
|
||||
let session_id = open_shell.session_id.expect("expected session_id");
|
||||
let process_id = open_shell
|
||||
.process_id
|
||||
.as_ref()
|
||||
.expect("expected process_id")
|
||||
.as_str();
|
||||
|
||||
write_stdin(
|
||||
&session,
|
||||
session_id,
|
||||
process_id,
|
||||
"export CODEX_INTERACTIVE_SHELL_VAR=codex\n",
|
||||
2_500,
|
||||
)
|
||||
@@ -233,7 +263,7 @@ mod tests {
|
||||
|
||||
let out_2 = write_stdin(
|
||||
&session,
|
||||
session_id,
|
||||
process_id,
|
||||
"echo $CODEX_INTERACTIVE_SHELL_VAR\n",
|
||||
2_500,
|
||||
)
|
||||
@@ -253,11 +283,15 @@ mod tests {
|
||||
let (session, turn) = test_session_and_turn();
|
||||
|
||||
let shell_a = exec_command(&session, &turn, "bash -i", 2_500).await?;
|
||||
let session_a = shell_a.session_id.expect("expected session id");
|
||||
let session_a = shell_a
|
||||
.process_id
|
||||
.as_ref()
|
||||
.expect("expected process id")
|
||||
.clone();
|
||||
|
||||
write_stdin(
|
||||
&session,
|
||||
session_a,
|
||||
session_a.as_str(),
|
||||
"export CODEX_INTERACTIVE_SHELL_VAR=codex\n",
|
||||
2_500,
|
||||
)
|
||||
@@ -265,9 +299,10 @@ mod tests {
|
||||
|
||||
let out_2 =
|
||||
exec_command(&session, &turn, "echo $CODEX_INTERACTIVE_SHELL_VAR", 2_500).await?;
|
||||
tokio::time::sleep(Duration::from_secs(2)).await;
|
||||
assert!(
|
||||
out_2.session_id.is_none(),
|
||||
"short command should not retain a session"
|
||||
out_2.process_id.is_none(),
|
||||
"short command should not report a process id if it exits quickly"
|
||||
);
|
||||
assert!(
|
||||
!out_2.output.contains("codex"),
|
||||
@@ -276,7 +311,11 @@ mod tests {
|
||||
|
||||
let out_3 = write_stdin(
|
||||
&session,
|
||||
session_a,
|
||||
shell_a
|
||||
.process_id
|
||||
.as_ref()
|
||||
.expect("expected process id")
|
||||
.as_str(),
|
||||
"echo $CODEX_INTERACTIVE_SHELL_VAR\n",
|
||||
2_500,
|
||||
)
|
||||
@@ -296,11 +335,15 @@ mod tests {
|
||||
let (session, turn) = test_session_and_turn();
|
||||
|
||||
let open_shell = exec_command(&session, &turn, "bash -i", 2_500).await?;
|
||||
let session_id = open_shell.session_id.expect("expected session id");
|
||||
let process_id = open_shell
|
||||
.process_id
|
||||
.as_ref()
|
||||
.expect("expected process id")
|
||||
.as_str();
|
||||
|
||||
write_stdin(
|
||||
&session,
|
||||
session_id,
|
||||
process_id,
|
||||
"export CODEX_INTERACTIVE_SHELL_VAR=codex\n",
|
||||
2_500,
|
||||
)
|
||||
@@ -308,7 +351,7 @@ mod tests {
|
||||
|
||||
let out_2 = write_stdin(
|
||||
&session,
|
||||
session_id,
|
||||
process_id,
|
||||
"sleep 5 && echo $CODEX_INTERACTIVE_SHELL_VAR\n",
|
||||
10,
|
||||
)
|
||||
@@ -320,7 +363,7 @@ mod tests {
|
||||
|
||||
tokio::time::sleep(Duration::from_secs(7)).await;
|
||||
|
||||
let out_3 = write_stdin(&session, session_id, "", 100).await?;
|
||||
let out_3 = write_stdin(&session, process_id, "", 100).await?;
|
||||
|
||||
assert!(
|
||||
out_3.output.contains("codex"),
|
||||
@@ -337,7 +380,7 @@ mod tests {
|
||||
|
||||
let result = exec_command(&session, &turn, "echo codex", 120_000).await?;
|
||||
|
||||
assert!(result.session_id.is_none());
|
||||
assert!(result.process_id.is_some());
|
||||
assert!(result.output.contains("codex"));
|
||||
|
||||
Ok(())
|
||||
@@ -350,8 +393,8 @@ mod tests {
|
||||
let result = exec_command(&session, &turn, "echo codex", 2_500).await?;
|
||||
|
||||
assert!(
|
||||
result.session_id.is_none(),
|
||||
"completed command should not retain session"
|
||||
result.process_id.is_some(),
|
||||
"completed command should report a process id"
|
||||
);
|
||||
assert!(result.output.contains("codex"));
|
||||
|
||||
@@ -359,9 +402,10 @@ mod tests {
|
||||
session
|
||||
.services
|
||||
.unified_exec_manager
|
||||
.sessions
|
||||
.session_store
|
||||
.lock()
|
||||
.await
|
||||
.sessions
|
||||
.is_empty()
|
||||
);
|
||||
|
||||
@@ -375,31 +419,36 @@ mod tests {
|
||||
let (session, turn) = test_session_and_turn();
|
||||
|
||||
let open_shell = exec_command(&session, &turn, "bash -i", 2_500).await?;
|
||||
let session_id = open_shell.session_id.expect("expected session id");
|
||||
let process_id = open_shell
|
||||
.process_id
|
||||
.as_ref()
|
||||
.expect("expected process id")
|
||||
.as_str();
|
||||
|
||||
write_stdin(&session, session_id, "exit\n", 2_500).await?;
|
||||
write_stdin(&session, process_id, "exit\n", 2_500).await?;
|
||||
|
||||
tokio::time::sleep(Duration::from_millis(200)).await;
|
||||
|
||||
let err = write_stdin(&session, session_id, "", 100)
|
||||
let err = write_stdin(&session, process_id, "", 100)
|
||||
.await
|
||||
.expect_err("expected unknown session error");
|
||||
|
||||
match err {
|
||||
UnifiedExecError::UnknownSessionId { session_id: err_id } => {
|
||||
assert_eq!(err_id, session_id);
|
||||
UnifiedExecError::UnknownSessionId { process_id: err_id } => {
|
||||
assert_eq!(err_id, process_id, "process id should match request");
|
||||
}
|
||||
other => panic!("expected UnknownSessionId, got {other:?}"),
|
||||
}
|
||||
|
||||
assert!(
|
||||
!session
|
||||
session
|
||||
.services
|
||||
.unified_exec_manager
|
||||
.sessions
|
||||
.session_store
|
||||
.lock()
|
||||
.await
|
||||
.contains_key(&session_id)
|
||||
.sessions
|
||||
.is_empty()
|
||||
);
|
||||
|
||||
Ok(())
|
||||
|
||||
@@ -1,9 +1,9 @@
|
||||
use rand::Rng;
|
||||
use std::cmp::Reverse;
|
||||
use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
|
||||
use tokio::sync::Notify;
|
||||
use tokio::sync::mpsc;
|
||||
use tokio::time::Duration;
|
||||
@@ -36,6 +36,7 @@ use crate::truncate::formatted_truncate_text;
|
||||
use super::ExecCommandRequest;
|
||||
use super::MAX_UNIFIED_EXEC_SESSIONS;
|
||||
use super::SessionEntry;
|
||||
use super::SessionStore;
|
||||
use super::UnifiedExecContext;
|
||||
use super::UnifiedExecError;
|
||||
use super::UnifiedExecResponse;
|
||||
@@ -75,9 +76,39 @@ struct PreparedSessionHandles {
|
||||
turn_ref: Arc<TurnContext>,
|
||||
command: Vec<String>,
|
||||
cwd: PathBuf,
|
||||
process_id: String,
|
||||
}
|
||||
|
||||
impl UnifiedExecSessionManager {
|
||||
pub(crate) async fn allocate_process_id(&self) -> String {
|
||||
loop {
|
||||
let mut store = self.session_store.lock().await;
|
||||
|
||||
let process_id = if !cfg!(test) && !cfg!(feature = "deterministic_process_ids") {
|
||||
// production mode → random
|
||||
rand::rng().random_range(1_000..100_000).to_string()
|
||||
} else {
|
||||
// test or deterministic mode
|
||||
let next = store
|
||||
.reserved_sessions_id
|
||||
.iter()
|
||||
.filter_map(|s| s.parse::<i32>().ok())
|
||||
.max()
|
||||
.map(|m| std::cmp::max(m, 999) + 1)
|
||||
.unwrap_or(1000);
|
||||
|
||||
next.to_string()
|
||||
};
|
||||
|
||||
if store.reserved_sessions_id.contains(&process_id) {
|
||||
continue;
|
||||
}
|
||||
|
||||
store.reserved_sessions_id.insert(process_id.clone());
|
||||
return process_id;
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn exec_command(
|
||||
&self,
|
||||
request: ExecCommandRequest,
|
||||
@@ -122,14 +153,20 @@ impl UnifiedExecSessionManager {
|
||||
let has_exited = session.has_exited();
|
||||
let exit_code = session.exit_code();
|
||||
let chunk_id = generate_chunk_id();
|
||||
let session_id = if has_exited {
|
||||
let process_id = if has_exited {
|
||||
None
|
||||
} else {
|
||||
// Only store session if not exited.
|
||||
let stored_id = self
|
||||
.store_session(session, context, &request.command, cwd.clone(), start)
|
||||
.await;
|
||||
Some(stored_id)
|
||||
self.store_session(
|
||||
session,
|
||||
context,
|
||||
&request.command,
|
||||
cwd.clone(),
|
||||
start,
|
||||
request.process_id.clone(),
|
||||
)
|
||||
.await;
|
||||
Some(request.process_id.clone())
|
||||
};
|
||||
let original_token_count = approx_token_count(&text);
|
||||
|
||||
@@ -138,18 +175,18 @@ impl UnifiedExecSessionManager {
|
||||
chunk_id,
|
||||
wall_time,
|
||||
output,
|
||||
session_id,
|
||||
process_id: process_id.clone(),
|
||||
exit_code,
|
||||
original_token_count: Some(original_token_count),
|
||||
session_command: Some(request.command.clone()),
|
||||
};
|
||||
|
||||
if response.session_id.is_some() {
|
||||
if !has_exited {
|
||||
Self::emit_waiting_status(&context.session, &context.turn, &request.command).await;
|
||||
}
|
||||
|
||||
// If the command completed during this call, emit an ExecCommandEnd via the emitter.
|
||||
if response.session_id.is_none() {
|
||||
if has_exited {
|
||||
let exit = response.exit_code.unwrap_or(-1);
|
||||
Self::emit_exec_end_from_context(
|
||||
context,
|
||||
@@ -158,6 +195,9 @@ impl UnifiedExecSessionManager {
|
||||
response.output.clone(),
|
||||
exit,
|
||||
response.wall_time,
|
||||
// We always emit the process ID in order to keep consistency between the Begin
|
||||
// event and the End event.
|
||||
Some(request.process_id),
|
||||
)
|
||||
.await;
|
||||
}
|
||||
@@ -169,7 +209,7 @@ impl UnifiedExecSessionManager {
|
||||
&self,
|
||||
request: WriteStdinRequest<'_>,
|
||||
) -> Result<UnifiedExecResponse, UnifiedExecError> {
|
||||
let session_id = request.session_id;
|
||||
let process_id = request.process_id.to_string();
|
||||
|
||||
let PreparedSessionHandles {
|
||||
writer_tx,
|
||||
@@ -180,13 +220,15 @@ impl UnifiedExecSessionManager {
|
||||
turn_ref,
|
||||
command: session_command,
|
||||
cwd: session_cwd,
|
||||
} = self.prepare_session_handles(session_id).await?;
|
||||
process_id,
|
||||
} = self.prepare_session_handles(process_id.as_str()).await?;
|
||||
|
||||
let interaction_emitter = ToolEmitter::unified_exec(
|
||||
&session_command,
|
||||
session_cwd.clone(),
|
||||
ExecCommandSource::UnifiedExecInteraction,
|
||||
(!request.input.is_empty()).then(|| request.input.to_string()),
|
||||
Some(process_id.clone()),
|
||||
);
|
||||
let make_event_ctx = || {
|
||||
ToolEventCtx::new(
|
||||
@@ -233,17 +275,21 @@ impl UnifiedExecSessionManager {
|
||||
let original_token_count = approx_token_count(&text);
|
||||
let chunk_id = generate_chunk_id();
|
||||
|
||||
let status = self.refresh_session_state(session_id).await;
|
||||
let (session_id, exit_code, completion_entry, event_call_id) = match status {
|
||||
SessionStatus::Alive { exit_code, call_id } => {
|
||||
(Some(session_id), exit_code, None, call_id)
|
||||
}
|
||||
let status = self.refresh_session_state(process_id.as_str()).await;
|
||||
let (process_id, exit_code, completion_entry, event_call_id) = match status {
|
||||
SessionStatus::Alive {
|
||||
exit_code,
|
||||
call_id,
|
||||
process_id,
|
||||
} => (Some(process_id), exit_code, None, call_id),
|
||||
SessionStatus::Exited { exit_code, entry } => {
|
||||
let call_id = entry.call_id.clone();
|
||||
(None, exit_code, Some(*entry), call_id)
|
||||
}
|
||||
SessionStatus::Unknown => {
|
||||
return Err(UnifiedExecError::UnknownSessionId { session_id });
|
||||
return Err(UnifiedExecError::UnknownSessionId {
|
||||
process_id: request.process_id.to_string(),
|
||||
});
|
||||
}
|
||||
};
|
||||
|
||||
@@ -252,7 +298,7 @@ impl UnifiedExecSessionManager {
|
||||
chunk_id,
|
||||
wall_time,
|
||||
output,
|
||||
session_id,
|
||||
process_id,
|
||||
exit_code,
|
||||
original_token_count: Some(original_token_count),
|
||||
session_command: Some(session_command.clone()),
|
||||
@@ -273,7 +319,7 @@ impl UnifiedExecSessionManager {
|
||||
)
|
||||
.await;
|
||||
|
||||
if response.session_id.is_some() {
|
||||
if response.process_id.is_some() {
|
||||
Self::emit_waiting_status(&session_ref, &turn_ref, &session_command).await;
|
||||
}
|
||||
|
||||
@@ -286,16 +332,17 @@ impl UnifiedExecSessionManager {
|
||||
Ok(response)
|
||||
}
|
||||
|
||||
async fn refresh_session_state(&self, session_id: i32) -> SessionStatus {
|
||||
let mut sessions = self.sessions.lock().await;
|
||||
let Some(entry) = sessions.get(&session_id) else {
|
||||
async fn refresh_session_state(&self, process_id: &str) -> SessionStatus {
|
||||
let mut store = self.session_store.lock().await;
|
||||
let Some(entry) = store.sessions.get(process_id) else {
|
||||
return SessionStatus::Unknown;
|
||||
};
|
||||
|
||||
let exit_code = entry.session.exit_code();
|
||||
let process_id = entry.process_id.clone();
|
||||
|
||||
if entry.session.has_exited() {
|
||||
let Some(entry) = sessions.remove(&session_id) else {
|
||||
let Some(entry) = store.remove(&process_id) else {
|
||||
return SessionStatus::Unknown;
|
||||
};
|
||||
SessionStatus::Exited {
|
||||
@@ -306,18 +353,23 @@ impl UnifiedExecSessionManager {
|
||||
SessionStatus::Alive {
|
||||
exit_code,
|
||||
call_id: entry.call_id.clone(),
|
||||
process_id,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
async fn prepare_session_handles(
|
||||
&self,
|
||||
session_id: i32,
|
||||
process_id: &str,
|
||||
) -> Result<PreparedSessionHandles, UnifiedExecError> {
|
||||
let mut sessions = self.sessions.lock().await;
|
||||
let entry = sessions
|
||||
.get_mut(&session_id)
|
||||
.ok_or(UnifiedExecError::UnknownSessionId { session_id })?;
|
||||
let mut store = self.session_store.lock().await;
|
||||
let entry =
|
||||
store
|
||||
.sessions
|
||||
.get_mut(process_id)
|
||||
.ok_or(UnifiedExecError::UnknownSessionId {
|
||||
process_id: process_id.to_string(),
|
||||
})?;
|
||||
entry.last_used = Instant::now();
|
||||
let OutputHandles {
|
||||
output_buffer,
|
||||
@@ -334,6 +386,7 @@ impl UnifiedExecSessionManager {
|
||||
turn_ref: Arc::clone(&entry.turn_ref),
|
||||
command: entry.command.clone(),
|
||||
cwd: entry.cwd.clone(),
|
||||
process_id: entry.process_id.clone(),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -347,6 +400,7 @@ impl UnifiedExecSessionManager {
|
||||
.map_err(|_| UnifiedExecError::WriteToStdin)
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
async fn store_session(
|
||||
&self,
|
||||
session: UnifiedExecSession,
|
||||
@@ -354,24 +408,22 @@ impl UnifiedExecSessionManager {
|
||||
command: &[String],
|
||||
cwd: PathBuf,
|
||||
started_at: Instant,
|
||||
) -> i32 {
|
||||
let session_id = self
|
||||
.next_session_id
|
||||
.fetch_add(1, std::sync::atomic::Ordering::SeqCst);
|
||||
process_id: String,
|
||||
) {
|
||||
let entry = SessionEntry {
|
||||
session,
|
||||
session_ref: Arc::clone(&context.session),
|
||||
turn_ref: Arc::clone(&context.turn),
|
||||
call_id: context.call_id.clone(),
|
||||
process_id: process_id.clone(),
|
||||
command: command.to_vec(),
|
||||
cwd,
|
||||
started_at,
|
||||
last_used: started_at,
|
||||
};
|
||||
let mut sessions = self.sessions.lock().await;
|
||||
Self::prune_sessions_if_needed(&mut sessions);
|
||||
sessions.insert(session_id, entry);
|
||||
session_id
|
||||
let mut store = self.session_store.lock().await;
|
||||
Self::prune_sessions_if_needed(&mut store);
|
||||
store.sessions.insert(process_id, entry);
|
||||
}
|
||||
|
||||
async fn emit_exec_end_from_entry(
|
||||
@@ -399,6 +451,7 @@ impl UnifiedExecSessionManager {
|
||||
entry.cwd,
|
||||
ExecCommandSource::UnifiedExecStartup,
|
||||
None,
|
||||
Some(entry.process_id.clone()),
|
||||
);
|
||||
emitter
|
||||
.emit(event_ctx, ToolEventStage::Success(output))
|
||||
@@ -412,6 +465,7 @@ impl UnifiedExecSessionManager {
|
||||
aggregated_output: String,
|
||||
exit_code: i32,
|
||||
duration: Duration,
|
||||
process_id: Option<String>,
|
||||
) {
|
||||
let output = ExecToolCallOutput {
|
||||
exit_code,
|
||||
@@ -427,8 +481,13 @@ impl UnifiedExecSessionManager {
|
||||
&context.call_id,
|
||||
None,
|
||||
);
|
||||
let emitter =
|
||||
ToolEmitter::unified_exec(command, cwd, ExecCommandSource::UnifiedExecStartup, None);
|
||||
let emitter = ToolEmitter::unified_exec(
|
||||
command,
|
||||
cwd,
|
||||
ExecCommandSource::UnifiedExecStartup,
|
||||
None,
|
||||
process_id,
|
||||
);
|
||||
emitter
|
||||
.emit(event_ctx, ToolEventStage::Success(output))
|
||||
.await;
|
||||
@@ -574,52 +633,53 @@ impl UnifiedExecSessionManager {
|
||||
collected
|
||||
}
|
||||
|
||||
fn prune_sessions_if_needed(sessions: &mut HashMap<i32, SessionEntry>) {
|
||||
if sessions.len() < MAX_UNIFIED_EXEC_SESSIONS {
|
||||
fn prune_sessions_if_needed(store: &mut SessionStore) {
|
||||
if store.sessions.len() < MAX_UNIFIED_EXEC_SESSIONS {
|
||||
return;
|
||||
}
|
||||
|
||||
let meta: Vec<(i32, Instant, bool)> = sessions
|
||||
let meta: Vec<(String, Instant, bool)> = store
|
||||
.sessions
|
||||
.iter()
|
||||
.map(|(id, entry)| (*id, entry.last_used, entry.session.has_exited()))
|
||||
.map(|(id, entry)| (id.clone(), entry.last_used, entry.session.has_exited()))
|
||||
.collect();
|
||||
|
||||
if let Some(session_id) = Self::session_id_to_prune_from_meta(&meta) {
|
||||
sessions.remove(&session_id);
|
||||
store.remove(&session_id);
|
||||
}
|
||||
}
|
||||
|
||||
// Centralized pruning policy so we can easily swap strategies later.
|
||||
fn session_id_to_prune_from_meta(meta: &[(i32, Instant, bool)]) -> Option<i32> {
|
||||
fn session_id_to_prune_from_meta(meta: &[(String, Instant, bool)]) -> Option<String> {
|
||||
if meta.is_empty() {
|
||||
return None;
|
||||
}
|
||||
|
||||
let mut by_recency = meta.to_vec();
|
||||
by_recency.sort_by_key(|(_, last_used, _)| Reverse(*last_used));
|
||||
let protected: HashSet<i32> = by_recency
|
||||
let protected: HashSet<String> = by_recency
|
||||
.iter()
|
||||
.take(8)
|
||||
.map(|(session_id, _, _)| *session_id)
|
||||
.map(|(process_id, _, _)| process_id.clone())
|
||||
.collect();
|
||||
|
||||
let mut lru = meta.to_vec();
|
||||
lru.sort_by_key(|(_, last_used, _)| *last_used);
|
||||
|
||||
if let Some((session_id, _, _)) = lru
|
||||
if let Some((process_id, _, _)) = lru
|
||||
.iter()
|
||||
.find(|(session_id, _, exited)| !protected.contains(session_id) && *exited)
|
||||
.find(|(process_id, _, exited)| !protected.contains(process_id) && *exited)
|
||||
{
|
||||
return Some(*session_id);
|
||||
return Some(process_id.clone());
|
||||
}
|
||||
|
||||
lru.into_iter()
|
||||
.find(|(session_id, _, _)| !protected.contains(session_id))
|
||||
.map(|(session_id, _, _)| session_id)
|
||||
.find(|(process_id, _, _)| !protected.contains(process_id))
|
||||
.map(|(process_id, _, _)| process_id)
|
||||
}
|
||||
|
||||
pub(crate) async fn terminate_all_sessions(&self) {
|
||||
let mut sessions = self.sessions.lock().await;
|
||||
let mut sessions = self.session_store.lock().await;
|
||||
sessions.clear();
|
||||
}
|
||||
}
|
||||
@@ -628,6 +688,7 @@ enum SessionStatus {
|
||||
Alive {
|
||||
exit_code: Option<i32>,
|
||||
call_id: String,
|
||||
process_id: String,
|
||||
},
|
||||
Exited {
|
||||
exit_code: Option<i32>,
|
||||
@@ -675,64 +736,67 @@ mod tests {
|
||||
#[test]
|
||||
fn pruning_prefers_exited_sessions_outside_recently_used() {
|
||||
let now = Instant::now();
|
||||
let id = |n: i32| n.to_string();
|
||||
let meta = vec![
|
||||
(1, now - Duration::from_secs(40), false),
|
||||
(2, now - Duration::from_secs(30), true),
|
||||
(3, now - Duration::from_secs(20), false),
|
||||
(4, now - Duration::from_secs(19), false),
|
||||
(5, now - Duration::from_secs(18), false),
|
||||
(6, now - Duration::from_secs(17), false),
|
||||
(7, now - Duration::from_secs(16), false),
|
||||
(8, now - Duration::from_secs(15), false),
|
||||
(9, now - Duration::from_secs(14), false),
|
||||
(10, now - Duration::from_secs(13), false),
|
||||
(id(1), now - Duration::from_secs(40), false),
|
||||
(id(2), now - Duration::from_secs(30), true),
|
||||
(id(3), now - Duration::from_secs(20), false),
|
||||
(id(4), now - Duration::from_secs(19), false),
|
||||
(id(5), now - Duration::from_secs(18), false),
|
||||
(id(6), now - Duration::from_secs(17), false),
|
||||
(id(7), now - Duration::from_secs(16), false),
|
||||
(id(8), now - Duration::from_secs(15), false),
|
||||
(id(9), now - Duration::from_secs(14), false),
|
||||
(id(10), now - Duration::from_secs(13), false),
|
||||
];
|
||||
|
||||
let candidate = UnifiedExecSessionManager::session_id_to_prune_from_meta(&meta);
|
||||
|
||||
assert_eq!(candidate, Some(2));
|
||||
assert_eq!(candidate, Some(id(2)));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pruning_falls_back_to_lru_when_no_exited() {
|
||||
let now = Instant::now();
|
||||
let id = |n: i32| n.to_string();
|
||||
let meta = vec![
|
||||
(1, now - Duration::from_secs(40), false),
|
||||
(2, now - Duration::from_secs(30), false),
|
||||
(3, now - Duration::from_secs(20), false),
|
||||
(4, now - Duration::from_secs(19), false),
|
||||
(5, now - Duration::from_secs(18), false),
|
||||
(6, now - Duration::from_secs(17), false),
|
||||
(7, now - Duration::from_secs(16), false),
|
||||
(8, now - Duration::from_secs(15), false),
|
||||
(9, now - Duration::from_secs(14), false),
|
||||
(10, now - Duration::from_secs(13), false),
|
||||
(id(1), now - Duration::from_secs(40), false),
|
||||
(id(2), now - Duration::from_secs(30), false),
|
||||
(id(3), now - Duration::from_secs(20), false),
|
||||
(id(4), now - Duration::from_secs(19), false),
|
||||
(id(5), now - Duration::from_secs(18), false),
|
||||
(id(6), now - Duration::from_secs(17), false),
|
||||
(id(7), now - Duration::from_secs(16), false),
|
||||
(id(8), now - Duration::from_secs(15), false),
|
||||
(id(9), now - Duration::from_secs(14), false),
|
||||
(id(10), now - Duration::from_secs(13), false),
|
||||
];
|
||||
|
||||
let candidate = UnifiedExecSessionManager::session_id_to_prune_from_meta(&meta);
|
||||
|
||||
assert_eq!(candidate, Some(1));
|
||||
assert_eq!(candidate, Some(id(1)));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pruning_protects_recent_sessions_even_if_exited() {
|
||||
let now = Instant::now();
|
||||
let id = |n: i32| n.to_string();
|
||||
let meta = vec![
|
||||
(1, now - Duration::from_secs(40), false),
|
||||
(2, now - Duration::from_secs(30), false),
|
||||
(3, now - Duration::from_secs(20), true),
|
||||
(4, now - Duration::from_secs(19), false),
|
||||
(5, now - Duration::from_secs(18), false),
|
||||
(6, now - Duration::from_secs(17), false),
|
||||
(7, now - Duration::from_secs(16), false),
|
||||
(8, now - Duration::from_secs(15), false),
|
||||
(9, now - Duration::from_secs(14), false),
|
||||
(10, now - Duration::from_secs(13), true),
|
||||
(id(1), now - Duration::from_secs(40), false),
|
||||
(id(2), now - Duration::from_secs(30), false),
|
||||
(id(3), now - Duration::from_secs(20), true),
|
||||
(id(4), now - Duration::from_secs(19), false),
|
||||
(id(5), now - Duration::from_secs(18), false),
|
||||
(id(6), now - Duration::from_secs(17), false),
|
||||
(id(7), now - Duration::from_secs(16), false),
|
||||
(id(8), now - Duration::from_secs(15), false),
|
||||
(id(9), now - Duration::from_secs(14), false),
|
||||
(id(10), now - Duration::from_secs(13), true),
|
||||
];
|
||||
|
||||
let candidate = UnifiedExecSessionManager::session_id_to_prune_from_meta(&meta);
|
||||
|
||||
// (10) is exited but among the last 8; we should drop the LRU outside that set.
|
||||
assert_eq!(candidate, Some(1));
|
||||
assert_eq!(candidate, Some(id(1)));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -70,7 +70,6 @@ async fn codex_delegate_forwards_exec_approval_and_proceeds_on_approval() {
|
||||
review_request: ReviewRequest {
|
||||
prompt: "Please review".to_string(),
|
||||
user_facing_hint: "review".to_string(),
|
||||
append_to_original_thread: true,
|
||||
},
|
||||
})
|
||||
.await
|
||||
@@ -146,7 +145,6 @@ async fn codex_delegate_forwards_patch_approval_and_proceeds_on_decision() {
|
||||
review_request: ReviewRequest {
|
||||
prompt: "Please review".to_string(),
|
||||
user_facing_hint: "review".to_string(),
|
||||
append_to_original_thread: true,
|
||||
},
|
||||
})
|
||||
.await
|
||||
@@ -201,7 +199,6 @@ async fn codex_delegate_ignores_legacy_deltas() {
|
||||
review_request: ReviewRequest {
|
||||
prompt: "Please review".to_string(),
|
||||
user_facing_hint: "review".to_string(),
|
||||
append_to_original_thread: true,
|
||||
},
|
||||
})
|
||||
.await
|
||||
|
||||
@@ -18,6 +18,7 @@ use codex_core::protocol::ReviewOutputEvent;
|
||||
use codex_core::protocol::ReviewRequest;
|
||||
use codex_core::protocol::RolloutItem;
|
||||
use codex_core::protocol::RolloutLine;
|
||||
use codex_core::review_format::render_review_output_text;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use core_test_support::load_default_config_for_test;
|
||||
use core_test_support::load_sse_fixture_with_id_from_str;
|
||||
@@ -82,7 +83,6 @@ async fn review_op_emits_lifecycle_and_review_output() {
|
||||
review_request: ReviewRequest {
|
||||
prompt: "Please review my changes".to_string(),
|
||||
user_facing_hint: "my changes".to_string(),
|
||||
append_to_original_thread: true,
|
||||
},
|
||||
})
|
||||
.await
|
||||
@@ -124,22 +124,36 @@ async fn review_op_emits_lifecycle_and_review_output() {
|
||||
|
||||
let mut saw_header = false;
|
||||
let mut saw_finding_line = false;
|
||||
let expected_assistant_text = render_review_output_text(&expected);
|
||||
let mut saw_assistant_plain = false;
|
||||
let mut saw_assistant_xml = false;
|
||||
for line in text.lines() {
|
||||
if line.trim().is_empty() {
|
||||
continue;
|
||||
}
|
||||
let v: serde_json::Value = serde_json::from_str(line).expect("jsonl line");
|
||||
let rl: RolloutLine = serde_json::from_value(v).expect("rollout line");
|
||||
if let RolloutItem::ResponseItem(ResponseItem::Message { role, content, .. }) = rl.item
|
||||
&& role == "user"
|
||||
{
|
||||
for c in content {
|
||||
if let ContentItem::InputText { text } = c {
|
||||
if text.contains("full review output from reviewer model") {
|
||||
saw_header = true;
|
||||
if let RolloutItem::ResponseItem(ResponseItem::Message { role, content, .. }) = rl.item {
|
||||
if role == "user" {
|
||||
for c in content {
|
||||
if let ContentItem::InputText { text } = c {
|
||||
if text.contains("full review output from reviewer model") {
|
||||
saw_header = true;
|
||||
}
|
||||
if text.contains("- Prefer Stylize helpers — /tmp/file.rs:10-20") {
|
||||
saw_finding_line = true;
|
||||
}
|
||||
}
|
||||
if text.contains("- Prefer Stylize helpers — /tmp/file.rs:10-20") {
|
||||
saw_finding_line = true;
|
||||
}
|
||||
} else if role == "assistant" {
|
||||
for c in content {
|
||||
if let ContentItem::OutputText { text } = c {
|
||||
if text.contains("<user_action>") {
|
||||
saw_assistant_xml = true;
|
||||
}
|
||||
if text == expected_assistant_text {
|
||||
saw_assistant_plain = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -150,6 +164,14 @@ async fn review_op_emits_lifecycle_and_review_output() {
|
||||
saw_finding_line,
|
||||
"formatted finding line missing from rollout"
|
||||
);
|
||||
assert!(
|
||||
saw_assistant_plain,
|
||||
"assistant review output missing from rollout"
|
||||
);
|
||||
assert!(
|
||||
!saw_assistant_xml,
|
||||
"assistant review output contains user_action markup"
|
||||
);
|
||||
|
||||
server.verify().await;
|
||||
}
|
||||
@@ -179,7 +201,6 @@ async fn review_op_with_plain_text_emits_review_fallback() {
|
||||
review_request: ReviewRequest {
|
||||
prompt: "Plain text review".to_string(),
|
||||
user_facing_hint: "plain text review".to_string(),
|
||||
append_to_original_thread: true,
|
||||
},
|
||||
})
|
||||
.await
|
||||
@@ -238,7 +259,6 @@ async fn review_filters_agent_message_related_events() {
|
||||
review_request: ReviewRequest {
|
||||
prompt: "Filter streaming events".to_string(),
|
||||
user_facing_hint: "Filter streaming events".to_string(),
|
||||
append_to_original_thread: true,
|
||||
},
|
||||
})
|
||||
.await
|
||||
@@ -247,7 +267,7 @@ async fn review_filters_agent_message_related_events() {
|
||||
let mut saw_entered = false;
|
||||
let mut saw_exited = false;
|
||||
|
||||
// Drain until TaskComplete; assert filtered events never surface.
|
||||
// Drain until TaskComplete; assert streaming-related events never surface.
|
||||
wait_for_event(&codex, |event| match event {
|
||||
EventMsg::TaskComplete(_) => true,
|
||||
EventMsg::EnteredReviewMode(_) => {
|
||||
@@ -265,12 +285,6 @@ async fn review_filters_agent_message_related_events() {
|
||||
EventMsg::AgentMessageDelta(_) => {
|
||||
panic!("unexpected AgentMessageDelta surfaced during review")
|
||||
}
|
||||
EventMsg::ItemCompleted(ev) => match &ev.item {
|
||||
codex_protocol::items::TurnItem::AgentMessage(_) => {
|
||||
panic!("unexpected ItemCompleted for TurnItem::AgentMessage surfaced during review")
|
||||
}
|
||||
_ => false,
|
||||
},
|
||||
_ => false,
|
||||
})
|
||||
.await;
|
||||
@@ -279,8 +293,9 @@ async fn review_filters_agent_message_related_events() {
|
||||
server.verify().await;
|
||||
}
|
||||
|
||||
/// When the model returns structured JSON in a review, ensure no AgentMessage
|
||||
/// is emitted; the UI consumes the structured result via ExitedReviewMode.
|
||||
/// When the model returns structured JSON in a review, ensure only a single
|
||||
/// non-streaming AgentMessage is emitted; the UI consumes the structured
|
||||
/// result via ExitedReviewMode plus a final assistant message.
|
||||
// Windows CI only: bump to 4 workers to prevent SSE/event starvation and test timeouts.
|
||||
#[cfg_attr(windows, tokio::test(flavor = "multi_thread", worker_threads = 4))]
|
||||
#[cfg_attr(not(windows), tokio::test(flavor = "multi_thread", worker_threads = 2))]
|
||||
@@ -323,19 +338,21 @@ async fn review_does_not_emit_agent_message_on_structured_output() {
|
||||
review_request: ReviewRequest {
|
||||
prompt: "check structured".to_string(),
|
||||
user_facing_hint: "check structured".to_string(),
|
||||
append_to_original_thread: true,
|
||||
},
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
// Drain events until TaskComplete; ensure none are AgentMessage.
|
||||
// Drain events until TaskComplete; ensure we only see a final
|
||||
// AgentMessage (no streaming assistant messages).
|
||||
let mut saw_entered = false;
|
||||
let mut saw_exited = false;
|
||||
let mut agent_messages = 0;
|
||||
wait_for_event(&codex, |event| match event {
|
||||
EventMsg::TaskComplete(_) => true,
|
||||
EventMsg::AgentMessage(_) => {
|
||||
panic!("unexpected AgentMessage during review with structured output")
|
||||
agent_messages += 1;
|
||||
false
|
||||
}
|
||||
EventMsg::EnteredReviewMode(_) => {
|
||||
saw_entered = true;
|
||||
@@ -348,6 +365,7 @@ async fn review_does_not_emit_agent_message_on_structured_output() {
|
||||
_ => false,
|
||||
})
|
||||
.await;
|
||||
assert_eq!(1, agent_messages, "expected exactly one AgentMessage event");
|
||||
assert!(saw_entered && saw_exited, "missing review lifecycle events");
|
||||
|
||||
server.verify().await;
|
||||
@@ -377,7 +395,6 @@ async fn review_uses_custom_review_model_from_config() {
|
||||
review_request: ReviewRequest {
|
||||
prompt: "use custom model".to_string(),
|
||||
user_facing_hint: "use custom model".to_string(),
|
||||
append_to_original_thread: true,
|
||||
},
|
||||
})
|
||||
.await
|
||||
@@ -495,7 +512,6 @@ async fn review_input_isolated_from_parent_history() {
|
||||
review_request: ReviewRequest {
|
||||
prompt: review_prompt.clone(),
|
||||
user_facing_hint: review_prompt.clone(),
|
||||
append_to_original_thread: true,
|
||||
},
|
||||
})
|
||||
.await
|
||||
@@ -583,11 +599,10 @@ async fn review_input_isolated_from_parent_history() {
|
||||
server.verify().await;
|
||||
}
|
||||
|
||||
/// After a review thread finishes, its conversation should not leak into the
|
||||
/// parent session. A subsequent parent turn must not include any review
|
||||
/// messages in its request `input`.
|
||||
/// After a review thread finishes, its conversation should be visible in the
|
||||
/// parent session so later turns can reference the results.
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn review_history_does_not_leak_into_parent_session() {
|
||||
async fn review_history_surfaces_in_parent_session() {
|
||||
skip_if_no_network!();
|
||||
|
||||
// Respond to both the review request and the subsequent parent request.
|
||||
@@ -608,7 +623,6 @@ async fn review_history_does_not_leak_into_parent_session() {
|
||||
review_request: ReviewRequest {
|
||||
prompt: "Start a review".to_string(),
|
||||
user_facing_hint: "Start a review".to_string(),
|
||||
append_to_original_thread: true,
|
||||
},
|
||||
})
|
||||
.await
|
||||
@@ -651,20 +665,26 @@ async fn review_history_does_not_leak_into_parent_session() {
|
||||
let last_text = last["content"][0]["text"].as_str().unwrap();
|
||||
assert_eq!(last_text, followup);
|
||||
|
||||
// Ensure no review-thread content leaked into the parent request
|
||||
let contains_review_prompt = input
|
||||
.iter()
|
||||
.any(|msg| msg["content"][0]["text"].as_str().unwrap_or_default() == "Start a review");
|
||||
// Ensure review-thread content is present for downstream turns.
|
||||
let contains_review_rollout_user = input.iter().any(|msg| {
|
||||
msg["content"][0]["text"]
|
||||
.as_str()
|
||||
.unwrap_or_default()
|
||||
.contains("User initiated a review task.")
|
||||
});
|
||||
let contains_review_assistant = input.iter().any(|msg| {
|
||||
msg["content"][0]["text"].as_str().unwrap_or_default() == "review assistant output"
|
||||
msg["content"][0]["text"]
|
||||
.as_str()
|
||||
.unwrap_or_default()
|
||||
.contains("review assistant output")
|
||||
});
|
||||
assert!(
|
||||
!contains_review_prompt,
|
||||
"review prompt leaked into parent turn input"
|
||||
contains_review_rollout_user,
|
||||
"review rollout user message missing from parent turn input"
|
||||
);
|
||||
assert!(
|
||||
!contains_review_assistant,
|
||||
"review assistant output leaked into parent turn input"
|
||||
contains_review_assistant,
|
||||
"review assistant output missing from parent turn input"
|
||||
);
|
||||
|
||||
server.verify().await;
|
||||
|
||||
@@ -44,7 +44,7 @@ fn extract_output_text(item: &Value) -> Option<&str> {
|
||||
struct ParsedUnifiedExecOutput {
|
||||
chunk_id: Option<String>,
|
||||
wall_time_seconds: f64,
|
||||
session_id: Option<i32>,
|
||||
process_id: Option<String>,
|
||||
exit_code: Option<i32>,
|
||||
original_token_count: Option<usize>,
|
||||
output: String,
|
||||
@@ -59,7 +59,7 @@ fn parse_unified_exec_output(raw: &str) -> Result<ParsedUnifiedExecOutput> {
|
||||
r#"(?:Chunk ID: (?P<chunk_id>[^\n]+)\n)?"#,
|
||||
r#"Wall time: (?P<wall_time>-?\d+(?:\.\d+)?) seconds\n"#,
|
||||
r#"(?:Process exited with code (?P<exit_code>-?\d+)\n)?"#,
|
||||
r#"(?:Process running with session ID (?P<session_id>-?\d+)\n)?"#,
|
||||
r#"(?:Process running with session ID (?P<process_id>-?\d+)\n)?"#,
|
||||
r#"(?:Original token count: (?P<original_token_count>\d+)\n)?"#,
|
||||
r#"Output:\n?(?P<output>.*)$"#,
|
||||
))
|
||||
@@ -92,15 +92,9 @@ fn parse_unified_exec_output(raw: &str) -> Result<ParsedUnifiedExecOutput> {
|
||||
})
|
||||
.transpose()?;
|
||||
|
||||
let session_id = captures
|
||||
.name("session_id")
|
||||
.map(|value| {
|
||||
value
|
||||
.as_str()
|
||||
.parse::<i32>()
|
||||
.context("failed to parse session id from unified exec output")
|
||||
})
|
||||
.transpose()?;
|
||||
let process_id = captures
|
||||
.name("process_id")
|
||||
.map(|value| value.as_str().to_string());
|
||||
|
||||
let original_token_count = captures
|
||||
.name("original_token_count")
|
||||
@@ -121,7 +115,7 @@ fn parse_unified_exec_output(raw: &str) -> Result<ParsedUnifiedExecOutput> {
|
||||
Ok(ParsedUnifiedExecOutput {
|
||||
chunk_id,
|
||||
wall_time_seconds,
|
||||
session_id,
|
||||
process_id,
|
||||
exit_code,
|
||||
original_token_count,
|
||||
output,
|
||||
@@ -335,7 +329,7 @@ async fn unified_exec_emits_exec_command_end_event() -> Result<()> {
|
||||
let poll_call_id = "uexec-end-event-poll";
|
||||
let poll_args = json!({
|
||||
"chars": "",
|
||||
"session_id": 0,
|
||||
"session_id": 1000,
|
||||
"yield_time_ms": 250,
|
||||
});
|
||||
|
||||
@@ -493,7 +487,7 @@ async fn unified_exec_emits_output_delta_for_write_stdin() -> Result<()> {
|
||||
let stdin_call_id = "uexec-stdin-delta";
|
||||
let stdin_args = json!({
|
||||
"chars": "echo WSTDIN-MARK\\n",
|
||||
"session_id": 0,
|
||||
"session_id": 1000,
|
||||
"yield_time_ms": 800,
|
||||
});
|
||||
|
||||
@@ -592,7 +586,7 @@ async fn unified_exec_emits_begin_for_write_stdin() -> Result<()> {
|
||||
let stdin_call_id = "uexec-stdin-begin";
|
||||
let stdin_args = json!({
|
||||
"chars": "echo hello",
|
||||
"session_id": 0,
|
||||
"session_id": 1000,
|
||||
"yield_time_ms": 400,
|
||||
});
|
||||
|
||||
@@ -694,7 +688,7 @@ async fn unified_exec_emits_begin_event_for_write_stdin_requests() -> Result<()>
|
||||
let poll_call_id = "uexec-poll-empty";
|
||||
let poll_args = json!({
|
||||
"chars": "",
|
||||
"session_id": 0,
|
||||
"session_id": 1000,
|
||||
"yield_time_ms": 150,
|
||||
});
|
||||
|
||||
@@ -880,8 +874,8 @@ async fn exec_command_reports_chunk_and_exit_metadata() -> Result<()> {
|
||||
);
|
||||
|
||||
assert!(
|
||||
metadata.session_id.is_none(),
|
||||
"exec_command for a completed process should not include session_id"
|
||||
metadata.process_id.is_none(),
|
||||
"exec_command for a completed process should not include process_id"
|
||||
);
|
||||
|
||||
let exit_code = metadata.exit_code.expect("expected exit_code");
|
||||
@@ -973,7 +967,7 @@ async fn unified_exec_respects_early_exit_notifications() -> Result<()> {
|
||||
.expect("missing early exit unified_exec output");
|
||||
|
||||
assert!(
|
||||
output.session_id.is_none(),
|
||||
output.process_id.is_none(),
|
||||
"short-lived process should not keep a session alive"
|
||||
);
|
||||
assert_eq!(
|
||||
@@ -1023,12 +1017,12 @@ async fn write_stdin_returns_exit_metadata_and_clears_session() -> Result<()> {
|
||||
});
|
||||
let send_args = serde_json::json!({
|
||||
"chars": "hello unified exec\n",
|
||||
"session_id": 0,
|
||||
"session_id": 1000,
|
||||
"yield_time_ms": 500,
|
||||
});
|
||||
let exit_args = serde_json::json!({
|
||||
"chars": "\u{0004}",
|
||||
"session_id": 0,
|
||||
"session_id": 1000,
|
||||
"yield_time_ms": 500,
|
||||
});
|
||||
|
||||
@@ -1099,12 +1093,13 @@ async fn write_stdin_returns_exit_metadata_and_clears_session() -> Result<()> {
|
||||
let start_output = outputs
|
||||
.get(start_call_id)
|
||||
.expect("missing start output for exec_command");
|
||||
let session_id = start_output
|
||||
.session_id
|
||||
.expect("expected session id from exec_command");
|
||||
let process_id = start_output
|
||||
.process_id
|
||||
.clone()
|
||||
.expect("expected process id from exec_command");
|
||||
assert!(
|
||||
session_id >= 0,
|
||||
"session_id should be non-negative, got {session_id}"
|
||||
process_id.len() > 3,
|
||||
"process_id should be at least 4 digits, got {process_id}"
|
||||
);
|
||||
assert!(
|
||||
start_output.exit_code.is_none(),
|
||||
@@ -1120,11 +1115,12 @@ async fn write_stdin_returns_exit_metadata_and_clears_session() -> Result<()> {
|
||||
"expected echoed output from cat, got {echoed:?}"
|
||||
);
|
||||
let echoed_session = send_output
|
||||
.session_id
|
||||
.expect("write_stdin should return session id while process is running");
|
||||
.process_id
|
||||
.clone()
|
||||
.expect("write_stdin should return process id while process is running");
|
||||
assert_eq!(
|
||||
echoed_session, session_id,
|
||||
"write_stdin should reuse existing session id"
|
||||
echoed_session, process_id,
|
||||
"write_stdin should reuse existing process id"
|
||||
);
|
||||
assert!(
|
||||
send_output.exit_code.is_none(),
|
||||
@@ -1135,8 +1131,8 @@ async fn write_stdin_returns_exit_metadata_and_clears_session() -> Result<()> {
|
||||
.get(exit_call_id)
|
||||
.expect("missing exit metadata output");
|
||||
assert!(
|
||||
exit_output.session_id.is_none(),
|
||||
"session_id should be omitted once the process exits"
|
||||
exit_output.process_id.is_none(),
|
||||
"process_id should be omitted once the process exits"
|
||||
);
|
||||
let exit_code = exit_output
|
||||
.exit_code
|
||||
@@ -1182,14 +1178,14 @@ async fn unified_exec_emits_end_event_when_session_dies_via_stdin() -> Result<()
|
||||
let echo_call_id = "uexec-end-on-exit-echo";
|
||||
let echo_args = serde_json::json!({
|
||||
"chars": "bye-END\n",
|
||||
"session_id": 0,
|
||||
"session_id": 1000,
|
||||
"yield_time_ms": 300,
|
||||
});
|
||||
|
||||
let exit_call_id = "uexec-end-on-exit";
|
||||
let exit_args = serde_json::json!({
|
||||
"chars": "\u{0004}",
|
||||
"session_id": 0,
|
||||
"session_id": 1000,
|
||||
"yield_time_ms": 500,
|
||||
});
|
||||
|
||||
@@ -1285,7 +1281,7 @@ async fn unified_exec_reuses_session_via_stdin() -> Result<()> {
|
||||
let second_call_id = "uexec-stdin";
|
||||
let second_args = serde_json::json!({
|
||||
"chars": "hello unified exec\n",
|
||||
"session_id": 0,
|
||||
"session_id": 1000,
|
||||
"yield_time_ms": 500,
|
||||
});
|
||||
|
||||
@@ -1347,17 +1343,20 @@ async fn unified_exec_reuses_session_via_stdin() -> Result<()> {
|
||||
let start_output = outputs
|
||||
.get(first_call_id)
|
||||
.expect("missing first unified_exec output");
|
||||
let session_id = start_output.session_id.unwrap_or_default();
|
||||
let process_id = start_output.process_id.clone().unwrap_or_default();
|
||||
assert!(
|
||||
session_id >= 0,
|
||||
"expected session id in first unified_exec response"
|
||||
!process_id.is_empty(),
|
||||
"expected process id in first unified_exec response"
|
||||
);
|
||||
assert!(start_output.output.is_empty());
|
||||
|
||||
let reuse_output = outputs
|
||||
.get(second_call_id)
|
||||
.expect("missing reused unified_exec output");
|
||||
assert_eq!(reuse_output.session_id.unwrap_or_default(), session_id);
|
||||
assert_eq!(
|
||||
reuse_output.process_id.clone().unwrap_or_default(),
|
||||
process_id
|
||||
);
|
||||
let echoed = reuse_output.output.as_str();
|
||||
assert!(
|
||||
echoed.contains("hello unified exec"),
|
||||
@@ -1413,7 +1412,7 @@ PY
|
||||
let second_call_id = "uexec-lag-poll";
|
||||
let second_args = serde_json::json!({
|
||||
"chars": "",
|
||||
"session_id": 0,
|
||||
"session_id": 1000,
|
||||
"yield_time_ms": 2_000,
|
||||
});
|
||||
|
||||
@@ -1480,9 +1479,9 @@ PY
|
||||
let start_output = outputs
|
||||
.get(first_call_id)
|
||||
.expect("missing initial unified_exec output");
|
||||
let session_id = start_output.session_id.unwrap_or_default();
|
||||
let process_id = start_output.process_id.clone().unwrap_or_default();
|
||||
assert!(
|
||||
session_id >= 0,
|
||||
!process_id.is_empty(),
|
||||
"expected session id from initial unified_exec response"
|
||||
);
|
||||
|
||||
@@ -1524,7 +1523,7 @@ async fn unified_exec_timeout_and_followup_poll() -> Result<()> {
|
||||
let second_call_id = "uexec-poll";
|
||||
let second_args = serde_json::json!({
|
||||
"chars": "",
|
||||
"session_id": 0,
|
||||
"session_id": 1000,
|
||||
"yield_time_ms": 800,
|
||||
});
|
||||
|
||||
@@ -1589,7 +1588,7 @@ async fn unified_exec_timeout_and_followup_poll() -> Result<()> {
|
||||
let outputs = collect_tool_outputs(&bodies)?;
|
||||
|
||||
let first_output = outputs.get(first_call_id).expect("missing timeout output");
|
||||
assert_eq!(first_output.session_id, Some(0));
|
||||
assert!(first_output.process_id.is_some());
|
||||
assert!(first_output.output.is_empty());
|
||||
|
||||
let poll_output = outputs.get(second_call_id).expect("missing poll output");
|
||||
@@ -1824,7 +1823,7 @@ async fn unified_exec_prunes_exited_sessions_first() -> Result<()> {
|
||||
let keep_write_call_id = "uexec-prune-keep-write";
|
||||
let keep_write_args = serde_json::json!({
|
||||
"chars": "still alive\n",
|
||||
"session_id": 0,
|
||||
"session_id": 1000,
|
||||
"yield_time_ms": 500,
|
||||
});
|
||||
events.push(ev_function_call(
|
||||
@@ -1836,7 +1835,7 @@ async fn unified_exec_prunes_exited_sessions_first() -> Result<()> {
|
||||
let probe_call_id = "uexec-prune-probe";
|
||||
let probe_args = serde_json::json!({
|
||||
"chars": "should fail\n",
|
||||
"session_id": 1,
|
||||
"session_id": 1001,
|
||||
"yield_time_ms": 500,
|
||||
});
|
||||
events.push(ev_function_call(
|
||||
@@ -1885,7 +1884,7 @@ async fn unified_exec_prunes_exited_sessions_first() -> Result<()> {
|
||||
.find_map(|req| req.function_call_output_text(keep_call_id))
|
||||
.expect("missing initial keep session output");
|
||||
let keep_start_output = parse_unified_exec_output(&keep_start)?;
|
||||
pretty_assertions::assert_eq!(keep_start_output.session_id, Some(0));
|
||||
assert!(keep_start_output.process_id.is_some());
|
||||
assert!(keep_start_output.exit_code.is_none());
|
||||
|
||||
let prune_start = requests
|
||||
@@ -1893,7 +1892,7 @@ async fn unified_exec_prunes_exited_sessions_first() -> Result<()> {
|
||||
.find_map(|req| req.function_call_output_text(prune_call_id))
|
||||
.expect("missing initial prune session output");
|
||||
let prune_start_output = parse_unified_exec_output(&prune_start)?;
|
||||
pretty_assertions::assert_eq!(prune_start_output.session_id, Some(1));
|
||||
assert!(prune_start_output.process_id.is_some());
|
||||
assert!(prune_start_output.exit_code.is_none());
|
||||
|
||||
let keep_write = requests
|
||||
@@ -1901,7 +1900,7 @@ async fn unified_exec_prunes_exited_sessions_first() -> Result<()> {
|
||||
.find_map(|req| req.function_call_output_text(keep_write_call_id))
|
||||
.expect("missing keep write output");
|
||||
let keep_write_output = parse_unified_exec_output(&keep_write)?;
|
||||
pretty_assertions::assert_eq!(keep_write_output.session_id, Some(0));
|
||||
assert!(keep_write_output.process_id.is_some());
|
||||
assert!(
|
||||
keep_write_output.output.contains("still alive"),
|
||||
"expected cat session to echo input, got {:?}",
|
||||
@@ -1913,7 +1912,7 @@ async fn unified_exec_prunes_exited_sessions_first() -> Result<()> {
|
||||
.find_map(|req| req.function_call_output_text(probe_call_id))
|
||||
.expect("missing probe output");
|
||||
assert!(
|
||||
pruned_probe.contains("UnknownSessionId") || pruned_probe.contains("Unknown session id"),
|
||||
pruned_probe.contains("UnknownSessionId") || pruned_probe.contains("Unknown process id"),
|
||||
"expected probe to fail after pruning, got {pruned_probe:?}"
|
||||
);
|
||||
|
||||
|
||||
@@ -406,6 +406,7 @@ impl EventProcessor for EventProcessorWithHumanOutput {
|
||||
FileChange::Update {
|
||||
unified_diff,
|
||||
move_path,
|
||||
..
|
||||
} => {
|
||||
let header = if let Some(dest) = move_path {
|
||||
format!(
|
||||
|
||||
@@ -637,6 +637,7 @@ fn exec_command_end_success_produces_completed_command_item() {
|
||||
"c1",
|
||||
EventMsg::ExecCommandBegin(ExecCommandBeginEvent {
|
||||
call_id: "1".to_string(),
|
||||
process_id: None,
|
||||
turn_id: "turn-1".to_string(),
|
||||
command: command.clone(),
|
||||
cwd: cwd.clone(),
|
||||
@@ -666,6 +667,7 @@ fn exec_command_end_success_produces_completed_command_item() {
|
||||
"c2",
|
||||
EventMsg::ExecCommandEnd(ExecCommandEndEvent {
|
||||
call_id: "1".to_string(),
|
||||
process_id: None,
|
||||
turn_id: "turn-1".to_string(),
|
||||
command,
|
||||
cwd,
|
||||
@@ -709,6 +711,7 @@ fn exec_command_end_failure_produces_failed_command_item() {
|
||||
"c1",
|
||||
EventMsg::ExecCommandBegin(ExecCommandBeginEvent {
|
||||
call_id: "2".to_string(),
|
||||
process_id: None,
|
||||
turn_id: "turn-1".to_string(),
|
||||
command: command.clone(),
|
||||
cwd: cwd.clone(),
|
||||
@@ -737,6 +740,7 @@ fn exec_command_end_failure_produces_failed_command_item() {
|
||||
"c2",
|
||||
EventMsg::ExecCommandEnd(ExecCommandEndEvent {
|
||||
call_id: "2".to_string(),
|
||||
process_id: None,
|
||||
turn_id: "turn-1".to_string(),
|
||||
command,
|
||||
cwd,
|
||||
@@ -777,6 +781,7 @@ fn exec_command_end_without_begin_is_ignored() {
|
||||
"c1",
|
||||
EventMsg::ExecCommandEnd(ExecCommandEndEvent {
|
||||
call_id: "no-begin".to_string(),
|
||||
process_id: None,
|
||||
turn_id: "turn-1".to_string(),
|
||||
command: Vec::new(),
|
||||
cwd: PathBuf::from("."),
|
||||
@@ -818,6 +823,8 @@ fn patch_apply_success_produces_item_completed_patchapply() {
|
||||
FileChange::Update {
|
||||
unified_diff: "--- c/modified.txt\n+++ c/modified.txt\n@@\n-old\n+new\n".to_string(),
|
||||
move_path: Some(PathBuf::from("c/renamed.txt")),
|
||||
old_content: "-old\n".to_string(),
|
||||
new_content: "+new\n".to_string(),
|
||||
},
|
||||
);
|
||||
|
||||
@@ -890,6 +897,8 @@ fn patch_apply_failure_produces_item_completed_patchapply_failed() {
|
||||
FileChange::Update {
|
||||
unified_diff: "--- file.txt\n+++ file.txt\n@@\n-old\n+new\n".to_string(),
|
||||
move_path: None,
|
||||
old_content: "-old\n".to_string(),
|
||||
new_content: "+new\n".to_string(),
|
||||
},
|
||||
);
|
||||
|
||||
|
||||
@@ -14,7 +14,7 @@ codex-core = { path = "../core" }
|
||||
reqwest = { version = "0.12", features = ["json", "stream"] }
|
||||
serde_json = "1"
|
||||
tokio = { version = "1", features = ["rt"] }
|
||||
tracing = { version = "0.1.41", features = ["log"] }
|
||||
tracing = { version = "0.1.43", features = ["log"] }
|
||||
which = "6.0"
|
||||
|
||||
[dev-dependencies]
|
||||
|
||||
@@ -269,6 +269,8 @@ async fn patch_approval_triggers_elicitation() -> anyhow::Result<()> {
|
||||
FileChange::Update {
|
||||
unified_diff: "@@ -1 +1 @@\n-original content\n+modified content\n".to_string(),
|
||||
move_path: None,
|
||||
old_content: "original content\n".to_string(),
|
||||
new_content: "modified content\n".to_string(),
|
||||
},
|
||||
);
|
||||
|
||||
|
||||
@@ -1256,13 +1256,18 @@ pub struct GitInfo {
|
||||
pub repository_url: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "snake_case")]
|
||||
pub enum ReviewDelivery {
|
||||
Inline,
|
||||
Detached,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, JsonSchema, TS)]
|
||||
/// Review request sent to the review session.
|
||||
pub struct ReviewRequest {
|
||||
pub prompt: String,
|
||||
pub user_facing_hint: String,
|
||||
#[serde(default)]
|
||||
pub append_to_original_thread: bool,
|
||||
}
|
||||
|
||||
/// Structured review result produced by a child review session.
|
||||
@@ -1328,6 +1333,10 @@ impl Default for ExecCommandSource {
|
||||
pub struct ExecCommandBeginEvent {
|
||||
/// Identifier so this can be paired with the ExecCommandEnd event.
|
||||
pub call_id: String,
|
||||
/// Identifier for the underlying PTY process (when available).
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional)]
|
||||
pub process_id: Option<String>,
|
||||
/// Turn ID that this command belongs to.
|
||||
pub turn_id: String,
|
||||
/// The command to be executed.
|
||||
@@ -1348,6 +1357,10 @@ pub struct ExecCommandBeginEvent {
|
||||
pub struct ExecCommandEndEvent {
|
||||
/// Identifier for the ExecCommandBegin that finished.
|
||||
pub call_id: String,
|
||||
/// Identifier for the underlying PTY process (when available).
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional)]
|
||||
pub process_id: Option<String>,
|
||||
/// Turn ID that this command belongs to.
|
||||
pub turn_id: String,
|
||||
/// The command that was executed.
|
||||
@@ -1640,6 +1653,8 @@ pub enum FileChange {
|
||||
Update {
|
||||
unified_diff: String,
|
||||
move_path: Option<PathBuf>,
|
||||
old_content: String,
|
||||
new_content: String,
|
||||
},
|
||||
}
|
||||
|
||||
@@ -1704,6 +1719,21 @@ mod tests {
|
||||
assert!(event.as_legacy_events(false).is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn file_change_update_serializes_old_and_new_content() {
|
||||
let change = FileChange::Update {
|
||||
unified_diff: "--- a\n+++ b\n@@ -1 +1 @@\n-old\n+new\n".to_string(),
|
||||
move_path: None,
|
||||
old_content: "old\n".to_string(),
|
||||
new_content: "new\n".to_string(),
|
||||
};
|
||||
|
||||
let serialized = serde_json::to_value(change).expect("should serialize");
|
||||
|
||||
assert_eq!(serialized["old_content"], "old\n");
|
||||
assert_eq!(serialized["new_content"], "new\n");
|
||||
}
|
||||
|
||||
/// Serialize Event to verify that its JSON representation has the expected
|
||||
/// amount of nesting.
|
||||
#[test]
|
||||
|
||||
@@ -1560,6 +1560,8 @@ impl ChatWidget {
|
||||
FileChange::Update {
|
||||
unified_diff: "+test\n-test2".to_string(),
|
||||
move_path: None,
|
||||
old_content: "test2".to_string(),
|
||||
new_content: "test".to_string(),
|
||||
},
|
||||
),
|
||||
]),
|
||||
@@ -2895,7 +2897,6 @@ impl ChatWidget {
|
||||
review_request: ReviewRequest {
|
||||
prompt: "Review the current code changes (staged, unstaged, and untracked files) and provide prioritized findings.".to_string(),
|
||||
user_facing_hint: "current changes".to_string(),
|
||||
append_to_original_thread: true,
|
||||
},
|
||||
}));
|
||||
},
|
||||
@@ -2952,7 +2953,6 @@ impl ChatWidget {
|
||||
"Review the code changes against the base branch '{branch}'. Start by finding the merge diff between the current branch and {branch}'s upstream e.g. (`git merge-base HEAD \"$(git rev-parse --abbrev-ref \"{branch}@{{upstream}}\")\"`), then run `git diff` against that SHA to see what changes we would merge into the {branch} branch. Provide prioritized, actionable findings."
|
||||
),
|
||||
user_facing_hint: format!("changes against '{branch}'"),
|
||||
append_to_original_thread: true,
|
||||
},
|
||||
}));
|
||||
})],
|
||||
@@ -2993,7 +2993,6 @@ impl ChatWidget {
|
||||
review_request: ReviewRequest {
|
||||
prompt,
|
||||
user_facing_hint: hint,
|
||||
append_to_original_thread: true,
|
||||
},
|
||||
}));
|
||||
})],
|
||||
@@ -3028,7 +3027,6 @@ impl ChatWidget {
|
||||
review_request: ReviewRequest {
|
||||
prompt: trimmed.clone(),
|
||||
user_facing_hint: trimmed,
|
||||
append_to_original_thread: true,
|
||||
},
|
||||
}));
|
||||
}),
|
||||
@@ -3244,7 +3242,6 @@ pub(crate) fn show_review_commit_picker_with_entries(
|
||||
review_request: ReviewRequest {
|
||||
prompt,
|
||||
user_facing_hint: hint,
|
||||
append_to_original_thread: true,
|
||||
},
|
||||
}));
|
||||
})],
|
||||
|
||||
@@ -155,7 +155,6 @@ fn entered_review_mode_uses_request_hint() {
|
||||
msg: EventMsg::EnteredReviewMode(ReviewRequest {
|
||||
prompt: "Review the latest changes".to_string(),
|
||||
user_facing_hint: "feature branch".to_string(),
|
||||
append_to_original_thread: true,
|
||||
}),
|
||||
});
|
||||
|
||||
@@ -175,7 +174,6 @@ fn entered_review_mode_defaults_to_current_changes_banner() {
|
||||
msg: EventMsg::EnteredReviewMode(ReviewRequest {
|
||||
prompt: "Review the current changes".to_string(),
|
||||
user_facing_hint: "current changes".to_string(),
|
||||
append_to_original_thread: true,
|
||||
}),
|
||||
});
|
||||
|
||||
@@ -243,7 +241,6 @@ fn review_restores_context_window_indicator() {
|
||||
msg: EventMsg::EnteredReviewMode(ReviewRequest {
|
||||
prompt: "Review the latest changes".to_string(),
|
||||
user_facing_hint: "feature branch".to_string(),
|
||||
append_to_original_thread: true,
|
||||
}),
|
||||
});
|
||||
|
||||
@@ -722,6 +719,7 @@ fn begin_exec_with_source(
|
||||
let interaction_input = None;
|
||||
let event = ExecCommandBeginEvent {
|
||||
call_id: call_id.to_string(),
|
||||
process_id: None,
|
||||
turn_id: "turn-1".to_string(),
|
||||
command,
|
||||
cwd,
|
||||
@@ -760,11 +758,13 @@ fn end_exec(
|
||||
parsed_cmd,
|
||||
source,
|
||||
interaction_input,
|
||||
process_id,
|
||||
} = begin_event;
|
||||
chat.handle_codex_event(Event {
|
||||
id: call_id.clone(),
|
||||
msg: EventMsg::ExecCommandEnd(ExecCommandEndEvent {
|
||||
call_id,
|
||||
process_id,
|
||||
turn_id,
|
||||
command,
|
||||
cwd,
|
||||
@@ -2880,6 +2880,7 @@ fn chatwidget_exec_and_status_layout_vt100_snapshot() {
|
||||
id: "c1".into(),
|
||||
msg: EventMsg::ExecCommandBegin(ExecCommandBeginEvent {
|
||||
call_id: "c1".into(),
|
||||
process_id: None,
|
||||
turn_id: "turn-1".into(),
|
||||
command: command.clone(),
|
||||
cwd: cwd.clone(),
|
||||
@@ -2892,6 +2893,7 @@ fn chatwidget_exec_and_status_layout_vt100_snapshot() {
|
||||
id: "c1".into(),
|
||||
msg: EventMsg::ExecCommandEnd(ExecCommandEndEvent {
|
||||
call_id: "c1".into(),
|
||||
process_id: None,
|
||||
turn_id: "turn-1".into(),
|
||||
command,
|
||||
cwd,
|
||||
|
||||
@@ -484,6 +484,8 @@ mod tests {
|
||||
FileChange::Update {
|
||||
unified_diff: patch,
|
||||
move_path: None,
|
||||
old_content: original.to_string(),
|
||||
new_content: modified.to_string(),
|
||||
},
|
||||
);
|
||||
|
||||
@@ -504,6 +506,8 @@ mod tests {
|
||||
FileChange::Update {
|
||||
unified_diff: patch,
|
||||
move_path: Some(PathBuf::from("new_name.rs")),
|
||||
old_content: original.to_string(),
|
||||
new_content: modified.to_string(),
|
||||
},
|
||||
);
|
||||
|
||||
@@ -524,6 +528,8 @@ mod tests {
|
||||
FileChange::Update {
|
||||
unified_diff: patch_a,
|
||||
move_path: None,
|
||||
old_content: "one\n".to_string(),
|
||||
new_content: "one changed\n".to_string(),
|
||||
},
|
||||
);
|
||||
|
||||
@@ -590,6 +596,8 @@ mod tests {
|
||||
FileChange::Update {
|
||||
unified_diff: patch,
|
||||
move_path: None,
|
||||
old_content: original.to_string(),
|
||||
new_content: modified.to_string(),
|
||||
},
|
||||
);
|
||||
|
||||
@@ -613,6 +621,8 @@ mod tests {
|
||||
FileChange::Update {
|
||||
unified_diff: patch,
|
||||
move_path: None,
|
||||
old_content: original.to_string(),
|
||||
new_content: modified.to_string(),
|
||||
},
|
||||
);
|
||||
|
||||
@@ -640,6 +650,8 @@ mod tests {
|
||||
FileChange::Update {
|
||||
unified_diff: patch,
|
||||
move_path: None,
|
||||
old_content: original,
|
||||
new_content: modified,
|
||||
},
|
||||
);
|
||||
|
||||
@@ -663,6 +675,8 @@ mod tests {
|
||||
FileChange::Update {
|
||||
unified_diff: patch,
|
||||
move_path: Some(abs_new),
|
||||
old_content: original.to_string(),
|
||||
new_content: modified.to_string(),
|
||||
},
|
||||
);
|
||||
|
||||
|
||||
@@ -2,11 +2,11 @@
|
||||
|
||||
If you already lean on Codex every day and just need a little more control, this page collects the knobs you are most likely to reach for: tweak defaults in [Config](./config.md), add extra tools through [Model Context Protocol support](#model-context-protocol), and script full runs with [`codex exec`](./exec.md). Jump to the section you need and keep building.
|
||||
|
||||
## Config quickstart {#config-quickstart}
|
||||
## Config quickstart
|
||||
|
||||
Most day-to-day tuning lives in `config.toml`: set approval + sandbox presets, pin model defaults, and add MCP server launchers. The [Config guide](./config.md) walks through every option and provides copy-paste examples for common setups.
|
||||
|
||||
## Tracing / verbose logging {#tracing-verbose-logging}
|
||||
## Tracing / verbose logging
|
||||
|
||||
Because Codex is written in Rust, it honors the `RUST_LOG` environment variable to configure its logging behavior.
|
||||
|
||||
@@ -20,15 +20,15 @@ By comparison, the non-interactive mode (`codex exec`) defaults to `RUST_LOG=err
|
||||
|
||||
See the Rust documentation on [`RUST_LOG`](https://docs.rs/env_logger/latest/env_logger/#enabling-logging) for more information on the configuration options.
|
||||
|
||||
## Model Context Protocol (MCP) {#model-context-protocol}
|
||||
## Model Context Protocol (MCP)
|
||||
|
||||
The Codex CLI and IDE extension is a MCP client which means that it can be configured to connect to MCP servers. For more information, refer to the [`config docs`](./config.md#mcp-integration).
|
||||
|
||||
## Using Codex as an MCP Server {#mcp-server}
|
||||
## Using Codex as an MCP Server
|
||||
|
||||
The Codex CLI can also be run as an MCP _server_ via `codex mcp-server`. For example, you can use `codex mcp-server` to make Codex available as a tool inside of a multi-agent framework like the OpenAI [Agents SDK](https://platform.openai.com/docs/guides/agents). Use `codex mcp` separately to add/list/get/remove MCP server launchers in your configuration.
|
||||
|
||||
### Codex MCP Server Quickstart {#mcp-server-quickstart}
|
||||
### Codex MCP Server Quickstart
|
||||
|
||||
You can launch a Codex MCP server with the [Model Context Protocol Inspector](https://modelcontextprotocol.io/legacy/tools/inspector):
|
||||
|
||||
@@ -58,7 +58,7 @@ Send a `tools/list` request and you will see that there are two tools available:
|
||||
| **`prompt`** (required) | string | The next user prompt to continue the Codex conversation. |
|
||||
| **`conversationId`** (required) | string | The id of the conversation to continue. |
|
||||
|
||||
### Trying it Out {#mcp-server-trying-it-out}
|
||||
### Trying it Out
|
||||
|
||||
> [!TIP]
|
||||
> Codex often takes a few minutes to run. To accommodate this, adjust the MCP inspector's Request and Total timeouts to 600000ms (10 minutes) under ⛭ Configuration.
|
||||
|
||||
@@ -180,6 +180,9 @@ chatgpt_base_url = "https://chatgpt.com/backend-api/"
|
||||
# Allowed values: chatgpt | api
|
||||
# forced_login_method = "chatgpt"
|
||||
|
||||
# Preferred store for MCP OAuth credentials: auto (default) | file | keyring
|
||||
mcp_oauth_credentials_store = "auto"
|
||||
|
||||
################################################################################
|
||||
# Project Documentation Controls
|
||||
################################################################################
|
||||
@@ -232,16 +235,6 @@ experimental_use_rmcp_client = false
|
||||
# Include apply_patch via freeform editing path (affects default tool set). Default: false
|
||||
experimental_use_freeform_apply_patch = false
|
||||
|
||||
# Enable model-based sandbox command assessment. Default: false
|
||||
experimental_sandbox_command_assessment = false
|
||||
|
||||
################################################################################
|
||||
# MCP (Model Context Protocol) servers
|
||||
################################################################################
|
||||
|
||||
# Preferred store for MCP OAuth credentials: auto (default) | file | keyring
|
||||
mcp_oauth_credentials_store = "auto"
|
||||
|
||||
# Define MCP servers under this table. Leave empty to disable.
|
||||
[mcp_servers]
|
||||
|
||||
|
||||
@@ -113,3 +113,7 @@ Paste images directly into the composer (Ctrl+V / Cmd+V) to attach them to your
|
||||
codex -i screenshot.png "Explain this error"
|
||||
codex --image img1.png,img2.jpg "Summarize these diagrams"
|
||||
```
|
||||
|
||||
#### Environment variables and executables
|
||||
|
||||
Make sure your environment is already set up before launching Codex so it does not spend tokens probing what to activate. For example, source your Python virtualenv (or other language runtimes), start any required daemons, and export the env vars you expect to use ahead of time.
|
||||
|
||||
@@ -1,3 +1,3 @@
|
||||
## Platform sandboxing
|
||||
|
||||
This content now lives alongside the rest of the sandbox guidance. See [Sandbox mechanics by platform](./sandbox.md#platform-sandboxing-details) for up-to-date details.
|
||||
This content now lives alongside the rest of the sandbox guidance. See [Sandbox mechanics by platform](./sandbox.md#sandbox-mechanics-by-platform) for up-to-date details.
|
||||
|
||||
Reference in New Issue
Block a user