Merge c52b5b5ce2 into sapling-pr-archive-bolinfest

This commit is contained in:
Michael Bolin
2026-04-22 19:21:07 -07:00
committed by GitHub
90 changed files with 2861 additions and 1450 deletions
+3
View File
@@ -76,6 +76,8 @@ jobs:
- name: Test argument comment lint package
working-directory: tools/argument-comment-lint
run: cargo test
env:
RUST_MIN_STACK: "8388608" # 8 MiB
argument_comment_lint_prebuilt:
name: Argument comment lint - ${{ matrix.name }}
@@ -671,6 +673,7 @@ jobs:
run: cargo nextest run --no-fail-fast --target ${{ matrix.target }} --cargo-profile ci-test --timings
env:
RUST_BACKTRACE: 1
RUST_MIN_STACK: "8388608" # 8 MiB
NEXTEST_STATUS_LEVEL: leak
- name: Upload Cargo timings (nextest)
+2
View File
@@ -131,6 +131,8 @@ jobs:
- name: Test argument comment lint package
working-directory: tools/argument-comment-lint
run: cargo test
env:
RUST_MIN_STACK: "8388608" # 8 MiB
argument_comment_lint_prebuilt:
name: Argument comment lint - ${{ matrix.name }}
+3
View File
@@ -2579,7 +2579,9 @@ dependencies = [
"arc-swap",
"async-trait",
"base64 0.22.1",
"bytes",
"codex-app-server-protocol",
"codex-client",
"codex-config",
"codex-protocol",
"codex-sandboxing",
@@ -3141,6 +3143,7 @@ version = "0.0.0"
dependencies = [
"anyhow",
"axum",
"bytes",
"codex-client",
"codex-config",
"codex-exec-server",
@@ -324,7 +324,7 @@ fn sample_turn_resolved_config(turn_id: &str) -> TurnResolvedConfigFact {
reasoning_summary: None,
service_tier: None,
approval_policy: AskForApproval::OnRequest,
approvals_reviewer: ApprovalsReviewer::GuardianSubagent,
approvals_reviewer: ApprovalsReviewer::AutoReview,
sandbox_network_access: true,
collaboration_mode: ModeKind::Plan,
personality: None,
@@ -311,6 +311,13 @@
}
],
"default": "turn"
},
"strictAutoReview": {
"description": "Review every subsequent command in this turn before normal sandboxed execution.",
"type": [
"boolean",
"null"
]
}
},
"required": [
@@ -3501,6 +3501,13 @@
}
],
"default": "turn"
},
"strictAutoReview": {
"description": "Review every subsequent command in this turn before normal sandboxed execution.",
"type": [
"boolean",
"null"
]
}
},
"required": [
@@ -4,4 +4,8 @@
import type { GrantedPermissionProfile } from "./GrantedPermissionProfile";
import type { PermissionGrantScope } from "./PermissionGrantScope";
export type PermissionsRequestApprovalResponse = { permissions: GrantedPermissionProfile, scope: PermissionGrantScope, };
export type PermissionsRequestApprovalResponse = { permissions: GrantedPermissionProfile, scope: PermissionGrantScope,
/**
* Review every subsequent command in this turn before normal sandboxed execution.
*/
strictAutoReview?: boolean, };
@@ -323,7 +323,7 @@ pub enum ApprovalsReviewer {
#[serde(rename = "user")]
User,
#[serde(rename = "auto_review", alias = "guardian_subagent")]
GuardianSubagent,
AutoReview,
}
impl JsonSchema for ApprovalsReviewer {
@@ -361,7 +361,7 @@ impl ApprovalsReviewer {
pub fn to_core(self) -> CoreApprovalsReviewer {
match self {
ApprovalsReviewer::User => CoreApprovalsReviewer::User,
ApprovalsReviewer::GuardianSubagent => CoreApprovalsReviewer::GuardianSubagent,
ApprovalsReviewer::AutoReview => CoreApprovalsReviewer::AutoReview,
}
}
}
@@ -370,7 +370,7 @@ impl From<CoreApprovalsReviewer> for ApprovalsReviewer {
fn from(value: CoreApprovalsReviewer) -> Self {
match value {
CoreApprovalsReviewer::User => ApprovalsReviewer::User,
CoreApprovalsReviewer::GuardianSubagent => ApprovalsReviewer::GuardianSubagent,
CoreApprovalsReviewer::AutoReview => ApprovalsReviewer::AutoReview,
}
}
}
@@ -7059,6 +7059,10 @@ pub struct PermissionsRequestApprovalResponse {
pub permissions: GrantedPermissionProfile,
#[serde(default)]
pub scope: PermissionGrantScope,
/// Review every subsequent command in this turn before normal sandboxed execution.
#[serde(default, skip_serializing_if = "Option::is_none")]
#[ts(optional)]
pub strict_auto_review: Option<bool>,
}
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
@@ -7400,8 +7404,7 @@ mod tests {
"\"user\""
);
assert_eq!(
serde_json::to_string(&ApprovalsReviewer::GuardianSubagent)
.expect("serialize reviewer"),
serde_json::to_string(&ApprovalsReviewer::AutoReview).expect("serialize reviewer"),
"\"auto_review\""
);
@@ -7412,7 +7415,7 @@ mod tests {
let expected = if value == "user" {
ApprovalsReviewer::User
} else {
ApprovalsReviewer::GuardianSubagent
ApprovalsReviewer::AutoReview
};
assert_eq!(expected, reviewer);
}
@@ -7809,6 +7812,18 @@ mod tests {
.expect("response should deserialize");
assert_eq!(response.scope, PermissionGrantScope::Turn);
assert_eq!(response.strict_auto_review, None);
}
#[test]
fn permissions_request_approval_response_accepts_strict_auto_review() {
let response = serde_json::from_value::<PermissionsRequestApprovalResponse>(json!({
"permissions": {},
"strictAutoReview": true,
}))
.expect("response should deserialize");
assert_eq!(response.strict_auto_review, Some(true));
}
#[test]
@@ -8695,7 +8710,7 @@ mod tests {
model_auto_compact_token_limit: None,
model_provider: None,
approval_policy: None,
approvals_reviewer: Some(ApprovalsReviewer::GuardianSubagent),
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
sandbox_mode: None,
sandbox_workspace_write: None,
forced_chatgpt_workspace_id: None,
@@ -8797,7 +8812,7 @@ mod tests {
model: None,
model_provider: None,
approval_policy: None,
approvals_reviewer: Some(ApprovalsReviewer::GuardianSubagent),
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
service_tier: None,
model_reasoning_effort: None,
model_reasoning_summary: None,
@@ -961,6 +961,7 @@ pub(crate) async fn apply_bespoke_event_handling(
let empty = CoreRequestPermissionsResponse {
permissions: Default::default(),
scope: CorePermissionGrantScope::Turn,
strict_auto_review: false,
};
if let Err(err) = conversation
.submit(Op::RequestPermissionsResponse {
@@ -2669,6 +2670,7 @@ fn request_permissions_response_from_client_result(
return Some(CoreRequestPermissionsResponse {
permissions: Default::default(),
scope: CorePermissionGrantScope::Turn,
strict_auto_review: false,
});
}
Err(err) => {
@@ -2676,6 +2678,7 @@ fn request_permissions_response_from_client_result(
return Some(CoreRequestPermissionsResponse {
permissions: Default::default(),
scope: CorePermissionGrantScope::Turn,
strict_auto_review: false,
});
}
};
@@ -2686,8 +2689,23 @@ fn request_permissions_response_from_client_result(
PermissionsRequestApprovalResponse {
permissions: V2GrantedPermissionProfile::default(),
scope: codex_app_server_protocol::PermissionGrantScope::Turn,
strict_auto_review: None,
}
});
let strict_auto_review = response.strict_auto_review.unwrap_or(false);
if strict_auto_review
&& matches!(
response.scope,
codex_app_server_protocol::PermissionGrantScope::Session
)
{
error!("strict auto review is only supported for turn-scoped permission grants");
return Some(CoreRequestPermissionsResponse {
permissions: Default::default(),
scope: CorePermissionGrantScope::Turn,
strict_auto_review: false,
});
}
let granted_permissions: CorePermissionProfile = response.permissions.into();
let permissions = if granted_permissions.is_empty() {
CoreRequestPermissionProfile::default()
@@ -2697,6 +2715,7 @@ fn request_permissions_response_from_client_result(
Some(CoreRequestPermissionsResponse {
permissions,
scope: response.scope.to_core(),
strict_auto_review,
})
}
@@ -3858,6 +3877,7 @@ mod tests {
CoreRequestPermissionsResponse {
permissions: expected_permissions,
scope: CorePermissionGrantScope::Turn,
strict_auto_review: false,
}
);
}
@@ -3880,10 +3900,63 @@ mod tests {
CoreRequestPermissionsResponse {
permissions: CoreRequestPermissionProfile::default(),
scope: CorePermissionGrantScope::Session,
strict_auto_review: false,
}
);
}
#[test]
fn request_permissions_response_rejects_session_scoped_strict_auto_review() {
let response = request_permissions_response_from_client_result(
CoreRequestPermissionProfile::default(),
Ok(Ok(serde_json::json!({
"scope": "session",
"strictAutoReview": true,
"permissions": {
"network": {
"enabled": true,
},
},
}))),
std::env::current_dir().expect("current dir").as_path(),
)
.expect("response should be accepted");
assert_eq!(
response,
CoreRequestPermissionsResponse {
permissions: CoreRequestPermissionProfile::default(),
scope: CorePermissionGrantScope::Turn,
strict_auto_review: false,
}
);
}
#[test]
fn request_permissions_response_preserves_turn_scoped_strict_auto_review() {
let response = request_permissions_response_from_client_result(
CoreRequestPermissionProfile {
network: Some(codex_protocol::models::NetworkPermissions {
enabled: Some(true),
}),
..Default::default()
},
Ok(Ok(serde_json::json!({
"strictAutoReview": true,
"permissions": {
"network": {
"enabled": true,
},
},
}))),
std::env::current_dir().expect("current dir").as_path(),
)
.expect("response should be accepted");
assert_eq!(response.scope, CorePermissionGrantScope::Turn);
assert!(response.strict_auto_review);
}
#[test]
fn request_permissions_response_accepts_explicit_child_grant_for_requested_cwd_scope() {
let temp_dir = TempDir::new().expect("temp dir");
@@ -5881,8 +5881,8 @@ impl CodexMessageProcessor {
let environment_manager = self.thread_manager.environment_manager();
let runtime_environment = match environment_manager.default_environment() {
Some(environment) => {
// Status listing has no turn cwd. This fallback is used only
// by executor-backed stdio MCPs whose config omits `cwd`.
// Status listing has no turn cwd. This fallback is used by
// stdio MCPs whose config omits `cwd`.
McpRuntimeEnvironment::new(environment, config.cwd.to_path_buf())
}
None => McpRuntimeEnvironment::new(
+2 -2
View File
@@ -459,7 +459,7 @@ mod tests {
]),
allowed_approvals_reviewers: Some(vec![
CoreApprovalsReviewer::User,
CoreApprovalsReviewer::GuardianSubagent,
CoreApprovalsReviewer::AutoReview,
]),
allowed_sandbox_modes: Some(vec![
CoreSandboxModeRequirement::ReadOnly,
@@ -524,7 +524,7 @@ mod tests {
mapped.allowed_approvals_reviewers,
Some(vec![
codex_app_server_protocol::ApprovalsReviewer::User,
codex_app_server_protocol::ApprovalsReviewer::GuardianSubagent,
codex_app_server_protocol::ApprovalsReviewer::AutoReview,
])
);
assert_eq!(
@@ -99,6 +99,7 @@ async fn request_permissions_round_trip() -> Result<()> {
}),
},
scope: PermissionGrantScope::Turn,
strict_auto_review: None,
})?,
)
.await?;
@@ -666,8 +666,8 @@ pub struct McpConnectionManager {
/// servers should run for the current caller. Keep it explicit at manager
/// construction time so status/snapshot paths and real sessions make the same
/// local-vs-remote decision. `fallback_cwd` is not a per-server override; it is
/// used only when an executor-backed stdio server omits `cwd` and the executor
/// API still needs a concrete process working directory.
/// used when a stdio server omits `cwd` and the launcher needs a concrete
/// process working directory.
#[derive(Clone)]
pub struct McpRuntimeEnvironment {
environment: Arc<Environment>,
@@ -1573,7 +1573,9 @@ async fn make_rmcp_client(
runtime_environment.fallback_cwd(),
))
} else {
Arc::new(LocalStdioServerLauncher) as Arc<dyn StdioServerLauncher>
Arc::new(LocalStdioServerLauncher::new(
runtime_environment.fallback_cwd(),
)) as Arc<dyn StdioServerLauncher>
};
// `RmcpClient` always sees a launched MCP stdio server. The
@@ -1589,23 +1591,11 @@ async fn make_rmcp_client(
env_http_headers,
bearer_token_env_var,
} => {
if remote_environment {
if !runtime_environment.environment().is_remote() {
return Err(StartupOutcomeError::from(anyhow!(
"remote MCP server `{server_name}` requires a remote executor environment"
)));
}
if remote_environment && !runtime_environment.environment().is_remote() {
return Err(StartupOutcomeError::from(anyhow!(
// Remote HTTP needs the future low-level executor
// `network/request` API so reqwest runs on the executor side.
// Do not fall back to local HTTP here; the config explicitly
// asked for remote placement.
"remote streamable HTTP MCP server `{server_name}` is not implemented yet"
"remote MCP server `{server_name}` requires a remote environment"
)));
}
// Local streamable HTTP remains the existing reqwest path from
// the orchestrator process.
let resolved_bearer_token =
match resolve_bearer_token(server_name, bearer_token_env_var.as_deref()) {
Ok(token) => token,
@@ -1618,6 +1608,7 @@ async fn make_rmcp_client(
http_headers,
env_http_headers,
store_mode,
runtime_environment.environment().get_http_client(),
)
.await
.map_err(StartupOutcomeError::from)
+5 -5
View File
@@ -1185,7 +1185,7 @@ mod tests {
let allowed_approval_policies = vec![AskForApproval::UnlessTrusted, AskForApproval::Never];
let allowed_approvals_reviewers =
vec![ApprovalsReviewer::GuardianSubagent, ApprovalsReviewer::User];
vec![ApprovalsReviewer::AutoReview, ApprovalsReviewer::User];
let allowed_sandbox_modes = vec![
SandboxModeRequirement::WorkspaceWrite,
SandboxModeRequirement::DangerFullAccess,
@@ -1690,7 +1690,7 @@ allowed_approvals_reviewers = ["user"]
Err(ConstraintError::InvalidValue {
field_name: "approvals_reviewer",
candidate: "User".into(),
allowed: "[GuardianSubagent]".into(),
allowed: "[AutoReview]".into(),
requirement_source: source_location,
})
);
@@ -1837,13 +1837,13 @@ allowed_approvals_reviewers = ["user"]
assert_eq!(
requirements.approvals_reviewer.value(),
ApprovalsReviewer::GuardianSubagent,
ApprovalsReviewer::AutoReview,
"currently, there is no way to specify the default value for approvals reviewer in the toml, so it picks the first allowed value"
);
assert!(
requirements
.approvals_reviewer
.can_set(&ApprovalsReviewer::GuardianSubagent)
.can_set(&ApprovalsReviewer::AutoReview)
.is_ok()
);
assert!(
@@ -1866,7 +1866,7 @@ allowed_approvals_reviewers = ["user"]
assert_eq!(
requirements.approvals_reviewer.value(),
ApprovalsReviewer::GuardianSubagent
ApprovalsReviewer::AutoReview
);
Ok(())
+16 -14
View File
@@ -163,15 +163,14 @@ impl AgentControl {
initial_operation: Op,
session_source: Option<SessionSource>,
) -> CodexResult<ThreadId> {
Ok(self
.spawn_agent_internal(
config,
initial_operation,
session_source,
SpawnAgentOptions::default(),
)
.await?
.thread_id)
let spawned_agent = Box::pin(self.spawn_agent_internal(
config,
initial_operation,
session_source,
SpawnAgentOptions::default(),
))
.await?;
Ok(spawned_agent.thread_id)
}
/// Spawn an agent thread with some metadata.
@@ -182,7 +181,7 @@ impl AgentControl {
session_source: Option<SessionSource>,
options: SpawnAgentOptions, // TODO(jif) drop with new fork.
) -> CodexResult<LiveAgent> {
self.spawn_agent_internal(config, initial_operation, session_source, options)
Box::pin(self.spawn_agent_internal(config, initial_operation, session_source, options))
.await
}
@@ -418,9 +417,12 @@ impl AgentControl {
session_source: SessionSource,
) -> CodexResult<ThreadId> {
let root_depth = thread_spawn_depth(&session_source).unwrap_or(0);
let resumed_thread_id = self
.resume_single_agent_from_rollout(config.clone(), thread_id, session_source)
.await?;
let resumed_thread_id = Box::pin(self.resume_single_agent_from_rollout(
config.clone(),
thread_id,
session_source,
))
.await?;
let state = self.upgrade()?;
let Ok(resumed_thread) = state.get_thread(resumed_thread_id).await else {
return Ok(resumed_thread_id);
@@ -698,7 +700,7 @@ impl AgentControl {
{
warn!("failed to persist thread-spawn edge status for {agent_id}: {err}");
}
self.shutdown_agent_tree(agent_id).await
Box::pin(self.shutdown_agent_tree(agent_id)).await
}
/// Shut down `agent_id` and any live descendants reachable from the in-memory spawn tree.
+2
View File
@@ -804,6 +804,7 @@ where
let empty = RequestPermissionsResponse {
permissions: Default::default(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
};
parent_session
.notify_request_permissions_response(call_id, empty.clone())
@@ -813,6 +814,7 @@ where
response = fut => response.unwrap_or_else(|| RequestPermissionsResponse {
permissions: Default::default(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
}),
}
}
+3 -2
View File
@@ -179,6 +179,7 @@ async fn handle_request_permissions_uses_tool_call_id_for_round_trip() {
..RequestPermissionProfile::default()
},
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
};
let delegated_cwd = parent_ctx.cwd.join("delegated-cwd");
let cancel_token = CancellationToken::new();
@@ -251,7 +252,7 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f
crate::session::tests::make_session_and_context_with_rx().await;
let mut parent_ctx = Arc::try_unwrap(parent_ctx).expect("single turn context ref");
let mut config = (*parent_ctx.config).clone();
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
parent_ctx.config = Arc::new(config);
parent_ctx
.approval_policy
@@ -363,7 +364,7 @@ async fn delegated_mcp_guardian_abort_returns_synthetic_decline_answer() {
crate::session::tests::make_session_and_context_with_rx().await;
let mut parent_ctx = Arc::try_unwrap(parent_ctx).expect("single turn context ref");
let mut config = (*parent_ctx.config).clone();
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
parent_ctx.config = Arc::new(config);
parent_ctx
.approval_policy
+12 -28
View File
@@ -3628,8 +3628,7 @@ async fn set_feature_enabled_updates_profile() -> anyhow::Result<()> {
}
#[tokio::test]
async fn set_feature_enabled_persists_default_false_feature_disable_in_profile()
-> anyhow::Result<()> {
async fn set_feature_enabled_persists_feature_disable_in_profile() -> anyhow::Result<()> {
let codex_home = TempDir::new()?;
ConfigEditsBuilder::new(codex_home.path())
@@ -6741,10 +6740,7 @@ approvals_reviewer = "guardian_subagent"
.build()
.await?;
assert_eq!(
config.approvals_reviewer,
ApprovalsReviewer::GuardianSubagent
);
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview);
Ok(())
}
@@ -6757,17 +6753,14 @@ async fn requirements_disallowing_default_approvals_reviewer_falls_back_to_requi
.codex_home(codex_home.path().to_path_buf())
.cloud_requirements(CloudRequirementsLoader::new(async {
Ok(Some(crate::config_loader::ConfigRequirementsToml {
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]),
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]),
..Default::default()
}))
}))
.build()
.await?;
assert_eq!(
config.approvals_reviewer,
ApprovalsReviewer::GuardianSubagent
);
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview);
Ok(())
}
@@ -6786,17 +6779,14 @@ async fn root_approvals_reviewer_falls_back_when_disallowed_by_requirements() ->
.fallback_cwd(Some(codex_home.path().to_path_buf()))
.cloud_requirements(CloudRequirementsLoader::new(async {
Ok(Some(crate::config_loader::ConfigRequirementsToml {
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]),
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]),
..Default::default()
}))
}))
.build()
.await?;
assert_eq!(
config.approvals_reviewer,
ApprovalsReviewer::GuardianSubagent
);
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview);
assert!(
config.startup_warnings.iter().any(|warning| {
warning
@@ -6826,17 +6816,14 @@ approvals_reviewer = "user"
.fallback_cwd(Some(codex_home.path().to_path_buf()))
.cloud_requirements(CloudRequirementsLoader::new(async {
Ok(Some(crate::config_loader::ConfigRequirementsToml {
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]),
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]),
..Default::default()
}))
}))
.build()
.await?;
assert_eq!(
config.approvals_reviewer,
ApprovalsReviewer::GuardianSubagent
);
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview);
Ok(())
}
@@ -6857,7 +6844,7 @@ async fn approvals_reviewer_preserves_valid_user_choice_when_allowed_by_requirem
Ok(Some(crate::config_loader::ConfigRequirementsToml {
allowed_approvals_reviewers: Some(vec![
ApprovalsReviewer::User,
ApprovalsReviewer::GuardianSubagent,
ApprovalsReviewer::AutoReview,
]),
..Default::default()
}))
@@ -6865,10 +6852,7 @@ async fn approvals_reviewer_preserves_valid_user_choice_when_allowed_by_requirem
.build()
.await?;
assert_eq!(
config.approvals_reviewer,
ApprovalsReviewer::GuardianSubagent
);
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview);
assert!(
config
.startup_warnings
@@ -6896,7 +6880,7 @@ smart_approvals = true
.build()
.await?;
assert!(!config.features.enabled(Feature::GuardianApproval));
assert!(config.features.enabled(Feature::GuardianApproval));
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::User);
let serialized = tokio::fs::read_to_string(codex_home.path().join(CONFIG_TOML_FILE)).await?;
@@ -6925,7 +6909,7 @@ smart_approvals = true
.build()
.await?;
assert!(!config.features.enabled(Feature::GuardianApproval));
assert!(config.features.enabled(Feature::GuardianApproval));
assert_eq!(config.approvals_reviewer, ApprovalsReviewer::User);
let serialized = tokio::fs::read_to_string(codex_home.path().join(CONFIG_TOML_FILE)).await?;
+3 -6
View File
@@ -1048,7 +1048,7 @@ impl From<LegacyManagedConfigToml> for ConfigRequirementsToml {
}
if let Some(approvals_reviewer) = approvals_reviewer {
let mut allowed_reviewers = vec![approvals_reviewer];
if approvals_reviewer == ApprovalsReviewer::GuardianSubagent {
if approvals_reviewer == ApprovalsReviewer::AutoReview {
allowed_reviewers.push(ApprovalsReviewer::User);
}
config_requirements_toml.allowed_approvals_reviewers = Some(allowed_reviewers);
@@ -1135,7 +1135,7 @@ foo = "xyzzy"
fn legacy_managed_config_backfill_allows_user_when_guardian_is_required() {
let legacy = LegacyManagedConfigToml {
approval_policy: None,
approvals_reviewer: Some(ApprovalsReviewer::GuardianSubagent),
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
sandbox_mode: None,
};
@@ -1143,10 +1143,7 @@ foo = "xyzzy"
assert_eq!(
requirements.allowed_approvals_reviewers,
Some(vec![
ApprovalsReviewer::GuardianSubagent,
ApprovalsReviewer::User,
])
Some(vec![ApprovalsReviewer::AutoReview, ApprovalsReviewer::User,])
);
}
@@ -188,7 +188,7 @@ fn approval_text(
),
};
if approvals_reviewer == ApprovalsReviewer::GuardianSubagent
if approvals_reviewer == ApprovalsReviewer::AutoReview
&& approval_policy != AskForApproval::Never
{
format!("{text}\n\n{AUTO_REVIEW_APPROVAL_SUFFIX}")
@@ -197,10 +197,10 @@ fn on_request_includes_tool_guidance_alongside_inline_permission_guidance_when_b
}
#[test]
fn guardian_subagent_approvals_append_guardian_specific_guidance() {
fn auto_review_approvals_append_auto_review_specific_guidance() {
let text = approval_text(
AskForApproval::OnRequest,
ApprovalsReviewer::GuardianSubagent,
ApprovalsReviewer::AutoReview,
&Policy::empty(),
/*exec_permission_approvals_enabled*/ false,
/*request_permissions_tool_enabled*/ false,
@@ -212,10 +212,10 @@ fn guardian_subagent_approvals_append_guardian_specific_guidance() {
}
#[test]
fn guardian_subagent_approvals_omit_guardian_specific_guidance_when_approval_is_never() {
fn auto_review_approvals_omit_auto_review_specific_guidance_when_approval_is_never() {
let text = approval_text(
AskForApproval::Never,
ApprovalsReviewer::GuardianSubagent,
ApprovalsReviewer::AutoReview,
&Policy::empty(),
/*exec_permission_approvals_enabled*/ false,
/*request_permissions_tool_enabled*/ false,
+1 -1
View File
@@ -146,7 +146,7 @@ pub(crate) fn routes_approval_to_guardian(turn: &TurnContext) -> bool {
matches!(
turn.approval_policy.value(),
AskForApproval::OnRequest | AskForApproval::Granular(_)
) && turn.config.approvals_reviewer == ApprovalsReviewer::GuardianSubagent
) && turn.config.approvals_reviewer == ApprovalsReviewer::AutoReview
}
pub(crate) fn is_guardian_reviewer_source(
+2 -2
View File
@@ -909,7 +909,7 @@ async fn routes_approval_to_guardian_requires_guardian_reviewer() {
assert!(!routes_approval_to_guardian(&turn));
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
turn.config = Arc::new(config);
assert!(routes_approval_to_guardian(&turn));
@@ -919,7 +919,7 @@ async fn routes_approval_to_guardian_requires_guardian_reviewer() {
async fn routes_approval_to_guardian_allows_granular_review_policy() {
let (_session, mut turn) = crate::session::tests::make_session_and_context().await;
let mut config = (*turn.config).clone();
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
turn.config = Arc::new(config);
turn.approval_policy
.set(AskForApproval::Granular(GranularApprovalConfig {
+3 -3
View File
@@ -1411,7 +1411,7 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() {
.expect("test setup should allow updating approval policy");
let mut config = (*turn_context.config).clone();
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
let config = Arc::new(config);
let models_manager = Arc::new(crate::test_support::models_manager_with_provider(
config.codex_home.to_path_buf(),
@@ -1490,7 +1490,7 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() {
.expect("test setup should allow updating approval policy");
let mut config = (*turn_context.config).clone();
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
let config = Arc::new(config);
let models_manager = Arc::new(crate::test_support::models_manager_with_provider(
config.codex_home.to_path_buf(),
@@ -1947,7 +1947,7 @@ async fn approve_mode_routes_arc_ask_user_to_guardian_when_guardian_reviewer_is_
let mut config = (*turn_context.config).clone();
config.chatgpt_base_url = server.uri();
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
let config = Arc::new(config);
let models_manager = Arc::new(crate::test_support::models_manager_with_provider(
config.codex_home.to_path_buf(),
+34 -1
View File
@@ -1893,6 +1893,7 @@ impl Session {
return Some(RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
});
}
AskForApproval::Granular(granular_config)
@@ -1901,6 +1902,7 @@ impl Session {
return Some(RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
});
}
AskForApproval::OnFailure
@@ -1944,11 +1946,13 @@ impl Session {
RequestPermissionsResponse {
permissions: requested_permissions.clone(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
}
}
ReviewDecision::ApprovedForSession => RequestPermissionsResponse {
permissions: requested_permissions.clone(),
scope: PermissionGrantScope::Session,
strict_auto_review: false,
},
ReviewDecision::NetworkPolicyAmendment {
network_policy_amendment,
@@ -1956,16 +1960,19 @@ impl Session {
NetworkPolicyRuleAction::Allow => RequestPermissionsResponse {
permissions: requested_permissions.clone(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
},
NetworkPolicyRuleAction::Deny => RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
},
},
ReviewDecision::Abort | ReviewDecision::Denied | ReviewDecision::TimedOut => {
RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
}
}
};
@@ -2137,6 +2144,14 @@ impl Session {
response: RequestPermissionsResponse,
cwd: &Path,
) -> RequestPermissionsResponse {
if response.strict_auto_review && matches!(response.scope, PermissionGrantScope::Session) {
return RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
};
}
if response.permissions.is_empty() {
return response;
}
@@ -2149,6 +2164,7 @@ impl Session {
)
.into(),
scope: response.scope,
strict_auto_review: response.strict_auto_review,
}
}
@@ -2164,7 +2180,11 @@ impl Session {
PermissionGrantScope::Turn => {
if let Some(turn_state) = originating_turn_state {
let mut ts = turn_state.lock().await;
ts.record_granted_permissions(response.permissions.clone().into());
let permissions: PermissionProfile = response.permissions.clone().into();
ts.record_granted_permissions(permissions);
if response.strict_auto_review {
ts.enable_strict_auto_review();
}
}
}
PermissionGrantScope::Session => {
@@ -2185,6 +2205,19 @@ impl Session {
ts.granted_permissions()
}
#[expect(
clippy::await_holding_invalid_type,
reason = "active turn reads must stay consistent with the matching turn state"
)]
pub(crate) async fn strict_auto_review_enabled_for_turn(&self) -> bool {
let active = self.active_turn.lock().await;
let Some(active) = active.as_ref() else {
return false;
};
let ts = active.turn_state.lock().await;
ts.strict_auto_review_enabled()
}
pub(crate) async fn granted_session_permissions(&self) -> Option<PermissionProfile> {
let state = self.state.lock().await;
state.granted_permissions()
+69 -2
View File
@@ -3352,6 +3352,7 @@ async fn notify_request_permissions_response_ignores_unmatched_call_id() {
..RequestPermissionProfile::default()
},
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
},
)
.await;
@@ -3381,6 +3382,7 @@ async fn record_granted_request_permissions_for_turn_uses_originating_turn() {
&codex_protocol::request_permissions::RequestPermissionsResponse {
permissions: requested_permissions.clone(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
},
Some(&originating_turn_state),
)
@@ -3394,6 +3396,67 @@ async fn record_granted_request_permissions_for_turn_uses_originating_turn() {
assert_eq!(session.granted_turn_permissions().await, None);
}
#[tokio::test]
async fn enable_strict_auto_review_for_turn_uses_originating_turn() {
let (session, _turn_context) = make_session_and_context().await;
let originating_active_turn = ActiveTurn::default();
let originating_turn_state = Arc::clone(&originating_active_turn.turn_state);
*session.active_turn.lock().await = Some(originating_active_turn);
let requested_permissions = RequestPermissionProfile {
network: Some(codex_protocol::models::NetworkPermissions {
enabled: Some(true),
}),
..RequestPermissionProfile::default()
};
session
.record_granted_request_permissions_for_turn(
&codex_protocol::request_permissions::RequestPermissionsResponse {
permissions: requested_permissions.clone(),
scope: PermissionGrantScope::Turn,
strict_auto_review: true,
},
Some(&originating_turn_state),
)
.await;
assert!(
originating_turn_state
.lock()
.await
.strict_auto_review_enabled()
);
}
#[test]
fn strict_auto_review_session_scope_grants_no_permissions() {
let requested_permissions = RequestPermissionProfile {
network: Some(codex_protocol::models::NetworkPermissions {
enabled: Some(true),
}),
..RequestPermissionProfile::default()
};
let response = Session::normalize_request_permissions_response(
requested_permissions.clone(),
codex_protocol::request_permissions::RequestPermissionsResponse {
permissions: requested_permissions,
scope: PermissionGrantScope::Session,
strict_auto_review: true,
},
std::path::Path::new("/tmp"),
);
assert_eq!(
response,
codex_protocol::request_permissions::RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
}
);
}
#[tokio::test]
async fn request_permissions_emits_event_when_granular_policy_allows_requests() {
let (session, mut turn_context, rx) = make_session_and_context_with_rx().await;
@@ -3421,6 +3484,7 @@ async fn request_permissions_emits_event_when_granular_policy_allows_requests()
..RequestPermissionProfile::default()
},
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
};
let handle = tokio::spawn({
@@ -3536,6 +3600,7 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec
codex_protocol::request_permissions::RequestPermissionsResponse {
permissions: request.permissions,
scope: PermissionGrantScope::Session,
strict_auto_review: false,
},
)
.await;
@@ -3550,6 +3615,7 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec
let expected_response = codex_protocol::request_permissions::RequestPermissionsResponse {
permissions: expected_permissions.clone(),
scope: PermissionGrantScope::Session,
strict_auto_review: false,
};
let response = tokio::time::timeout(StdDuration::from_secs(1), handle)
@@ -3606,6 +3672,7 @@ async fn request_permissions_is_auto_denied_when_granular_policy_blocks_tool_req
codex_protocol::request_permissions::RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
}
)
);
@@ -3827,7 +3894,7 @@ async fn user_turn_updates_approvals_reviewer() {
}],
cwd: config.cwd.to_path_buf(),
approval_policy: config.permissions.approval_policy.value(),
approvals_reviewer: Some(codex_config::types::ApprovalsReviewer::GuardianSubagent),
approvals_reviewer: Some(codex_config::types::ApprovalsReviewer::AutoReview),
sandbox_policy: config.permissions.sandbox_policy.get().clone(),
model: turn_context.model_info.slug.clone(),
effort: config.model_reasoning_effort,
@@ -3843,7 +3910,7 @@ async fn user_turn_updates_approvals_reviewer() {
let state = session.state.lock().await;
assert_eq!(
state.session_configuration.approvals_reviewer,
codex_config::types::ApprovalsReviewer::GuardianSubagent
codex_config::types::ApprovalsReviewer::AutoReview
);
}
@@ -88,7 +88,7 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() {
.enable(Feature::GuardianApproval)
.expect("test setup should allow enabling guardian approvals");
let mut config = (*turn_context_raw.config).clone();
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
let config = Arc::new(config);
let models_manager = Arc::new(crate::test_support::models_manager_with_provider(
@@ -131,6 +131,7 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() {
Some(RequestPermissionsResponse {
permissions: requested_permissions.clone(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
})
);
assert_eq!(
@@ -166,7 +167,7 @@ async fn request_permissions_guardian_review_stops_when_cancelled() {
.enable(Feature::GuardianApproval)
.expect("test setup should allow enabling guardian approvals");
let mut config = (*turn_context_raw.config).clone();
config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
let config = Arc::new(config);
let models_manager = Arc::new(crate::test_support::models_manager_with_provider(
@@ -379,6 +380,119 @@ async fn guardian_allows_shell_additional_permissions_requests_past_policy_valid
assert!(exec_output.output.contains("hi"));
}
#[tokio::test]
async fn strict_auto_review_turn_grant_forces_guardian_for_shell_policy_skip() {
let server = start_mock_server().await;
let guardian_request_log = mount_sse_once(
&server,
sse(vec![
ev_response_created("resp-guardian"),
ev_assistant_message(
"msg-guardian",
&serde_json::json!({
"risk_level": "low",
"user_authorization": "high",
"outcome": "allow",
"rationale": "The command stays within the strict turn permission grant.",
})
.to_string(),
),
ev_completed("resp-guardian"),
]),
)
.await;
let (mut session, mut turn_context_raw) = make_session_and_context().await;
let active_turn = crate::state::ActiveTurn::default();
let originating_turn_state = Arc::clone(&active_turn.turn_state);
*session.active_turn.lock().await = Some(active_turn);
session
.record_granted_request_permissions_for_turn(
&RequestPermissionsResponse {
permissions: RequestPermissionProfile {
network: Some(NetworkPermissions {
enabled: Some(true),
}),
..Default::default()
},
scope: PermissionGrantScope::Turn,
strict_auto_review: true,
},
Some(&originating_turn_state),
)
.await;
turn_context_raw
.approval_policy
.set(AskForApproval::OnFailure)
.expect("test setup should allow updating approval policy");
turn_context_raw
.sandbox_policy
.set(SandboxPolicy::DangerFullAccess)
.expect("test setup should allow updating sandbox policy");
turn_context_raw.file_system_sandbox_policy =
FileSystemSandboxPolicy::from(turn_context_raw.sandbox_policy.get());
turn_context_raw.network_sandbox_policy =
NetworkSandboxPolicy::from(turn_context_raw.sandbox_policy.get());
let mut config = (*turn_context_raw.config).clone();
config.approvals_reviewer = ApprovalsReviewer::User;
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
let config = Arc::new(config);
let models_manager = Arc::new(crate::test_support::models_manager_with_provider(
config.codex_home.to_path_buf(),
Arc::clone(&session.services.auth_manager),
config.model_provider.clone(),
));
session.services.models_manager = models_manager;
turn_context_raw.config = Arc::clone(&config);
turn_context_raw.provider = create_model_provider(
config.model_provider.clone(),
turn_context_raw.auth_manager.clone(),
);
let session = Arc::new(session);
let turn_context = Arc::new(turn_context_raw);
let handler = ShellHandler;
let command = if cfg!(windows) {
vec![
"cmd.exe".to_string(),
"/Q".to_string(),
"/D".to_string(),
"/C".to_string(),
"echo hi".to_string(),
]
} else {
vec![
"/bin/sh".to_string(),
"-c".to_string(),
"echo hi".to_string(),
]
};
let resp = handler
.handle(ToolInvocation {
session: Arc::clone(&session),
turn: Arc::clone(&turn_context),
cancellation_token: CancellationToken::new(),
tracker: Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())),
call_id: "strict-shell-call".to_string(),
tool_name: codex_tools::ToolName::plain("shell"),
payload: ToolPayload::Function {
arguments: serde_json::json!({
"command": command,
"workdir": Some(turn_context.cwd.to_string_lossy().to_string()),
"timeout_ms": 1_000_u64,
})
.to_string(),
},
})
.await;
let output = expect_text_output(&resp.expect("expected Ok result"));
assert!(output.contains("hi"));
let guardian_request = guardian_request_log.single_request();
assert!(guardian_request.body_contains_text("echo hi"));
}
#[tokio::test]
async fn guardian_allows_unified_exec_additional_permissions_requests_past_policy_validation() {
let (mut session, mut turn_context_raw) = make_session_and_context().await;
+9
View File
@@ -106,6 +106,7 @@ pub(crate) struct TurnState {
pending_input: Vec<ResponseInputItem>,
mailbox_delivery_phase: MailboxDeliveryPhase,
granted_permissions: Option<PermissionProfile>,
strict_auto_review_enabled: bool,
pub(crate) tool_calls: u64,
pub(crate) has_memory_citation: bool,
pub(crate) token_usage_at_turn_start: TokenUsage,
@@ -254,6 +255,14 @@ impl TurnState {
pub(crate) fn granted_permissions(&self) -> Option<PermissionProfile> {
self.granted_permissions.clone()
}
pub(crate) fn enable_strict_auto_review(&mut self) {
self.strict_auto_review_enabled = true;
}
pub(crate) fn strict_auto_review_enabled(&self) -> bool {
self.strict_auto_review_enabled
}
}
impl ActiveTurn {
+2 -2
View File
@@ -370,7 +370,7 @@ pub struct ExecCommandToolOutput {
pub process_id: Option<i32>,
pub exit_code: Option<i32>,
pub original_token_count: Option<usize>,
pub session_command: Option<Vec<String>>,
pub hook_command: Option<String>,
}
impl ToolOutput for ExecCommandToolOutput {
@@ -394,7 +394,7 @@ impl ToolOutput for ExecCommandToolOutput {
}
fn post_tool_use_response(&self, _call_id: &str, _payload: &ToolPayload) -> Option<JsonValue> {
if self.process_id.is_some() || self.session_command.is_none() {
if self.process_id.is_some() || self.hook_command.is_none() {
return None;
}
+1 -1
View File
@@ -390,7 +390,7 @@ fn exec_command_tool_output_formats_truncated_response() {
process_id: None,
exit_code: Some(0),
original_token_count: Some(10),
session_command: None,
hook_command: None,
}
.to_response_item("call-42", &payload);
@@ -324,11 +324,12 @@ impl ToolHandler for ApplyPatchHandler {
&self,
call_id: &str,
payload: &ToolPayload,
result: &dyn ToolOutput,
result: &Self::Output,
) -> Option<PostToolUsePayload> {
let tool_response = result.post_tool_use_response(call_id, payload)?;
Some(PostToolUsePayload {
tool_name: HookToolName::apply_patch(),
tool_use_id: call_id.to_string(),
command: apply_patch_payload_command(payload)?,
tool_response,
})
@@ -90,6 +90,7 @@ fn post_tool_use_payload_uses_patch_input_and_tool_output() {
handler.post_tool_use_payload("call-apply-patch", &payload, &output),
Some(PostToolUsePayload {
tool_name: HookToolName::apply_patch(),
tool_use_id: "call-apply-patch".to_string(),
command: patch.to_string(),
tool_response: json!("Success. Updated files."),
})
@@ -66,10 +66,7 @@ impl ToolHandler for Handler {
return Err(collab_agent_error(agent_id, err));
}
};
let result = session
.services
.agent_control
.close_agent(agent_id)
let result = Box::pin(session.services.agent_control.close_agent(agent_id))
.await
.map_err(|err| collab_agent_error(agent_id, err))
.map(|_| ());
@@ -61,7 +61,14 @@ impl ToolHandler for Handler {
.get_status(receiver_thread_id)
.await;
let (receiver_agent, error) = if matches!(status, AgentStatus::NotFound) {
match try_resume_closed_agent(&session, &turn, receiver_thread_id, child_depth).await {
match Box::pin(try_resume_closed_agent(
&session,
&turn,
receiver_thread_id,
child_depth,
))
.await
{
Ok(()) => {
status = session
.services
@@ -149,21 +156,18 @@ async fn try_resume_closed_agent(
child_depth: i32,
) -> Result<(), FunctionCallError> {
let config = build_agent_resume_config(turn.as_ref(), child_depth)?;
session
.services
.agent_control
.resume_agent_from_rollout(
config,
receiver_thread_id,
thread_spawn_source(
session.conversation_id,
&turn.session_source,
child_depth,
/*agent_role*/ None,
/*task_name*/ None,
)?,
)
.await
.map(|_| ())
.map_err(|err| collab_agent_error(receiver_thread_id, err))
Box::pin(session.services.agent_control.resume_agent_from_rollout(
config,
receiver_thread_id,
thread_spawn_source(
session.conversation_id,
&turn.session_source,
child_depth,
/*agent_role*/ None,
/*task_name*/ None,
)?,
))
.await
.map(|_| ())
.map_err(|err| collab_agent_error(receiver_thread_id, err))
}
@@ -82,26 +82,23 @@ impl ToolHandler for Handler {
apply_spawn_agent_runtime_overrides(&mut config, turn.as_ref())?;
apply_spawn_agent_overrides(&mut config, child_depth);
let result = session
.services
.agent_control
.spawn_agent_with_metadata(
config,
input_items,
Some(thread_spawn_source(
session.conversation_id,
&turn.session_source,
child_depth,
role_name,
/*task_name*/ None,
)?),
SpawnAgentOptions {
fork_parent_spawn_call_id: args.fork_context.then(|| call_id.clone()),
fork_mode: args.fork_context.then_some(SpawnAgentForkMode::FullHistory),
},
)
.await
.map_err(collab_spawn_error);
let result = Box::pin(session.services.agent_control.spawn_agent_with_metadata(
config,
input_items,
Some(thread_spawn_source(
session.conversation_id,
&turn.session_source,
child_depth,
role_name,
/*task_name*/ None,
)?),
SpawnAgentOptions {
fork_parent_spawn_call_id: args.fork_context.then(|| call_id.clone()),
fork_mode: args.fork_context.then_some(SpawnAgentForkMode::FullHistory),
},
))
.await
.map_err(collab_spawn_error);
let (new_thread_id, new_agent_metadata, status) = match &result {
Ok(spawned_agent) => (
Some(spawned_agent.thread_id),
+4 -2
View File
@@ -216,11 +216,12 @@ impl ToolHandler for ShellHandler {
&self,
call_id: &str,
payload: &ToolPayload,
result: &dyn ToolOutput,
result: &Self::Output,
) -> Option<PostToolUsePayload> {
let tool_response = result.post_tool_use_response(call_id, payload)?;
Some(PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: call_id.to_string(),
command: shell_payload_command(payload)?,
tool_response,
})
@@ -328,11 +329,12 @@ impl ToolHandler for ShellCommandHandler {
&self,
call_id: &str,
payload: &ToolPayload,
result: &dyn ToolOutput,
result: &Self::Output,
) -> Option<PostToolUsePayload> {
let tool_response = result.post_tool_use_response(call_id, payload)?;
Some(PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: call_id.to_string(),
command: shell_command_payload_command(payload)?,
tool_response,
})
@@ -284,6 +284,7 @@ fn build_post_tool_use_payload_uses_tool_output_wire_value() {
handler.post_tool_use_payload("call-42", &payload, &output),
Some(crate::tools::registry::PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: "call-42".to_string(),
command: "printf shell command".to_string(),
tool_response: json!("shell output"),
})
@@ -147,21 +147,23 @@ impl ToolHandler for UnifiedExecHandler {
&self,
call_id: &str,
payload: &ToolPayload,
result: &dyn ToolOutput,
result: &Self::Output,
) -> Option<PostToolUsePayload> {
let ToolPayload::Function { arguments } = payload else {
let ToolPayload::Function { .. } = payload else {
return None;
};
let args = parse_arguments::<ExecCommandArgs>(arguments).ok()?;
if args.tty {
return None;
}
let tool_response = result.post_tool_use_response(call_id, payload)?;
let command = result.hook_command.clone()?;
let tool_use_id = if result.event_call_id.is_empty() {
call_id.to_string()
} else {
result.event_call_id.clone()
};
let tool_response = result.post_tool_use_response(&tool_use_id, payload)?;
Some(PostToolUsePayload {
tool_name: HookToolName::bash(),
command: args.cmd,
tool_use_id,
command,
tool_response,
})
}
@@ -200,11 +202,12 @@ impl ToolHandler for UnifiedExecHandler {
"exec_command" => {
let cwd = resolve_workdir_base_path(&arguments, &context.turn.cwd)?;
let args: ExecCommandArgs = parse_arguments_with_base_path(&arguments, &cwd)?;
let hook_command = args.cmd.clone();
let workdir = context.turn.resolve_path(args.workdir.clone());
maybe_emit_implicit_skill_invocation(
session.as_ref(),
context.turn.as_ref(),
&args.cmd,
&hook_command,
&workdir,
)
.await;
@@ -313,17 +316,16 @@ impl ToolHandler for UnifiedExecHandler {
process_id: None,
exit_code: None,
original_token_count: None,
session_command: None,
hook_command: None,
});
}
emit_unified_exec_tty_metric(&turn.session_telemetry, tty);
let session_command = command.clone();
match manager
.exec_command(
ExecCommandRequest {
command,
hook_command: args.cmd,
hook_command: hook_command.clone(),
process_id,
yield_time_ms,
max_output_tokens,
@@ -357,7 +359,7 @@ impl ToolHandler for UnifiedExecHandler {
process_id: None,
exit_code: Some(output.exit_code),
original_token_count: Some(original_token_count),
session_command: Some(session_command),
hook_command: Some(hook_command),
}
}
Err(err) => {
@@ -252,7 +252,7 @@ fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_co
arguments: serde_json::json!({ "cmd": "echo three", "tty": false }).to_string(),
};
let output = ExecCommandToolOutput {
event_call_id: "event-43".to_string(),
event_call_id: "call-43".to_string(),
chunk_id: "chunk-1".to_string(),
wall_time: std::time::Duration::from_millis(498),
raw_output: b"three".to_vec(),
@@ -260,17 +260,14 @@ fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_co
process_id: None,
exit_code: Some(0),
original_token_count: None,
session_command: Some(vec![
"/bin/zsh".to_string(),
"-lc".to_string(),
"echo three".to_string(),
]),
hook_command: Some("echo three".to_string()),
};
assert_eq!(
UnifiedExecHandler.post_tool_use_payload("call-43", &payload, &output),
Some(crate::tools::registry::PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: "call-43".to_string(),
command: "echo three".to_string(),
tool_response: serde_json::json!("three"),
})
@@ -278,12 +275,12 @@ fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_co
}
#[test]
fn exec_command_post_tool_use_payload_skips_interactive_exec() {
fn exec_command_post_tool_use_payload_uses_output_for_interactive_completion() {
let payload = ToolPayload::Function {
arguments: serde_json::json!({ "cmd": "echo three", "tty": true }).to_string(),
};
let output = ExecCommandToolOutput {
event_call_id: "event-44".to_string(),
event_call_id: "call-44".to_string(),
chunk_id: "chunk-1".to_string(),
wall_time: std::time::Duration::from_millis(498),
raw_output: b"three".to_vec(),
@@ -291,16 +288,17 @@ fn exec_command_post_tool_use_payload_skips_interactive_exec() {
process_id: None,
exit_code: Some(0),
original_token_count: None,
session_command: Some(vec![
"/bin/zsh".to_string(),
"-lc".to_string(),
"echo three".to_string(),
]),
hook_command: Some("echo three".to_string()),
};
assert_eq!(
UnifiedExecHandler.post_tool_use_payload("call-44", &payload, &output),
None
Some(crate::tools::registry::PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: "call-44".to_string(),
command: "echo three".to_string(),
tool_response: serde_json::json!("three"),
})
);
}
@@ -318,11 +316,7 @@ fn exec_command_post_tool_use_payload_skips_running_sessions() {
process_id: Some(45),
exit_code: None,
original_token_count: None,
session_command: Some(vec![
"/bin/zsh".to_string(),
"-lc".to_string(),
"echo three".to_string(),
]),
hook_command: Some("echo three".to_string()),
};
assert_eq!(
@@ -330,3 +324,87 @@ fn exec_command_post_tool_use_payload_skips_running_sessions() {
None
);
}
#[test]
fn write_stdin_post_tool_use_payload_uses_original_exec_call_id_and_command_on_completion() {
let payload = ToolPayload::Function {
arguments: serde_json::json!({
"session_id": 45,
"chars": "",
})
.to_string(),
};
let output = ExecCommandToolOutput {
event_call_id: "exec-call-45".to_string(),
chunk_id: "chunk-2".to_string(),
wall_time: std::time::Duration::from_millis(498),
raw_output: b"finished\n".to_vec(),
max_output_tokens: None,
process_id: None,
exit_code: Some(0),
original_token_count: None,
hook_command: Some("sleep 1; echo finished".to_string()),
};
assert_eq!(
UnifiedExecHandler.post_tool_use_payload("write-stdin-call", &payload, &output),
Some(crate::tools::registry::PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: "exec-call-45".to_string(),
command: "sleep 1; echo finished".to_string(),
tool_response: serde_json::json!("finished\n"),
})
);
}
#[test]
fn write_stdin_post_tool_use_payload_keeps_parallel_session_metadata_separate() {
let payload = ToolPayload::Function {
arguments: serde_json::json!({ "session_id": 45, "chars": "" }).to_string(),
};
let output_a = ExecCommandToolOutput {
event_call_id: "exec-call-a".to_string(),
chunk_id: "chunk-a".to_string(),
wall_time: std::time::Duration::from_millis(498),
raw_output: b"alpha\n".to_vec(),
max_output_tokens: None,
process_id: None,
exit_code: Some(0),
original_token_count: None,
hook_command: Some("sleep 2; echo alpha".to_string()),
};
let output_b = ExecCommandToolOutput {
event_call_id: "exec-call-b".to_string(),
chunk_id: "chunk-b".to_string(),
wall_time: std::time::Duration::from_millis(498),
raw_output: b"beta\n".to_vec(),
max_output_tokens: None,
process_id: None,
exit_code: Some(0),
original_token_count: None,
hook_command: Some("sleep 1; echo beta".to_string()),
};
let payloads = [
UnifiedExecHandler.post_tool_use_payload("write-call-b", &payload, &output_b),
UnifiedExecHandler.post_tool_use_payload("write-call-a", &payload, &output_a),
];
assert_eq!(
payloads,
[
Some(crate::tools::registry::PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: "exec-call-b".to_string(),
command: "sleep 1; echo beta".to_string(),
tool_response: serde_json::json!("beta\n"),
}),
Some(crate::tools::registry::PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: "exec-call-a".to_string(),
command: "sleep 2; echo alpha".to_string(),
tool_response: serde_json::json!("alpha\n"),
}),
]
);
}
+78 -65
View File
@@ -116,7 +116,8 @@ impl ToolOrchestrator {
let otel = turn_ctx.session_telemetry.clone();
let otel_tn = &tool_ctx.tool_name;
let otel_ci = &tool_ctx.call_id;
let use_guardian = routes_approval_to_guardian(turn_ctx);
let strict_auto_review = tool_ctx.session.strict_auto_review_enabled_for_turn().await;
let use_guardian = routes_approval_to_guardian(turn_ctx) || strict_auto_review;
// 1) Approval
let mut already_approved = false;
@@ -126,12 +127,37 @@ impl ToolOrchestrator {
});
match requirement {
ExecApprovalRequirement::Skip { .. } => {
otel.tool_decision(
otel_tn,
otel_ci,
&ReviewDecision::Approved,
ToolDecisionSource::Config,
);
if strict_auto_review {
let guardian_review_id = Some(new_guardian_review_id());
let approval_ctx = ApprovalCtx {
session: &tool_ctx.session,
turn: &tool_ctx.turn,
call_id: &tool_ctx.call_id,
guardian_review_id: guardian_review_id.clone(),
retry_reason: None,
network_approval_context: None,
};
let decision = Self::request_approval(
tool,
req,
tool_ctx.call_id.as_str(),
approval_ctx,
tool_ctx,
/*evaluate_permission_request_hooks*/ false,
&otel,
)
.await?;
Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision)
.await?;
already_approved = true;
} else {
otel.tool_decision(
otel_tn,
otel_ci,
&ReviewDecision::Approved,
ToolDecisionSource::Config,
);
}
}
ExecApprovalRequirement::Forbidden { reason } => {
return Err(ToolError::Rejected(reason));
@@ -152,35 +178,13 @@ impl ToolOrchestrator {
tool_ctx.call_id.as_str(),
approval_ctx,
tool_ctx,
use_guardian,
/*evaluate_permission_request_hooks*/ !strict_auto_review,
&otel,
)
.await?;
match decision {
ReviewDecision::Denied | ReviewDecision::Abort => {
let reason = if let Some(review_id) = guardian_review_id.as_deref() {
guardian_rejection_message(tool_ctx.session.as_ref(), review_id).await
} else {
"rejected by user".to_string()
};
return Err(ToolError::Rejected(reason));
}
ReviewDecision::TimedOut => {
return Err(ToolError::Rejected(guardian_timeout_message()));
}
ReviewDecision::Approved
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::ApprovedForSession => {}
ReviewDecision::NetworkPolicyAmendment {
network_policy_amendment,
} => match network_policy_amendment.action {
NetworkPolicyRuleAction::Allow => {}
NetworkPolicyRuleAction::Deny => {
return Err(ToolError::Rejected("rejected by user".to_string()));
}
},
}
Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision)
.await?;
already_approved = true;
}
}
@@ -287,9 +291,10 @@ impl ToolOrchestrator {
build_denial_reason_from_output(output.as_ref())
};
// Ask for approval before retrying with the escalated sandbox.
let bypass_retry_approval = tool
.should_bypass_approval(approval_policy, already_approved)
// Strict auto-review approval covers the sandboxed attempt only;
// retrying without the sandbox requires a fresh guardian review.
let bypass_retry_approval = !strict_auto_review
&& tool.should_bypass_approval(approval_policy, already_approved)
&& network_approval_context.is_none();
if !bypass_retry_approval {
let guardian_review_id = use_guardian.then(new_guardian_review_id);
@@ -309,36 +314,13 @@ impl ToolOrchestrator {
&permission_request_run_id,
approval_ctx,
tool_ctx,
use_guardian,
/*evaluate_permission_request_hooks*/ !strict_auto_review,
&otel,
)
.await?;
match decision {
ReviewDecision::Denied | ReviewDecision::Abort => {
let reason = if let Some(review_id) = guardian_review_id.as_deref() {
guardian_rejection_message(tool_ctx.session.as_ref(), review_id)
.await
} else {
"rejected by user".to_string()
};
return Err(ToolError::Rejected(reason));
}
ReviewDecision::TimedOut => {
return Err(ToolError::Rejected(guardian_timeout_message()));
}
ReviewDecision::Approved
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::ApprovedForSession => {}
ReviewDecision::NetworkPolicyAmendment {
network_policy_amendment,
} => match network_policy_amendment.action {
NetworkPolicyRuleAction::Allow => {}
NetworkPolicyRuleAction::Deny => {
return Err(ToolError::Rejected("rejected by user".to_string()));
}
},
}
Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision)
.await?;
}
let escalated_attempt = SandboxAttempt {
@@ -385,13 +367,15 @@ impl ToolOrchestrator {
permission_request_run_id: &str,
approval_ctx: ApprovalCtx<'_>,
tool_ctx: &ToolCtx,
use_guardian: bool,
evaluate_permission_request_hooks: bool,
otel: &codex_otel::SessionTelemetry,
) -> Result<ReviewDecision, ToolError>
where
T: ToolRuntime<Rq, Out>,
{
if let Some(permission_request) = tool.permission_request_payload(req) {
if evaluate_permission_request_hooks
&& let Some(permission_request) = tool.permission_request_payload(req)
{
match run_permission_request_hooks(
approval_ctx.session,
approval_ctx.turn,
@@ -424,12 +408,12 @@ impl ToolOrchestrator {
}
}
let decision = tool.start_approval_async(req, approval_ctx).await;
let otel_source = if use_guardian {
let otel_source = if approval_ctx.guardian_review_id.is_some() {
ToolDecisionSource::AutomatedReviewer
} else {
ToolDecisionSource::User
};
let decision = tool.start_approval_async(req, approval_ctx).await;
otel.tool_decision(
&tool_ctx.tool_name,
&tool_ctx.call_id,
@@ -438,6 +422,35 @@ impl ToolOrchestrator {
);
Ok(decision)
}
async fn reject_if_not_approved(
tool_ctx: &ToolCtx,
guardian_review_id: Option<&str>,
decision: ReviewDecision,
) -> Result<(), ToolError> {
match decision {
ReviewDecision::Denied | ReviewDecision::Abort => {
let reason = if let Some(review_id) = guardian_review_id {
guardian_rejection_message(tool_ctx.session.as_ref(), review_id).await
} else {
"rejected by user".to_string()
};
Err(ToolError::Rejected(reason))
}
ReviewDecision::TimedOut => Err(ToolError::Rejected(guardian_timeout_message())),
ReviewDecision::Approved
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::ApprovedForSession => Ok(()),
ReviewDecision::NetworkPolicyAmendment {
network_policy_amendment,
} => match network_policy_amendment.action {
NetworkPolicyRuleAction::Allow => Ok(()),
NetworkPolicyRuleAction::Deny => {
Err(ToolError::Rejected("rejected by user".to_string()))
}
},
}
}
}
fn build_denial_reason_from_output(_output: &ExecToolCallOutput) -> String {
+1
View File
@@ -178,6 +178,7 @@ impl ToolCallRuntime {
result: Box::new(AbortedToolOutput {
message: Self::abort_message(call, secs),
}),
post_tool_use_payload: None,
}
}
+13 -29
View File
@@ -72,7 +72,7 @@ pub trait ToolHandler: Send + Sync {
&self,
_call_id: &str,
_payload: &ToolPayload,
_result: &dyn ToolOutput,
_result: &Self::Output,
) -> Option<PostToolUsePayload> {
None
}
@@ -107,6 +107,7 @@ pub(crate) struct AnyToolResult {
pub(crate) call_id: String,
pub(crate) payload: ToolPayload,
pub(crate) result: Box<dyn ToolOutput>,
pub(crate) post_tool_use_payload: Option<PostToolUsePayload>,
}
impl AnyToolResult {
@@ -143,9 +144,11 @@ pub(crate) struct PreToolUsePayload {
pub(crate) struct PostToolUsePayload {
/// Hook-facing tool name model.
///
/// Keep this aligned with the corresponding pre-use payload so external
/// hook consumers can pair events by `tool_use_id`.
/// The canonical name is serialized to hook stdin, while aliases are used
/// only for matcher compatibility.
pub(crate) tool_name: HookToolName,
/// The originating tool-use id exposed at `tool_use_id`.
pub(crate) tool_use_id: String,
/// Command-shaped input exposed at `tool_input.command`.
pub(crate) command: String,
/// Tool result exposed at `tool_response`.
@@ -159,15 +162,7 @@ trait AnyToolHandler: Send + Sync {
fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option<PreToolUsePayload>;
fn post_tool_use_payload(
&self,
call_id: &str,
payload: &ToolPayload,
result: &dyn ToolOutput,
) -> Option<PostToolUsePayload>;
fn create_diff_consumer(&self) -> Option<Box<dyn ToolArgumentDiffConsumer>>;
fn handle_any<'a>(
&'a self,
invocation: ToolInvocation,
@@ -190,19 +185,9 @@ where
ToolHandler::pre_tool_use_payload(self, invocation)
}
fn post_tool_use_payload(
&self,
call_id: &str,
payload: &ToolPayload,
result: &dyn ToolOutput,
) -> Option<PostToolUsePayload> {
ToolHandler::post_tool_use_payload(self, call_id, payload, result)
}
fn create_diff_consumer(&self) -> Option<Box<dyn ToolArgumentDiffConsumer>> {
ToolHandler::create_diff_consumer(self)
}
fn handle_any<'a>(
&'a self,
invocation: ToolInvocation,
@@ -211,10 +196,13 @@ where
let call_id = invocation.call_id.clone();
let payload = invocation.payload.clone();
let output = self.handle(invocation).await?;
let post_tool_use_payload =
ToolHandler::post_tool_use_payload(self, &call_id, &payload, &output);
Ok(AnyToolResult {
call_id,
payload,
result: Box::new(output),
post_tool_use_payload,
})
})
}
@@ -400,13 +388,9 @@ impl ToolRegistry {
emit_metric_for_tool_read(&invocation, success).await;
let post_tool_use_payload = if success {
let guard = response_cell.lock().await;
guard.as_ref().and_then(|result| {
handler.post_tool_use_payload(
&result.call_id,
&result.payload,
result.result.as_ref(),
)
})
guard
.as_ref()
.and_then(|result| result.post_tool_use_payload.clone())
} else {
None
};
@@ -415,7 +399,7 @@ impl ToolRegistry {
run_post_tool_use_hooks(
&invocation.session,
&invocation.turn,
invocation.call_id.clone(),
post_tool_use_payload.tool_use_id,
post_tool_use_payload.tool_name.name().to_string(),
post_tool_use_payload.tool_name.matcher_aliases().to_vec(),
post_tool_use_payload.command,
@@ -139,14 +139,14 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
let changes = req.changes.clone();
let guardian_review_id = ctx.guardian_review_id.clone();
Box::pin(async move {
if req.permissions_preapproved && retry_reason.is_none() {
return ReviewDecision::Approved;
}
if let Some(review_id) = guardian_review_id {
let action = ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id);
return review_approval_request(session, turn, review_id, action, retry_reason)
.await;
}
if req.permissions_preapproved && retry_reason.is_none() {
return ReviewDecision::Approved;
}
if let Some(reason) = retry_reason {
let rx_approve = session
.request_patch_approval(
+1 -1
View File
@@ -148,7 +148,7 @@ struct ProcessEntry {
process: Arc<UnifiedExecProcess>,
call_id: String,
process_id: i32,
command: Vec<String>,
hook_command: String,
tty: bool,
network_approval_id: Option<String>,
session: Weak<Session>,
+2 -2
View File
@@ -113,7 +113,7 @@ async fn exec_command_with_tty(
process: Arc::clone(&process),
call_id: context.call_id.clone(),
process_id,
command: command.clone(),
hook_command: cmd.to_string(),
tty,
network_approval_id: None,
session: Arc::downgrade(session),
@@ -165,7 +165,7 @@ async fn exec_command_with_tty(
process_id: response_process_id,
exit_code,
original_token_count: Some(approx_token_count(&text)),
session_command: Some(command),
hook_command: Some(cmd.to_string()),
})
}
@@ -169,7 +169,7 @@ struct PreparedProcessHandles {
output_closed_notify: Arc<Notify>,
cancellation_token: CancellationToken,
pause_state: Option<watch::Receiver<bool>>,
command: Vec<String>,
hook_command: String,
process_id: i32,
tty: bool,
}
@@ -279,6 +279,7 @@ impl UnifiedExecProcessManager {
Arc::clone(&process),
context,
&request.command,
request.hook_command.clone(),
cwd.clone(),
start,
request.process_id,
@@ -398,7 +399,7 @@ impl UnifiedExecProcessManager {
process_id: response_process_id,
exit_code,
original_token_count: Some(original_token_count),
session_command: Some(request.command.clone()),
hook_command: Some(request.hook_command.clone()),
};
Ok(response)
@@ -418,7 +419,7 @@ impl UnifiedExecProcessManager {
output_closed_notify,
cancellation_token,
pause_state,
command: session_command,
hook_command,
process_id,
tty,
..
@@ -517,7 +518,7 @@ impl UnifiedExecProcessManager {
process_id,
exit_code,
original_token_count: Some(original_token_count),
session_command: Some(session_command.clone()),
hook_command: Some(hook_command),
};
Ok(response)
@@ -585,7 +586,7 @@ impl UnifiedExecProcessManager {
output_closed_notify,
cancellation_token,
pause_state,
command: entry.command.clone(),
hook_command: entry.hook_command.clone(),
process_id: entry.process_id,
tty: entry.tty,
})
@@ -597,6 +598,7 @@ impl UnifiedExecProcessManager {
process: Arc<UnifiedExecProcess>,
context: &UnifiedExecContext,
command: &[String],
hook_command: String,
cwd: AbsolutePathBuf,
started_at: Instant,
process_id: i32,
@@ -608,7 +610,7 @@ impl UnifiedExecProcessManager {
process: Arc::clone(&process),
call_id: context.call_id.clone(),
process_id,
command: command.to_vec(),
hook_command,
tty,
network_approval_id,
session: Arc::downgrade(&context.session),
+165
View File
@@ -33,6 +33,7 @@ use core_test_support::responses::mount_sse_sequence;
use core_test_support::responses::sse;
use core_test_support::responses::start_mock_server;
use core_test_support::skip_if_no_network;
use core_test_support::skip_if_windows;
use core_test_support::streaming_sse::StreamingSseChunk;
use core_test_support::streaming_sse::start_streaming_sse_server;
use core_test_support::test_codex::test_codex;
@@ -407,6 +408,64 @@ elif mode == "exit_2":
Ok(())
}
fn write_logging_pre_and_blocking_post_tool_use_hooks(home: &Path, feedback: &str) -> Result<()> {
let pre_script_path = home.join("pre_tool_use_hook.py");
let pre_log_path = home.join("pre_tool_use_hook_log.jsonl");
let post_script_path = home.join("post_tool_use_hook.py");
let post_log_path = home.join("post_tool_use_hook_log.jsonl");
let feedback_json =
serde_json::to_string(feedback).context("serialize post tool use feedback")?;
let pre_script = format!(
r#"import json
from pathlib import Path
import sys
payload = json.load(sys.stdin)
with Path(r"{pre_log_path}").open("a", encoding="utf-8") as handle:
handle.write(json.dumps(payload) + "\n")
"#,
pre_log_path = pre_log_path.display(),
);
let post_script = format!(
r#"import json
from pathlib import Path
import sys
payload = json.load(sys.stdin)
with Path(r"{post_log_path}").open("a", encoding="utf-8") as handle:
handle.write(json.dumps(payload) + "\n")
sys.stderr.write({feedback_json} + "\n")
raise SystemExit(2)
"#,
post_log_path = post_log_path.display(),
);
let hooks = serde_json::json!({
"hooks": {
"PreToolUse": [{
"matcher": "Bash",
"hooks": [{
"type": "command",
"command": format!("python3 {}", pre_script_path.display()),
"statusMessage": "running pre tool use hook",
}]
}],
"PostToolUse": [{
"matcher": "Bash",
"hooks": [{
"type": "command",
"command": format!("python3 {}", post_script_path.display()),
"statusMessage": "running post tool use hook",
}]
}]
}
});
fs::write(&pre_script_path, pre_script).context("write pre tool use hook script")?;
fs::write(&post_script_path, post_script).context("write post tool use hook script")?;
fs::write(home.join("hooks.json"), hooks.to_string()).context("write hooks.json")?;
Ok(())
}
fn write_session_start_hook_recording_transcript(home: &Path) -> Result<()> {
let script_path = home.join("session_start_hook.py");
let log_path = home.join("session_start_hook_log.jsonl");
@@ -2514,6 +2573,112 @@ async fn post_tool_use_exit_two_replaces_one_shot_exec_command_output_with_feedb
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn post_tool_use_blocks_when_exec_session_completes_via_write_stdin() -> Result<()> {
skip_if_no_network!(Ok(()));
skip_if_windows!(Ok(()));
let server = start_mock_server().await;
let start_call_id = "posttooluse-exec-session-start";
let poll_call_id = "posttooluse-exec-session-poll";
let command = "sleep 1; printf session-post-hook-output".to_string();
let start_args = serde_json::json!({
"cmd": command,
"shell": "/bin/sh",
"login": false,
"tty": false,
"yield_time_ms": 250,
});
let poll_args = serde_json::json!({
"session_id": 1000,
"chars": "",
"yield_time_ms": 5_000,
});
let feedback = "blocked by session post hook";
let responses = mount_sse_sequence(
&server,
vec![
sse(vec![
ev_response_created("resp-1"),
core_test_support::responses::ev_function_call(
start_call_id,
"exec_command",
&serde_json::to_string(&start_args)?,
),
ev_completed("resp-1"),
]),
sse(vec![
ev_response_created("resp-2"),
core_test_support::responses::ev_function_call(
poll_call_id,
"write_stdin",
&serde_json::to_string(&poll_args)?,
),
ev_completed("resp-2"),
]),
sse(vec![
ev_response_created("resp-3"),
ev_assistant_message("msg-1", "session post hook observed"),
ev_completed("resp-3"),
]),
],
)
.await;
let mut builder = test_codex()
.with_pre_build_hook(|home| {
if let Err(error) = write_logging_pre_and_blocking_post_tool_use_hooks(home, feedback) {
panic!("failed to write tool use hook test fixture: {error}");
}
})
.with_config(|config| {
config.use_experimental_unified_exec_tool = true;
config
.features
.enable(Feature::CodexHooks)
.expect("test config should allow feature update");
config
.features
.enable(Feature::UnifiedExec)
.expect("test config should allow feature update");
});
let test = builder.build(&server).await?;
test.submit_turn("run the exec command session with post hook")
.await?;
let requests = responses.requests();
assert_eq!(requests.len(), 3);
let output_item = requests[2].function_call_output(poll_call_id);
let output = output_item
.get("output")
.and_then(Value::as_str)
.expect("write_stdin output string");
assert_eq!(output, feedback);
let pre_hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())?;
assert_eq!(pre_hook_inputs.len(), 1);
assert_eq!(pre_hook_inputs[0]["tool_name"], "Bash");
assert_eq!(pre_hook_inputs[0]["tool_use_id"], start_call_id);
assert_eq!(pre_hook_inputs[0]["tool_input"]["command"], command);
let post_hook_inputs = read_post_tool_use_hook_inputs(test.codex_home_path())?;
assert_eq!(post_hook_inputs.len(), 1);
assert_eq!(post_hook_inputs[0]["hook_event_name"], "PostToolUse");
assert_eq!(post_hook_inputs[0]["tool_name"], "Bash");
assert_eq!(post_hook_inputs[0]["tool_use_id"], start_call_id);
assert_eq!(post_hook_inputs[0]["tool_input"]["command"], command);
assert!(
post_hook_inputs[0]["tool_response"]
.as_str()
.is_some_and(|tool_response| tool_response.contains("session-post-hook-output")),
"PostToolUse should see the final session output, got {:?}",
post_hook_inputs[0]["tool_response"]
);
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn post_tool_use_records_additional_context_for_apply_patch() -> Result<()> {
skip_if_no_network!(Ok(()));
@@ -483,6 +483,7 @@ async fn request_permissions_tool_is_auto_denied_when_granular_request_permissio
RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
}
);
@@ -1089,6 +1090,7 @@ async fn request_permissions_grants_apply_to_later_exec_command_calls() -> Resul
response: RequestPermissionsResponse {
permissions: normalized_requested_permissions.clone(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
},
})
.await?;
@@ -1203,6 +1205,7 @@ async fn request_permissions_preapprove_explicit_exec_permissions_outside_on_req
response: RequestPermissionsResponse {
permissions: normalized_requested_permissions,
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
},
})
.await?;
@@ -1316,6 +1319,7 @@ async fn request_permissions_grants_apply_to_later_shell_command_calls() -> Resu
response: RequestPermissionsResponse {
permissions: normalized_requested_permissions.clone(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
},
})
.await?;
@@ -1425,6 +1429,7 @@ async fn request_permissions_grants_apply_to_later_shell_command_calls_without_i
response: RequestPermissionsResponse {
permissions: normalized_requested_permissions.clone(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
},
})
.await?;
@@ -1571,6 +1576,7 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions()
response: RequestPermissionsResponse {
permissions: granted_permissions.clone(),
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
},
})
.await?;
@@ -1692,6 +1698,7 @@ async fn request_permissions_grants_do_not_carry_across_turns() -> Result<()> {
response: RequestPermissionsResponse {
permissions: normalized_requested_permissions,
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
},
})
.await?;
@@ -1809,6 +1816,7 @@ async fn request_permissions_session_grants_carry_across_turns() -> Result<()> {
response: RequestPermissionsResponse {
permissions: normalized_requested_permissions,
scope: PermissionGrantScope::Session,
strict_auto_review: false,
},
})
.await?;
@@ -4,6 +4,7 @@
use anyhow::Result;
use codex_core::config::Constrained;
use codex_features::Feature;
use codex_protocol::config_types::ApprovalsReviewer;
use codex_protocol::models::FileSystemPermissions;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::EventMsg;
@@ -134,6 +135,7 @@ async fn submit_turn(
prompt: &str,
approval_policy: AskForApproval,
sandbox_policy: SandboxPolicy,
approvals_reviewer: Option<ApprovalsReviewer>,
) -> Result<()> {
let session_model = test.session_configured.model.clone();
test.codex
@@ -146,7 +148,7 @@ async fn submit_turn(
final_output_json_schema: None,
cwd: test.cwd.path().to_path_buf(),
approval_policy,
approvals_reviewer: None,
approvals_reviewer,
sandbox_policy,
model: session_model,
effort: None,
@@ -159,6 +161,13 @@ async fn submit_turn(
Ok(())
}
async fn wait_for_completion(test: &TestCodex) {
wait_for_event(&test.codex, |event| {
matches!(event, EventMsg::TurnComplete(_))
})
.await;
}
async fn expect_request_permissions_event(
test: &TestCodex,
expected_call_id: &str,
@@ -248,6 +257,7 @@ async fn approved_folder_write_request_permissions_unblocks_later_exec_without_s
"write outside the workspace",
approval_policy,
sandbox_policy,
/*approvals_reviewer*/ None,
)
.await?;
@@ -262,6 +272,7 @@ async fn approved_folder_write_request_permissions_unblocks_later_exec_without_s
response: RequestPermissionsResponse {
permissions: normalized_requested_permissions,
scope: PermissionGrantScope::Turn,
strict_auto_review: false,
},
})
.await?;
@@ -305,11 +316,17 @@ async fn approved_folder_write_request_permissions_unblocks_later_exec_without_s
#[tokio::test(flavor = "current_thread")]
#[cfg(target_os = "macos")]
async fn approved_folder_write_request_permissions_unblocks_later_apply_patch_without_prompt()
-> Result<()> {
async fn approved_folder_write_request_permissions_unblocks_later_apply_patch() -> Result<()> {
skip_if_no_network!(Ok(()));
skip_if_sandbox!(Ok(()));
apply_patch_after_request_permissions(/*strict_auto_review*/ false).await?;
apply_patch_after_request_permissions(/*strict_auto_review*/ true).await?;
Ok(())
}
async fn apply_patch_after_request_permissions(strict_auto_review: bool) -> Result<()> {
let server = start_mock_server().await;
let approval_policy = AskForApproval::OnRequest;
let sandbox_policy = workspace_write_excluding_tmp();
@@ -330,43 +347,72 @@ async fn approved_folder_write_request_permissions_unblocks_later_apply_patch_wi
let test = builder.build(&server).await?;
let requested_dir = tempfile::tempdir()?;
let requested_file = requested_dir.path().join("allowed-patch.txt");
let requested_file_name = if strict_auto_review {
"strict-allowed-patch.txt"
} else {
"allowed-patch.txt"
};
let patch_content = if strict_auto_review {
"patched-after-strict-review"
} else {
"patched-via-request-permissions"
};
let requested_file = requested_dir.path().join(requested_file_name);
let requested_permissions = requested_directory_write_permissions(requested_dir.path());
let normalized_requested_permissions =
normalized_directory_write_permissions(requested_dir.path())?;
let patch = build_add_file_patch(&requested_file, "patched-via-request-permissions");
let patch = build_add_file_patch(&requested_file, patch_content);
let responses = mount_sse_sequence(
&server,
vec![
sse(vec![
ev_response_created("resp-request-permissions-patch-1"),
request_permissions_tool_event(
"permissions-call",
"Allow patching outside the workspace",
&requested_permissions,
)?,
ev_completed("resp-request-permissions-patch-1"),
]),
sse(vec![
ev_response_created("resp-request-permissions-patch-2"),
ev_apply_patch_function_call("apply-patch-call", &patch),
ev_completed("resp-request-permissions-patch-2"),
]),
sse(vec![
ev_response_created("resp-request-permissions-patch-3"),
ev_assistant_message("msg-request-permissions-patch-1", "done"),
ev_completed("resp-request-permissions-patch-3"),
]),
],
)
.await;
let response_prefix = if strict_auto_review {
"resp-strict-request-permissions-patch"
} else {
"resp-request-permissions-patch"
};
let mut sse_sequence = vec![
sse(vec![
ev_response_created(&format!("{response_prefix}-1")),
request_permissions_tool_event(
"permissions-call",
"Allow patching outside the workspace",
&requested_permissions,
)?,
ev_completed(&format!("{response_prefix}-1")),
]),
sse(vec![
ev_response_created(&format!("{response_prefix}-2")),
ev_apply_patch_function_call("apply-patch-call", &patch),
ev_completed(&format!("{response_prefix}-2")),
]),
];
if strict_auto_review {
sse_sequence.push(sse(vec![
ev_response_created(&format!("{response_prefix}-guardian")),
ev_assistant_message(
"msg-strict-request-permissions-patch-guardian",
&serde_json::json!({
"risk_level": "low",
"user_authorization": "high",
"outcome": "allow",
"rationale": "The patch stays within the strict turn grant.",
})
.to_string(),
),
ev_completed(&format!("{response_prefix}-guardian")),
]));
}
sse_sequence.push(sse(vec![
ev_response_created(&format!("{response_prefix}-3")),
ev_assistant_message("msg-request-permissions-patch-1", "done"),
ev_completed(&format!("{response_prefix}-3")),
]));
let responses = mount_sse_sequence(&server, sse_sequence).await;
submit_turn(
&test,
"patch outside the workspace",
approval_policy,
sandbox_policy,
strict_auto_review.then_some(ApprovalsReviewer::User),
)
.await?;
@@ -381,26 +427,38 @@ async fn approved_folder_write_request_permissions_unblocks_later_apply_patch_wi
response: RequestPermissionsResponse {
permissions: normalized_requested_permissions,
scope: PermissionGrantScope::Turn,
strict_auto_review,
},
})
.await?;
let event = wait_for_event(&test.codex, |event| {
matches!(
event,
EventMsg::ApplyPatchApprovalRequest(_) | EventMsg::TurnComplete(_)
)
})
.await;
match event {
EventMsg::TurnComplete(_) => {}
EventMsg::ApplyPatchApprovalRequest(approval) => {
panic!(
"unexpected apply_patch approval request after granted permissions: {:?}",
approval.call_id
if strict_auto_review {
wait_for_completion(&test).await;
let guardian_request = responses
.requests()
.into_iter()
.find(|request| request.body_contains_text(requested_file_name))
.expect("expected guardian request for strict apply_patch");
assert!(guardian_request.body_contains_text(requested_file_name));
assert!(guardian_request.body_contains_text(patch_content));
} else {
let event = wait_for_event(&test.codex, |event| {
matches!(
event,
EventMsg::ApplyPatchApprovalRequest(_) | EventMsg::TurnComplete(_)
)
})
.await;
match event {
EventMsg::TurnComplete(_) => {}
EventMsg::ApplyPatchApprovalRequest(approval) => {
panic!(
"unexpected apply_patch approval request after granted permissions: {:?}",
approval.call_id
)
}
other => panic!("unexpected event: {other:?}"),
}
other => panic!("unexpected event: {other:?}"),
}
let patch_output = responses
@@ -415,7 +473,7 @@ async fn approved_folder_write_request_permissions_unblocks_later_apply_patch_wi
);
assert_eq!(
fs::read_to_string(&requested_file)?,
"patched-via-request-permissions\n"
format!("{patch_content}\n")
);
Ok(())
+63
View File
@@ -618,6 +618,69 @@ async fn remote_stdio_server_uses_runtime_fallback_cwd_when_config_omits_cwd() -
Ok(())
}
#[cfg(unix)]
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
#[serial(mcp_cwd)]
async fn local_stdio_server_uses_runtime_fallback_cwd_when_config_omits_cwd() -> anyhow::Result<()>
{
skip_if_no_network!(Ok(()));
let server = responses::start_mock_server().await;
let server_name = "rmcp_local_fallback_cwd";
let expected_cwd = Arc::new(Mutex::new(None::<PathBuf>));
let expected_cwd_for_config = Arc::clone(&expected_cwd);
let rmcp_test_server_bin = cargo_bin("test_stdio_server")?;
let relative_server_path = PathBuf::from("mcp-bin").join(
rmcp_test_server_bin
.file_name()
.expect("test stdio server binary should have a file name"),
);
let relative_command = relative_server_path.to_string_lossy().into_owned();
let fixture = test_codex()
.with_config(move |config| {
*expected_cwd_for_config
.lock()
.expect("expected cwd lock should not be poisoned") =
Some(config.cwd.to_path_buf());
let target_bin = config.cwd.join(&relative_server_path).into_path_buf();
let target_dir = target_bin
.parent()
.expect("relative test server path should include a parent");
fs::create_dir_all(target_dir).expect("create relative MCP bin directory");
fs::copy(&rmcp_test_server_bin, &target_bin).expect("copy test stdio server");
insert_mcp_server(
config,
server_name,
stdio_transport(
relative_command,
Some(HashMap::from([(
"MCP_TEST_VALUE".to_string(),
"local-fallback-cwd".to_string(),
)])),
Vec::new(),
),
TestMcpServerOptions::default(),
);
})
.build(&server)
.await?;
let expected_cwd = expected_cwd
.lock()
.expect("expected cwd lock should not be poisoned")
.clone()
.expect("test config should record runtime fallback cwd");
let structured =
call_cwd_tool(&server, &fixture, server_name, "call-local-fallback-cwd").await?;
assert_cwd_tool_output(&structured, &expected_cwd);
server.verify().await;
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
async fn stdio_mcp_tool_call_includes_sandbox_state_meta() -> anyhow::Result<()> {
skip_if_no_network!(Ok(()));
+2
View File
@@ -14,7 +14,9 @@ workspace = true
arc-swap = { workspace = true }
async-trait = { workspace = true }
base64 = { workspace = true }
bytes = { workspace = true }
codex-app-server-protocol = { workspace = true }
codex-client = { workspace = true }
codex-config = { workspace = true }
codex-protocol = { workspace = true }
codex-sandboxing = { workspace = true }
+24
View File
@@ -8,6 +8,8 @@ use std::time::Duration;
use arc_swap::ArcSwap;
use codex_app_server_protocol::JSONRPCNotification;
use futures::FutureExt;
use futures::future::BoxFuture;
use serde_json::Value;
use tokio::sync::Mutex;
use tokio::sync::OnceCell;
@@ -20,6 +22,7 @@ use tracing::debug;
use crate::ProcessId;
use crate::client_api::ExecServerClientConnectOptions;
use crate::client_api::HttpClient;
use crate::client_api::RemoteExecServerConnectArgs;
use crate::connection::JsonRpcConnection;
use crate::process::ExecProcessEvent;
@@ -206,6 +209,25 @@ impl LazyRemoteExecServerClient {
}
}
impl HttpClient for LazyRemoteExecServerClient {
fn http_request(
&self,
params: crate::HttpRequestParams,
) -> BoxFuture<'_, Result<crate::HttpRequestResponse, ExecServerError>> {
async move { self.get().await?.http_request(params).await }.boxed()
}
fn http_request_stream(
&self,
params: crate::HttpRequestParams,
) -> BoxFuture<
'_,
Result<(crate::HttpRequestResponse, crate::HttpResponseBodyStream), ExecServerError>,
> {
async move { self.get().await?.http_request_stream(params).await }.boxed()
}
}
#[derive(Debug, thiserror::Error)]
pub enum ExecServerError {
#[error("failed to spawn exec-server: {0}")]
@@ -226,6 +248,8 @@ pub enum ExecServerError {
Disconnected(String),
#[error("failed to serialize or deserialize exec-server JSON: {0}")]
Json(#[from] serde_json::Error),
#[error("HTTP request failed: {0}")]
HttpRequest(String),
#[error("exec-server protocol error: {0}")]
Protocol(String),
#[error("exec-server rejected request ({code}): {message}")]
+24 -520
View File
@@ -1,522 +1,26 @@
use std::collections::HashMap;
use std::sync::Arc;
use std::sync::atomic::Ordering;
use std::time::Duration;
//! HTTP client capability implementations shared by local and remote environments.
//!
//! This module is the facade for the environment-owned [`crate::HttpClient`]
//! capability:
//! - [`ReqwestHttpClient`] executes requests directly with `reqwest`
//! - [`ExecServerClient`] forwards requests over the JSON-RPC transport
//! - [`HttpResponseBodyStream`] presents buffered local bodies and streamed
//! remote `http/request/bodyDelta` notifications through one byte-stream API
//!
//! Runtime split:
//! - orchestrator process: holds an `Arc<dyn HttpClient>` and chooses local or
//! remote execution
//! - remote runtime: serves the `http/request` RPC and runs the concrete local
//! HTTP request there when the orchestrator uses [`ExecServerClient`]
use codex_app_server_protocol::JSONRPCErrorError;
use futures::StreamExt;
use reqwest::Method;
use reqwest::Url;
use reqwest::header::HeaderMap;
use reqwest::header::HeaderName;
use reqwest::header::HeaderValue;
use serde_json::Value;
use serde_json::from_value;
use tokio::runtime::Handle;
use tokio::sync::mpsc;
use tokio::sync::mpsc::error::TrySendError;
use tracing::debug;
#[path = "reqwest_http_client.rs"]
mod reqwest_http_client;
#[path = "http_response_body_stream.rs"]
pub(crate) mod response_body_stream;
#[path = "rpc_http_client.rs"]
mod rpc_http_client;
use super::ExecServerClient;
use super::ExecServerError;
use super::Inner;
use crate::protocol::HTTP_REQUEST_BODY_DELTA_METHOD;
use crate::protocol::HTTP_REQUEST_METHOD;
use crate::protocol::HttpHeader;
use crate::protocol::HttpRequestBodyDeltaNotification;
use crate::protocol::HttpRequestParams;
use crate::protocol::HttpRequestResponse;
use crate::rpc::RpcNotificationSender;
use crate::rpc::internal_error;
use crate::rpc::invalid_params;
/// Maximum queued body frames per streamed executor HTTP response.
const HTTP_BODY_DELTA_CHANNEL_CAPACITY: usize = 256;
pub(crate) struct ExecutorPendingHttpBodyStream {
pub(crate) request_id: String,
response: reqwest::Response,
}
pub(crate) struct ExecutorHttpRequestRunner {
client: reqwest::Client,
}
/// Request-scoped stream of body chunks for an executor HTTP response.
///
/// The initial `http/request` call returns status and headers. This stream then
/// receives the ordered `http/request/bodyDelta` notifications for that request
/// id until EOF or a terminal error.
pub struct HttpResponseBodyStream {
inner: Arc<Inner>,
request_id: String,
next_seq: u64,
rx: mpsc::Receiver<HttpRequestBodyDeltaNotification>,
// Terminal frames can carry a final chunk; return that once, then EOF.
pending_eof: bool,
closed: bool,
}
impl ExecServerClient {
/// Performs an executor-side HTTP request and buffers the response body.
pub async fn http_request(
&self,
mut params: HttpRequestParams,
) -> Result<HttpRequestResponse, ExecServerError> {
params.stream_response = false;
self.call(HTTP_REQUEST_METHOD, &params).await
}
/// Performs an executor-side HTTP request and returns a body stream.
///
/// The method sets `stream_response` and replaces any caller-supplied
/// `request_id` with a connection-local id, so late deltas from abandoned
/// streams cannot be confused with later requests.
pub async fn http_request_stream(
&self,
mut params: HttpRequestParams,
) -> Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError> {
params.stream_response = true;
let request_id = self.inner.next_http_body_stream_request_id();
params.request_id = request_id.clone();
let (tx, rx) = mpsc::channel(HTTP_BODY_DELTA_CHANNEL_CAPACITY);
self.inner
.insert_http_body_stream(request_id.clone(), tx)
.await?;
let mut registration = HttpBodyStreamRegistration {
inner: Arc::clone(&self.inner),
request_id: request_id.clone(),
active: true,
};
let response = match self.call(HTTP_REQUEST_METHOD, &params).await {
Ok(response) => response,
Err(error) => {
self.inner.remove_http_body_stream(&request_id).await;
registration.active = false;
return Err(error);
}
};
registration.active = false;
Ok((
response,
HttpResponseBodyStream {
inner: Arc::clone(&self.inner),
request_id,
next_seq: 1,
rx,
pending_eof: false,
closed: false,
},
))
}
}
impl HttpResponseBodyStream {
/// Receives the next response-body chunk.
///
/// Returns `Ok(None)` at EOF and converts sequence gaps or executor-side
/// stream errors into protocol errors.
pub async fn recv(&mut self) -> Result<Option<Vec<u8>>, ExecServerError> {
if self.pending_eof {
self.pending_eof = false;
self.finish().await;
return Ok(None);
}
let Some(delta) = self.rx.recv().await else {
self.finish().await;
if let Some(error) = self
.inner
.take_http_body_stream_failure(&self.request_id)
.await
{
return Err(ExecServerError::Protocol(format!(
"http response stream `{}` failed: {error}",
self.request_id
)));
}
return Ok(None);
};
if delta.seq != self.next_seq {
self.finish().await;
return Err(ExecServerError::Protocol(format!(
"http response stream `{}` received seq {}, expected {}",
self.request_id, delta.seq, self.next_seq
)));
}
self.next_seq += 1;
let chunk = delta.delta.into_inner();
if let Some(error) = delta.error {
self.finish().await;
return Err(ExecServerError::Protocol(format!(
"http response stream `{}` failed: {error}",
self.request_id
)));
}
if delta.done {
self.finish().await;
if chunk.is_empty() {
return Ok(None);
}
self.pending_eof = true;
}
Ok(Some(chunk))
}
/// Removes this stream from the connection routing table once it reaches EOF.
async fn finish(&mut self) {
if self.closed {
return;
}
self.closed = true;
self.inner.remove_http_body_stream(&self.request_id).await;
}
}
impl Drop for HttpResponseBodyStream {
/// Schedules stream-route removal if the consumer drops before EOF.
fn drop(&mut self) {
if self.closed {
return;
}
self.closed = true;
spawn_remove_http_body_stream(Arc::clone(&self.inner), self.request_id.clone());
}
}
impl ExecutorHttpRequestRunner {
pub(crate) fn new(timeout_ms: Option<u64>) -> Result<Self, JSONRPCErrorError> {
let client = match timeout_ms {
None => reqwest::Client::builder(),
Some(timeout_ms) => {
reqwest::Client::builder().timeout(Duration::from_millis(timeout_ms))
}
}
.build()
.map_err(|err| internal_error(format!("failed to build http/request client: {err}")))?;
Ok(Self { client })
}
pub(crate) async fn run(
&self,
params: HttpRequestParams,
) -> Result<(HttpRequestResponse, Option<ExecutorPendingHttpBodyStream>), JSONRPCErrorError>
{
let method = Method::from_bytes(params.method.as_bytes())
.map_err(|err| invalid_params(format!("http/request method is invalid: {err}")))?;
let url = Url::parse(&params.url)
.map_err(|err| invalid_params(format!("http/request url is invalid: {err}")))?;
match url.scheme() {
"http" | "https" => {}
scheme => {
return Err(invalid_params(format!(
"http/request only supports http and https URLs, got {scheme}"
)));
}
}
let headers = Self::build_headers(params.headers)?;
let mut request = self.client.request(method, url).headers(headers);
if let Some(body) = params.body {
request = request.body(body.into_inner());
}
let response = request
.send()
.await
.map_err(|err| internal_error(format!("http/request failed: {err}")))?;
let status = response.status().as_u16();
let headers = Self::response_headers(response.headers());
if params.stream_response {
return Ok((
HttpRequestResponse {
status,
headers,
body: Vec::new().into(),
},
Some(ExecutorPendingHttpBodyStream {
request_id: params.request_id,
response,
}),
));
}
let body = response.bytes().await.map_err(|err| {
internal_error(format!("failed to read http/request response body: {err}"))
})?;
Ok((
HttpRequestResponse {
status,
headers,
body: body.to_vec().into(),
},
None,
))
}
fn build_headers(headers: Vec<HttpHeader>) -> Result<HeaderMap, JSONRPCErrorError> {
let mut header_map = HeaderMap::new();
for header in headers {
let name = HeaderName::from_bytes(header.name.as_bytes()).map_err(|err| {
invalid_params(format!("http/request header name is invalid: {err}"))
})?;
let value = HeaderValue::from_str(&header.value).map_err(|err| {
invalid_params(format!(
"http/request header value is invalid for {}: {err}",
header.name
))
})?;
header_map.append(name, value);
}
Ok(header_map)
}
fn response_headers(headers: &HeaderMap) -> Vec<HttpHeader> {
headers
.iter()
.filter_map(|(name, value)| {
Some(HttpHeader {
name: name.as_str().to_string(),
value: value.to_str().ok()?.to_string(),
})
})
.collect()
}
pub(crate) async fn stream_body(
pending_stream: ExecutorPendingHttpBodyStream,
notifications: RpcNotificationSender,
) {
let ExecutorPendingHttpBodyStream {
request_id,
response,
} = pending_stream;
let mut seq = 1;
let mut body = response.bytes_stream();
while let Some(chunk) = body.next().await {
match chunk {
Ok(bytes) => {
if !send_executor_body_delta(
&notifications,
HttpRequestBodyDeltaNotification {
request_id: request_id.clone(),
seq,
delta: bytes.to_vec().into(),
done: false,
error: None,
},
)
.await
{
return;
}
seq += 1;
}
Err(err) => {
let _ = send_executor_body_delta(
&notifications,
HttpRequestBodyDeltaNotification {
request_id,
seq,
delta: Vec::new().into(),
done: true,
error: Some(err.to_string()),
},
)
.await;
return;
}
}
}
let _ = send_executor_body_delta(
&notifications,
HttpRequestBodyDeltaNotification {
request_id,
seq,
delta: Vec::new().into(),
done: true,
error: None,
},
)
.await;
}
}
impl Inner {
/// Routes one streamed HTTP body notification into its request-local receiver.
pub(super) async fn handle_http_body_delta_notification(
&self,
params: Option<Value>,
) -> Result<(), ExecServerError> {
let params: HttpRequestBodyDeltaNotification = from_value(params.unwrap_or(Value::Null))?;
// Unknown request ids are ignored intentionally: a stream may have already
// reached EOF and released its route.
if let Some(tx) = self
.http_body_streams
.load()
.get(&params.request_id)
.cloned()
{
let request_id = params.request_id.clone();
let terminal_delta = params.done || params.error.is_some();
match tx.try_send(params) {
Ok(()) => {
if terminal_delta {
self.remove_http_body_stream(&request_id).await;
}
}
Err(TrySendError::Closed(_)) => {
self.remove_http_body_stream(&request_id).await;
debug!("http response stream receiver dropped before body delta delivery");
}
Err(TrySendError::Full(_)) => {
self.record_http_body_stream_failure(
&request_id,
"body delta channel filled before delivery".to_string(),
)
.await;
self.remove_http_body_stream(&request_id).await;
debug!(
"closing http response stream `{request_id}` after body delta backpressure"
);
}
}
}
Ok(())
}
/// Fails active streamed HTTP bodies so callers do not wait forever after a
/// transport disconnect or notification handling failure.
pub(super) async fn fail_all_http_body_streams(&self, message: String) {
let _streams_write_guard = self.http_body_streams_write_lock.lock().await;
let streams = self.http_body_streams.load();
let streams = streams.as_ref().clone();
self.http_body_streams.store(Arc::new(HashMap::new()));
for (request_id, tx) in streams {
if tx
.try_send(HttpRequestBodyDeltaNotification {
request_id: request_id.clone(),
seq: 1,
delta: Vec::new().into(),
done: true,
error: Some(message.clone()),
})
.is_err()
{
let mut next_failures = self.http_body_stream_failures.load().as_ref().clone();
next_failures.insert(request_id, message.clone());
self.http_body_stream_failures
.store(Arc::new(next_failures));
}
}
}
/// Allocates a connection-local streamed HTTP response id.
fn next_http_body_stream_request_id(&self) -> String {
let id = self
.http_body_stream_next_id
.fetch_add(1, Ordering::Relaxed);
format!("http-{id}")
}
/// Registers a request id before issuing an executor streaming HTTP call.
async fn insert_http_body_stream(
&self,
request_id: String,
tx: mpsc::Sender<HttpRequestBodyDeltaNotification>,
) -> Result<(), ExecServerError> {
let _streams_write_guard = self.http_body_streams_write_lock.lock().await;
let streams = self.http_body_streams.load();
if streams.contains_key(&request_id) {
return Err(ExecServerError::Protocol(format!(
"http response stream already registered for request {request_id}"
)));
}
let mut next_streams = streams.as_ref().clone();
next_streams.insert(request_id.clone(), tx);
self.http_body_streams.store(Arc::new(next_streams));
let failures = self.http_body_stream_failures.load();
if failures.contains_key(&request_id) {
let mut next_failures = failures.as_ref().clone();
next_failures.remove(&request_id);
self.http_body_stream_failures
.store(Arc::new(next_failures));
}
Ok(())
}
/// Removes a request id after EOF, terminal error, or request failure.
async fn remove_http_body_stream(
&self,
request_id: &str,
) -> Option<mpsc::Sender<HttpRequestBodyDeltaNotification>> {
let _streams_write_guard = self.http_body_streams_write_lock.lock().await;
let streams = self.http_body_streams.load();
let stream = streams.get(request_id).cloned();
stream.as_ref()?;
let mut next_streams = streams.as_ref().clone();
next_streams.remove(request_id);
self.http_body_streams.store(Arc::new(next_streams));
stream
}
async fn record_http_body_stream_failure(&self, request_id: &str, message: String) {
let _streams_write_guard = self.http_body_streams_write_lock.lock().await;
let failures = self.http_body_stream_failures.load();
let mut next_failures = failures.as_ref().clone();
next_failures.insert(request_id.to_string(), message);
self.http_body_stream_failures
.store(Arc::new(next_failures));
}
async fn take_http_body_stream_failure(&self, request_id: &str) -> Option<String> {
let _streams_write_guard = self.http_body_streams_write_lock.lock().await;
let failures = self.http_body_stream_failures.load();
let error = failures.get(request_id).cloned();
error.as_ref()?;
let mut next_failures = failures.as_ref().clone();
next_failures.remove(request_id);
self.http_body_stream_failures
.store(Arc::new(next_failures));
error
}
}
/// Active route registration owned while `http_request_stream` awaits headers.
struct HttpBodyStreamRegistration {
inner: Arc<Inner>,
request_id: String,
active: bool,
}
impl Drop for HttpBodyStreamRegistration {
/// Removes the route if the stream request future is cancelled before headers return.
fn drop(&mut self) {
if self.active {
spawn_remove_http_body_stream(Arc::clone(&self.inner), self.request_id.clone());
}
}
}
/// Schedules HTTP body route removal from synchronous drop paths.
fn spawn_remove_http_body_stream(inner: Arc<Inner>, request_id: String) {
if let Ok(handle) = Handle::try_current() {
handle.spawn(async move {
inner.remove_http_body_stream(&request_id).await;
});
}
}
async fn send_executor_body_delta(
notifications: &RpcNotificationSender,
delta: HttpRequestBodyDeltaNotification,
) -> bool {
notifications
.notify(HTTP_REQUEST_BODY_DELTA_METHOD, &delta)
.await
.is_ok()
}
pub(crate) use reqwest_http_client::PendingReqwestHttpBodyStream;
pub use reqwest_http_client::ReqwestHttpClient;
pub(crate) use reqwest_http_client::ReqwestHttpRequestRunner;
pub use response_body_stream::HttpResponseBodyStream;
@@ -0,0 +1,355 @@
//! Shared HTTP response-body stream plumbing for local and remote execution.
//!
//! This module owns the byte-stream type exposed by the `HttpClient`
//! capability plus the remote-side routing table used to turn
//! `http/request/bodyDelta` notifications back into per-request streams.
use std::collections::HashMap;
use std::pin::Pin;
use std::sync::Arc;
use std::sync::atomic::Ordering;
use bytes::Bytes;
use futures::StreamExt;
use reqwest::Response;
use serde_json::Value;
use serde_json::from_value;
use tokio::runtime::Handle;
use tokio::sync::mpsc;
use tokio::sync::mpsc::error::TrySendError;
use tracing::debug;
use crate::client::ExecServerError;
use crate::client::Inner;
use crate::protocol::HTTP_REQUEST_BODY_DELTA_METHOD;
use crate::protocol::HttpRequestBodyDeltaNotification;
use crate::rpc::RpcNotificationSender;
pub(super) struct HttpBodyStreamRegistration {
inner: Arc<Inner>,
request_id: String,
active: bool,
}
enum HttpResponseBodyStreamInner {
Local {
body: Pin<Box<dyn futures::Stream<Item = Result<Bytes, reqwest::Error>> + Send>>,
},
Remote {
inner: Arc<Inner>,
request_id: String,
next_seq: u64,
rx: mpsc::Receiver<HttpRequestBodyDeltaNotification>,
pending_eof: bool,
closed: bool,
},
}
/// Request-scoped stream of body chunks for an HTTP response.
///
/// The initial `http/request` call returns status and headers. This stream then
/// receives the ordered `http/request/bodyDelta` notifications for that request
/// id until EOF or a terminal error.
pub struct HttpResponseBodyStream {
inner: HttpResponseBodyStreamInner,
}
impl HttpResponseBodyStream {
pub(super) fn local(response: Response) -> Self {
Self {
inner: HttpResponseBodyStreamInner::Local {
body: Box::pin(response.bytes_stream()),
},
}
}
pub(super) fn remote(
inner: Arc<Inner>,
request_id: String,
rx: mpsc::Receiver<HttpRequestBodyDeltaNotification>,
) -> Self {
Self {
inner: HttpResponseBodyStreamInner::Remote {
inner,
request_id,
next_seq: 1,
rx,
pending_eof: false,
closed: false,
},
}
}
/// Receives the next response-body chunk.
///
/// Returns `Ok(None)` at EOF and converts sequence gaps or stream-side
/// stream errors into protocol errors.
pub async fn recv(&mut self) -> Result<Option<Vec<u8>>, ExecServerError> {
match &mut self.inner {
HttpResponseBodyStreamInner::Local { body } => match body.next().await {
Some(chunk) => match chunk {
Ok(bytes) => Ok(Some(bytes.to_vec())),
Err(error) => Err(ExecServerError::HttpRequest(error.to_string())),
},
None => Ok(None),
},
HttpResponseBodyStreamInner::Remote {
inner,
request_id,
next_seq,
rx,
pending_eof,
closed,
} => {
if *pending_eof {
*pending_eof = false;
finish_remote_stream(inner, request_id, closed).await;
return Ok(None);
}
let Some(delta) = rx.recv().await else {
finish_remote_stream(inner, request_id, closed).await;
if let Some(error) = inner.take_http_body_stream_failure(request_id).await {
return Err(ExecServerError::Protocol(format!(
"http response stream `{request_id}` failed: {error}",
)));
}
return Ok(None);
};
if delta.seq != *next_seq {
finish_remote_stream(inner, request_id, closed).await;
return Err(ExecServerError::Protocol(format!(
"http response stream `{request_id}` received seq {}, expected {}",
delta.seq, *next_seq
)));
}
*next_seq += 1;
let chunk = delta.delta.into_inner();
if let Some(error) = delta.error {
finish_remote_stream(inner, request_id, closed).await;
return Err(ExecServerError::Protocol(format!(
"http response stream `{request_id}` failed: {error}",
)));
}
if delta.done {
finish_remote_stream(inner, request_id, closed).await;
if chunk.is_empty() {
return Ok(None);
}
*pending_eof = true;
}
Ok(Some(chunk))
}
}
}
}
impl Drop for HttpResponseBodyStream {
/// Schedules stream-route removal if the consumer drops before EOF.
fn drop(&mut self) {
if let HttpResponseBodyStreamInner::Remote {
inner,
request_id,
closed,
..
} = &mut self.inner
{
if *closed {
return;
}
*closed = true;
spawn_remove_http_body_stream(Arc::clone(inner), request_id.clone());
}
}
}
impl HttpBodyStreamRegistration {
pub(super) fn new(inner: Arc<Inner>, request_id: String) -> Self {
Self {
inner,
request_id,
active: true,
}
}
pub(super) fn disarm(&mut self) {
self.active = false;
}
}
impl Drop for HttpBodyStreamRegistration {
/// Removes the route if the stream request future is cancelled before headers return.
fn drop(&mut self) {
if self.active {
spawn_remove_http_body_stream(Arc::clone(&self.inner), self.request_id.clone());
}
}
}
async fn finish_remote_stream(inner: &Arc<Inner>, request_id: &str, closed: &mut bool) {
if *closed {
return;
}
*closed = true;
inner.remove_http_body_stream(request_id).await;
}
/// Schedules HTTP body route removal from synchronous drop paths.
fn spawn_remove_http_body_stream(inner: Arc<Inner>, request_id: String) {
if let Ok(handle) = Handle::try_current() {
handle.spawn(async move {
inner.remove_http_body_stream(&request_id).await;
});
}
}
pub(super) async fn send_body_delta(
notifications: &RpcNotificationSender,
delta: HttpRequestBodyDeltaNotification,
) -> bool {
notifications
.notify(HTTP_REQUEST_BODY_DELTA_METHOD, &delta)
.await
.is_ok()
}
impl Inner {
/// Routes one streamed HTTP body notification into its request-local receiver.
pub(crate) async fn handle_http_body_delta_notification(
&self,
params: Option<Value>,
) -> Result<(), ExecServerError> {
let params: HttpRequestBodyDeltaNotification = from_value(params.unwrap_or(Value::Null))?;
// Unknown request ids are ignored intentionally: a stream may have already
// reached EOF and released its route.
if let Some(tx) = self
.http_body_streams
.load()
.get(&params.request_id)
.cloned()
{
let request_id = params.request_id.clone();
let terminal_delta = params.done || params.error.is_some();
match tx.try_send(params) {
Ok(()) => {
if terminal_delta {
self.remove_http_body_stream(&request_id).await;
}
}
Err(TrySendError::Closed(_)) => {
self.remove_http_body_stream(&request_id).await;
debug!("http response stream receiver dropped before body delta delivery");
}
Err(TrySendError::Full(_)) => {
self.record_http_body_stream_failure(
&request_id,
"body delta channel filled before delivery".to_string(),
)
.await;
self.remove_http_body_stream(&request_id).await;
debug!(
"closing http response stream `{request_id}` after body delta backpressure"
);
}
}
}
Ok(())
}
/// Fails active streamed HTTP bodies so callers do not wait forever after a
/// transport disconnect or notification handling failure.
pub(crate) async fn fail_all_http_body_streams(&self, message: String) {
let _streams_write_guard = self.http_body_streams_write_lock.lock().await;
let streams = self.http_body_streams.load();
let streams = streams.as_ref().clone();
self.http_body_streams.store(Arc::new(HashMap::new()));
for (request_id, tx) in streams {
if tx
.try_send(HttpRequestBodyDeltaNotification {
request_id: request_id.clone(),
seq: 1,
delta: Vec::new().into(),
done: true,
error: Some(message.clone()),
})
.is_err()
{
let mut next_failures = self.http_body_stream_failures.load().as_ref().clone();
next_failures.insert(request_id, message.clone());
self.http_body_stream_failures
.store(Arc::new(next_failures));
}
}
}
/// Allocates a connection-local streamed HTTP response id.
pub(super) fn next_http_body_stream_request_id(&self) -> String {
let id = self
.http_body_stream_next_id
.fetch_add(1, Ordering::Relaxed);
format!("http-{id}")
}
/// Registers a request id before issuing a streaming HTTP call.
pub(super) async fn insert_http_body_stream(
&self,
request_id: String,
tx: mpsc::Sender<HttpRequestBodyDeltaNotification>,
) -> Result<(), ExecServerError> {
let _streams_write_guard = self.http_body_streams_write_lock.lock().await;
let streams = self.http_body_streams.load();
if streams.contains_key(&request_id) {
return Err(ExecServerError::Protocol(format!(
"http response stream already registered for request {request_id}"
)));
}
let mut next_streams = streams.as_ref().clone();
next_streams.insert(request_id.clone(), tx);
self.http_body_streams.store(Arc::new(next_streams));
let failures = self.http_body_stream_failures.load();
if failures.contains_key(&request_id) {
let mut next_failures = failures.as_ref().clone();
next_failures.remove(&request_id);
self.http_body_stream_failures
.store(Arc::new(next_failures));
}
Ok(())
}
/// Removes a request id after EOF, terminal error, or request failure.
pub(super) async fn remove_http_body_stream(
&self,
request_id: &str,
) -> Option<mpsc::Sender<HttpRequestBodyDeltaNotification>> {
let _streams_write_guard = self.http_body_streams_write_lock.lock().await;
let streams = self.http_body_streams.load();
let stream = streams.get(request_id).cloned();
stream.as_ref()?;
let mut next_streams = streams.as_ref().clone();
next_streams.remove(request_id);
self.http_body_streams.store(Arc::new(next_streams));
stream
}
async fn record_http_body_stream_failure(&self, request_id: &str, message: String) {
let _streams_write_guard = self.http_body_streams_write_lock.lock().await;
let failures = self.http_body_stream_failures.load();
let mut next_failures = failures.as_ref().clone();
next_failures.insert(request_id.to_string(), message);
self.http_body_stream_failures
.store(Arc::new(next_failures));
}
async fn take_http_body_stream_failure(&self, request_id: &str) -> Option<String> {
let _streams_write_guard = self.http_body_streams_write_lock.lock().await;
let failures = self.http_body_stream_failures.load();
let error = failures.get(request_id).cloned();
error.as_ref()?;
let mut next_failures = failures.as_ref().clone();
next_failures.remove(request_id);
self.http_body_stream_failures
.store(Arc::new(next_failures));
error
}
}
@@ -0,0 +1,267 @@
//! `reqwest`-backed `HttpClient` implementation.
//!
//! This code runs wherever the real network request should originate:
//! - in a local environment, that means the orchestrator process
//! - in a remote environment, that means the remote runtime after the
//! orchestrator has forwarded `http/request` over JSON-RPC
use std::time::Duration;
use codex_app_server_protocol::JSONRPCErrorError;
use codex_client::build_reqwest_client_with_custom_ca;
use futures::FutureExt;
use futures::StreamExt;
use futures::future::BoxFuture;
use reqwest::Method;
use reqwest::Url;
use reqwest::header::HeaderMap;
use reqwest::header::HeaderName;
use reqwest::header::HeaderValue;
use super::HttpResponseBodyStream;
use super::response_body_stream::send_body_delta;
use crate::HttpClient;
use crate::client::ExecServerError;
use crate::protocol::HttpHeader;
use crate::protocol::HttpRequestBodyDeltaNotification;
use crate::protocol::HttpRequestParams;
use crate::protocol::HttpRequestResponse;
use crate::rpc::RpcNotificationSender;
use crate::rpc::internal_error;
use crate::rpc::invalid_params;
/// `HttpClient` implementation that performs the actual HTTP request with
/// `reqwest`.
#[derive(Clone, Default)]
pub struct ReqwestHttpClient;
/// Streaming response state held between the initial HTTP response and
/// downstream body-delta forwarding.
pub(crate) struct PendingReqwestHttpBodyStream {
pub(crate) request_id: String,
pub(crate) response: reqwest::Response,
}
/// Validates `http/request` parameters and runs the actual `reqwest` call used
/// by the exec-server route and the local [`HttpClient`] backend.
pub(crate) struct ReqwestHttpRequestRunner {
client: reqwest::Client,
}
impl ReqwestHttpClient {
fn build_client(timeout_ms: Option<u64>) -> Result<reqwest::Client, ExecServerError> {
let builder = match timeout_ms {
None => reqwest::Client::builder(),
Some(timeout_ms) => {
reqwest::Client::builder().timeout(Duration::from_millis(timeout_ms))
}
};
build_reqwest_client_with_custom_ca(builder)
.map_err(|error| ExecServerError::HttpRequest(error.to_string()))
}
}
impl HttpClient for ReqwestHttpClient {
fn http_request(
&self,
params: HttpRequestParams,
) -> BoxFuture<'_, Result<HttpRequestResponse, ExecServerError>> {
async move {
let runner = ReqwestHttpRequestRunner::new(params.timeout_ms)
.map_err(|error| ExecServerError::HttpRequest(error.message))?;
let (response, _) = runner
.run(HttpRequestParams {
stream_response: false,
..params
})
.await
.map_err(|error| ExecServerError::HttpRequest(error.message))?;
Ok(response)
}
.boxed()
}
fn http_request_stream(
&self,
params: HttpRequestParams,
) -> BoxFuture<'_, Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError>> {
async move {
let runner = ReqwestHttpRequestRunner::new(params.timeout_ms)
.map_err(|error| ExecServerError::HttpRequest(error.message))?;
let (response, pending_stream) = runner
.run(HttpRequestParams {
stream_response: true,
..params
})
.await
.map_err(|error| ExecServerError::HttpRequest(error.message))?;
let pending_stream = pending_stream.ok_or_else(|| {
ExecServerError::Protocol(
"http request stream did not return a response body stream".to_string(),
)
})?;
Ok((
response,
HttpResponseBodyStream::local(pending_stream.response),
))
}
.boxed()
}
}
impl ReqwestHttpRequestRunner {
pub(crate) fn new(timeout_ms: Option<u64>) -> Result<Self, JSONRPCErrorError> {
let client = ReqwestHttpClient::build_client(timeout_ms)
.map_err(|error| internal_error(error.to_string()))?;
Ok(Self { client })
}
pub(crate) async fn run(
&self,
params: HttpRequestParams,
) -> Result<(HttpRequestResponse, Option<PendingReqwestHttpBodyStream>), JSONRPCErrorError>
{
let method = Method::from_bytes(params.method.as_bytes())
.map_err(|error| invalid_params(format!("http/request method is invalid: {error}")))?;
let url = Url::parse(&params.url)
.map_err(|error| invalid_params(format!("http/request url is invalid: {error}")))?;
match url.scheme() {
"http" | "https" => {}
scheme => {
return Err(invalid_params(format!(
"http/request only supports http and https URLs, got {scheme}"
)));
}
}
let headers = Self::build_headers(params.headers)?;
let mut request = self.client.request(method, url).headers(headers);
if let Some(body) = params.body {
request = request.body(body.into_inner());
}
let response = request
.send()
.await
.map_err(|error| internal_error(format!("http/request failed: {error}")))?;
let status = response.status().as_u16();
let headers = Self::response_headers(response.headers());
if params.stream_response {
return Ok((
HttpRequestResponse {
status,
headers,
body: Vec::new().into(),
},
Some(PendingReqwestHttpBodyStream {
request_id: params.request_id,
response,
}),
));
}
let body = response.bytes().await.map_err(|error| {
internal_error(format!(
"failed to read http/request response body: {error}"
))
})?;
Ok((
HttpRequestResponse {
status,
headers,
body: body.to_vec().into(),
},
None,
))
}
pub(crate) async fn stream_body(
pending_stream: PendingReqwestHttpBodyStream,
notifications: RpcNotificationSender,
) {
let PendingReqwestHttpBodyStream {
request_id,
response,
} = pending_stream;
let mut seq = 1;
let mut body = response.bytes_stream();
while let Some(chunk) = body.next().await {
match chunk {
Ok(bytes) => {
if !send_body_delta(
&notifications,
HttpRequestBodyDeltaNotification {
request_id: request_id.clone(),
seq,
delta: bytes.to_vec().into(),
done: false,
error: None,
},
)
.await
{
return;
}
seq += 1;
}
Err(error) => {
let _ = send_body_delta(
&notifications,
HttpRequestBodyDeltaNotification {
request_id,
seq,
delta: Vec::new().into(),
done: true,
error: Some(error.to_string()),
},
)
.await;
return;
}
}
}
let _ = send_body_delta(
&notifications,
HttpRequestBodyDeltaNotification {
request_id,
seq,
delta: Vec::new().into(),
done: true,
error: None,
},
)
.await;
}
fn build_headers(headers: Vec<HttpHeader>) -> Result<HeaderMap, JSONRPCErrorError> {
let mut header_map = HeaderMap::new();
for header in headers {
let name = HeaderName::from_bytes(header.name.as_bytes()).map_err(|error| {
invalid_params(format!("http/request header name is invalid: {error}"))
})?;
let value = HeaderValue::from_str(&header.value).map_err(|error| {
invalid_params(format!(
"http/request header value is invalid for {}: {error}",
header.name
))
})?;
header_map.append(name, value);
}
Ok(header_map)
}
fn response_headers(headers: &HeaderMap) -> Vec<HttpHeader> {
headers
.iter()
.filter_map(|(name, value)| {
Some(HttpHeader {
name: name.as_str().to_string(),
value: value.to_str().ok()?.to_string(),
})
})
.collect()
}
}
@@ -0,0 +1,88 @@
//! JSON-RPC-backed `HttpClient` implementation.
//!
//! This code runs in the orchestrator process. It does not issue network
//! requests directly; instead it forwards `http/request` to the remote runtime
//! and then reconstructs streamed bodies from `http/request/bodyDelta`
//! notifications on the shared connection.
use std::sync::Arc;
use futures::FutureExt;
use futures::future::BoxFuture;
use tokio::sync::mpsc;
use super::HttpResponseBodyStream;
use super::response_body_stream::HttpBodyStreamRegistration;
use crate::HttpClient;
use crate::client::ExecServerClient;
use crate::client::ExecServerError;
use crate::protocol::HTTP_REQUEST_METHOD;
use crate::protocol::HttpRequestParams;
use crate::protocol::HttpRequestResponse;
/// Maximum queued body frames per streamed HTTP response.
const HTTP_BODY_DELTA_CHANNEL_CAPACITY: usize = 256;
impl ExecServerClient {
/// Performs an HTTP request and buffers the response body.
pub async fn http_request(
&self,
mut params: HttpRequestParams,
) -> Result<HttpRequestResponse, ExecServerError> {
params.stream_response = false;
self.call(HTTP_REQUEST_METHOD, &params).await
}
/// Performs an HTTP request and returns a body stream.
///
/// The method sets `stream_response` and replaces any caller-supplied
/// `request_id` with a connection-local id, so late deltas from abandoned
/// streams cannot be confused with later requests.
pub async fn http_request_stream(
&self,
mut params: HttpRequestParams,
) -> Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError> {
params.stream_response = true;
let request_id = self.inner.next_http_body_stream_request_id();
params.request_id = request_id.clone();
let (tx, rx) = mpsc::channel(HTTP_BODY_DELTA_CHANNEL_CAPACITY);
self.inner
.insert_http_body_stream(request_id.clone(), tx)
.await?;
let mut registration =
HttpBodyStreamRegistration::new(Arc::clone(&self.inner), request_id.clone());
let response = match self.call(HTTP_REQUEST_METHOD, &params).await {
Ok(response) => response,
Err(error) => {
self.inner.remove_http_body_stream(&request_id).await;
registration.disarm();
return Err(error);
}
};
registration.disarm();
Ok((
response,
HttpResponseBodyStream::remote(Arc::clone(&self.inner), request_id, rx),
))
}
}
impl HttpClient for ExecServerClient {
/// Orchestrator-side adapter that forwards buffered HTTP requests to the
/// remote runtime over the shared JSON-RPC connection.
fn http_request(
&self,
params: HttpRequestParams,
) -> BoxFuture<'_, Result<HttpRequestResponse, ExecServerError>> {
async move { ExecServerClient::http_request(self, params).await }.boxed()
}
/// Orchestrator-side adapter that forwards streamed HTTP requests to the
/// remote runtime and exposes body deltas as a byte stream.
fn http_request_stream(
&self,
params: HttpRequestParams,
) -> BoxFuture<'_, Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError>> {
async move { ExecServerClient::http_request_stream(self, params).await }.boxed()
}
}
+26
View File
@@ -1,5 +1,12 @@
use std::time::Duration;
use futures::future::BoxFuture;
use crate::ExecServerError;
use crate::HttpRequestParams;
use crate::HttpRequestResponse;
use crate::HttpResponseBodyStream;
/// Connection options for any exec-server client transport.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct ExecServerClientConnectOptions {
@@ -17,3 +24,22 @@ pub struct RemoteExecServerConnectArgs {
pub initialize_timeout: Duration,
pub resume_session_id: Option<String>,
}
/// Sends HTTP requests through a runtime-selected transport.
///
/// This is the HTTP capability counterpart to [`crate::ExecBackend`]. Callers
/// use it when they need environment-owned network requests but should not
/// depend on the concrete connection type or how that connection is established.
pub trait HttpClient: Send + Sync {
/// Perform an HTTP request and buffer the response body.
fn http_request(
&self,
params: HttpRequestParams,
) -> BoxFuture<'_, Result<HttpRequestResponse, ExecServerError>>;
/// Perform an HTTP request and return a streamed body handle.
fn http_request_stream(
&self,
params: HttpRequestParams,
) -> BoxFuture<'_, Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError>>;
}
+12 -1
View File
@@ -3,7 +3,9 @@ use std::sync::Arc;
use crate::ExecServerError;
use crate::ExecServerRuntimePaths;
use crate::HttpClient;
use crate::client::LazyRemoteExecServerClient;
use crate::client::http_client::ReqwestHttpClient;
use crate::file_system::ExecutorFileSystem;
use crate::local_file_system::LocalFileSystem;
use crate::local_process::LocalProcess;
@@ -136,6 +138,7 @@ pub struct Environment {
exec_server_url: Option<String>,
exec_backend: Arc<dyn ExecBackend>,
filesystem: Arc<dyn ExecutorFileSystem>,
http_client: Arc<dyn HttpClient>,
local_runtime_paths: Option<ExecServerRuntimePaths>,
}
@@ -146,6 +149,7 @@ impl Environment {
exec_server_url: None,
exec_backend: Arc::new(LocalProcess::default()),
filesystem: Arc::new(LocalFileSystem::unsandboxed()),
http_client: Arc::new(ReqwestHttpClient),
local_runtime_paths: None,
}
}
@@ -202,6 +206,7 @@ impl Environment {
filesystem: Arc::new(LocalFileSystem::with_runtime_paths(
local_runtime_paths.clone(),
)),
http_client: Arc::new(ReqwestHttpClient),
local_runtime_paths: Some(local_runtime_paths),
}
}
@@ -216,12 +221,14 @@ impl Environment {
) -> Self {
let client = LazyRemoteExecServerClient::new(exec_server_url.clone());
let exec_backend: Arc<dyn ExecBackend> = Arc::new(RemoteProcess::new(client.clone()));
let filesystem: Arc<dyn ExecutorFileSystem> = Arc::new(RemoteFileSystem::new(client));
let filesystem: Arc<dyn ExecutorFileSystem> =
Arc::new(RemoteFileSystem::new(client.clone()));
Self {
exec_server_url: Some(exec_server_url),
exec_backend,
filesystem,
http_client: Arc::new(client),
local_runtime_paths,
}
}
@@ -243,6 +250,10 @@ impl Environment {
Arc::clone(&self.exec_backend)
}
pub fn get_http_client(&self) -> Arc<dyn HttpClient> {
Arc::clone(&self.http_client)
}
pub fn get_filesystem(&self) -> Arc<dyn ExecutorFileSystem> {
Arc::clone(&self.filesystem)
}
+3
View File
@@ -20,7 +20,10 @@ mod server;
pub use client::ExecServerClient;
pub use client::ExecServerError;
pub use client::http_client::HttpResponseBodyStream;
pub use client::http_client::ReqwestHttpClient;
pub use client_api::ExecServerClientConnectOptions;
pub use client_api::HttpClient;
pub use client_api::RemoteExecServerConnectArgs;
pub use environment::CODEX_EXEC_SERVER_URL_ENV_VAR;
pub use environment::Environment;
+5 -5
View File
@@ -12,8 +12,8 @@ use tokio_util::sync::CancellationToken;
use tokio_util::task::TaskTracker;
use crate::ExecServerRuntimePaths;
use crate::client::http_client::ExecutorHttpRequestRunner;
use crate::client::http_client::ExecutorPendingHttpBodyStream;
use crate::client::http_client::PendingReqwestHttpBodyStream;
use crate::client::http_client::ReqwestHttpRequestRunner;
use crate::protocol::ExecParams;
use crate::protocol::ExecResponse;
use crate::protocol::FsCopyParams;
@@ -178,7 +178,7 @@ impl ExecServerHandler {
if stream_response {
self.reserve_http_body_stream(&http_request_id).await?;
}
let response = ExecutorHttpRequestRunner::new(params.timeout_ms)?
let response = ReqwestHttpRequestRunner::new(params.timeout_ms)?
.run(params)
.await;
if response.is_err() && stream_response {
@@ -306,7 +306,7 @@ impl ExecServerHandler {
async fn start_http_body_stream(
self: &Arc<Self>,
pending_stream: ExecutorPendingHttpBodyStream,
pending_stream: PendingReqwestHttpBodyStream,
) {
let request_id = pending_stream.request_id.clone();
if self.background_task_shutdown.is_cancelled() {
@@ -320,7 +320,7 @@ impl ExecServerHandler {
self.background_tasks.spawn(async move {
tokio::select! {
_ = shutdown.cancelled() => {}
_ = ExecutorHttpRequestRunner::stream_body(pending_stream, notifications) => {}
_ = ReqwestHttpRequestRunner::stream_body(pending_stream, notifications) => {}
}
handler.release_http_body_stream(&finished_request_id).await;
});
+4 -7
View File
@@ -375,7 +375,7 @@ async fn thread_start_params_include_review_policy_when_auto_review_is_enabled()
let config = ConfigBuilder::default()
.codex_home(codex_home.path().to_path_buf())
.harness_overrides(ConfigOverrides {
approvals_reviewer: Some(ApprovalsReviewer::GuardianSubagent),
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
..Default::default()
})
.fallback_cwd(Some(cwd.path().to_path_buf()))
@@ -387,7 +387,7 @@ async fn thread_start_params_include_review_policy_when_auto_review_is_enabled()
assert_eq!(
params.approvals_reviewer,
Some(codex_app_server_protocol::ApprovalsReviewer::GuardianSubagent)
Some(codex_app_server_protocol::ApprovalsReviewer::AutoReview)
);
}
@@ -419,7 +419,7 @@ fn session_configured_from_thread_response_uses_review_policy_from_response() {
cwd: test_path_buf("/tmp").abs(),
instruction_sources: Vec::new(),
approval_policy: codex_app_server_protocol::AskForApproval::OnRequest,
approvals_reviewer: codex_app_server_protocol::ApprovalsReviewer::GuardianSubagent,
approvals_reviewer: codex_app_server_protocol::ApprovalsReviewer::AutoReview,
sandbox: codex_app_server_protocol::SandboxPolicy::WorkspaceWrite {
writable_roots: vec![],
read_only_access: codex_app_server_protocol::ReadOnlyAccess::FullAccess,
@@ -440,8 +440,5 @@ fn session_configured_from_thread_response_uses_review_policy_from_response() {
let event = session_configured_from_thread_start_response(&response)
.expect("build bootstrap session configured event");
assert_eq!(
event.approvals_reviewer,
ApprovalsReviewer::GuardianSubagent
);
assert_eq!(event.approvals_reviewer, ApprovalsReviewer::AutoReview);
}
+2 -6
View File
@@ -917,12 +917,8 @@ pub const FEATURES: &[FeatureSpec] = &[
FeatureSpec {
id: Feature::GuardianApproval,
key: "guardian_approval",
stage: Stage::Experimental {
name: "Auto-review",
menu_description: "When Codex needs approval for higher-risk actions (e.g. sandbox escapes or blocked network access), route eligible approval requests to a carefully-prompted security reviewer subagent rather than blocking the agent on your input. This can consume significantly more tokens because it runs a subagent on every approval request.",
announcement: "",
},
default_enabled: false,
stage: Stage::Stable,
default_enabled: true,
},
FeatureSpec {
id: Feature::CollaborationModes,
+3 -12
View File
@@ -87,20 +87,11 @@ fn code_mode_only_requires_code_mode() {
}
#[test]
fn guardian_approval_is_experimental_and_user_toggleable() {
fn guardian_approval_is_stable_and_enabled_by_default() {
let spec = Feature::GuardianApproval.info();
let stage = spec.stage;
assert!(matches!(stage, Stage::Experimental { .. }));
assert_eq!(stage.experimental_menu_name(), Some("Auto-review"));
assert_eq!(
stage.experimental_menu_description().map(str::to_owned),
Some(
"When Codex needs approval for higher-risk actions (e.g. sandbox escapes or blocked network access), route eligible approval requests to a carefully-prompted security reviewer subagent rather than blocking the agent on your input. This can consume significantly more tokens because it runs a subagent on every approval request.".to_string()
)
);
assert_eq!(stage.experimental_announcement(), None);
assert_eq!(Feature::GuardianApproval.default_enabled(), false);
assert_eq!(spec.stage, Stage::Stable);
assert_eq!(Feature::GuardianApproval.default_enabled(), true);
}
#[test]
+31 -1
View File
@@ -109,7 +109,7 @@ pub(crate) fn matcher_pattern_for_event(
}
pub(crate) fn validate_matcher_pattern(matcher: &str) -> Result<(), regex::Error> {
if is_match_all_matcher(matcher) {
if is_match_all_matcher(matcher) || is_exact_matcher(matcher) {
return Ok(());
}
regex::Regex::new(matcher).map(|_| ())
@@ -119,6 +119,9 @@ pub(crate) fn matches_matcher(matcher: Option<&str>, input: Option<&str>) -> boo
match matcher {
None => true,
Some(matcher) if is_match_all_matcher(matcher) => true,
Some(matcher) if is_exact_matcher(matcher) => input
.map(|input| matcher.split('|').any(|candidate| candidate == input))
.unwrap_or(false),
Some(matcher) => input
.and_then(|input| {
regex::Regex::new(matcher)
@@ -144,6 +147,12 @@ fn is_match_all_matcher(matcher: &str) -> bool {
matcher.is_empty() || matcher == "*"
}
fn is_exact_matcher(matcher: &str) -> bool {
matcher
.chars()
.all(|ch| ch.is_ascii_alphanumeric() || ch == '_' || ch == '|')
}
#[cfg(test)]
mod tests {
use codex_protocol::protocol::HookEventName;
@@ -181,6 +190,27 @@ mod tests {
assert_eq!(validate_matcher_pattern("Edit|Write"), Ok(()));
}
#[test]
fn matcher_exact_string_does_not_substring_match() {
assert!(matches_matcher(Some("Bash"), Some("Bash")));
assert!(!matches_matcher(Some("Bash"), Some("BashOutput")));
assert_eq!(validate_matcher_pattern("Bash"), Ok(()));
}
#[test]
fn matcher_uses_regex_when_it_contains_other_characters() {
assert!(matches_matcher(Some("^Bash"), Some("BashOutput")));
assert!(matches_matcher(
Some("mcp__memory__.*"),
Some("mcp__memory__create_entities")
));
assert!(!matches_matcher(
Some("mcp__memory"),
Some("mcp__memory__create_entities")
));
assert_eq!(validate_matcher_pattern("mcp__memory__.*"), Ok(()));
}
#[test]
fn matcher_supports_anchored_regexes() {
assert!(matches_matcher(Some("^Bash$"), Some("Bash")));
+3 -4
View File
@@ -89,7 +89,7 @@ pub enum ApprovalsReviewer {
User,
#[serde(rename = "auto_review", alias = "guardian_subagent")]
#[strum(serialize = "auto_review")]
GuardianSubagent,
AutoReview,
}
impl JsonSchema for ApprovalsReviewer {
@@ -608,8 +608,7 @@ mod tests {
"\"user\""
);
assert_eq!(
serde_json::to_string(&ApprovalsReviewer::GuardianSubagent)
.expect("serialize reviewer"),
serde_json::to_string(&ApprovalsReviewer::AutoReview).expect("serialize reviewer"),
"\"auto_review\""
);
@@ -620,7 +619,7 @@ mod tests {
let expected = if value == "user" {
ApprovalsReviewer::User
} else {
ApprovalsReviewer::GuardianSubagent
ApprovalsReviewer::AutoReview
};
assert_eq!(expected, reviewer);
}
@@ -58,6 +58,9 @@ pub struct RequestPermissionsResponse {
pub permissions: RequestPermissionProfile,
#[serde(default)]
pub scope: PermissionGrantScope,
/// Review every subsequent command in this turn before normal sandboxed execution.
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
pub strict_auto_review: bool,
}
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)]
+3
View File
@@ -3,4 +3,7 @@ load("//:defs.bzl", "codex_rust_crate")
codex_rust_crate(
name = "rmcp-client",
crate_name = "codex_rmcp_client",
extra_binaries = [
"//codex-rs/cli:codex",
],
)
+1
View File
@@ -20,6 +20,7 @@ codex-keyring-store = { workspace = true }
codex-protocol = { workspace = true }
codex-utils-pty = { workspace = true }
codex-utils-home-dir = { workspace = true }
bytes = { workspace = true }
futures = { workspace = true, default-features = false, features = ["std"] }
keyring = { workspace = true, features = ["crypto-rust"] }
oauth2 = "5"
@@ -0,0 +1,391 @@
//! RMCP Streamable HTTP adapter built on top of the shared `HttpClient`
//! capability.
//!
//! This module runs in the orchestrator process. It turns high-level RMCP
//! operations like `post_message` and `get_stream` into calls on
//! `Arc<dyn HttpClient>`, which may be:
//! - a local HTTP client that issues requests from the orchestrator, or
//! - a remote HTTP client that forwards requests to the remote runtime
use std::io;
use std::sync::Arc;
use bytes::Bytes;
use codex_exec_server::ExecServerError;
use codex_exec_server::HttpClient;
use codex_exec_server::HttpHeader;
use codex_exec_server::HttpRequestParams;
use codex_exec_server::HttpResponseBodyStream;
use futures::StreamExt;
use futures::stream;
use futures::stream::BoxStream;
use reqwest::StatusCode;
use reqwest::header::ACCEPT;
use reqwest::header::AUTHORIZATION;
use reqwest::header::CONTENT_TYPE;
use reqwest::header::HeaderMap;
use reqwest::header::HeaderName;
use rmcp::model::ClientJsonRpcMessage;
use rmcp::model::ServerJsonRpcMessage;
use rmcp::transport::streamable_http_client::AuthRequiredError;
use rmcp::transport::streamable_http_client::StreamableHttpClient;
use rmcp::transport::streamable_http_client::StreamableHttpError;
use rmcp::transport::streamable_http_client::StreamableHttpPostResponse;
use sse_stream::Sse;
use sse_stream::SseStream;
const EVENT_STREAM_MIME_TYPE: &str = "text/event-stream";
const JSON_MIME_TYPE: &str = "application/json";
const HEADER_SESSION_ID: &str = "Mcp-Session-Id";
const NON_JSON_RESPONSE_BODY_PREVIEW_BYTES: usize = 8_192;
#[derive(Clone)]
pub(crate) struct StreamableHttpClientAdapter {
http_client: Arc<dyn HttpClient>,
default_headers: HeaderMap,
}
#[derive(Debug, thiserror::Error)]
pub(crate) enum StreamableHttpClientAdapterError {
#[error("streamable HTTP session expired with 404 Not Found")]
SessionExpired404,
#[error(transparent)]
HttpRequest(#[from] ExecServerError),
#[error("invalid HTTP header: {0}")]
Header(String),
}
impl StreamableHttpClientAdapter {
pub(crate) fn new(http_client: Arc<dyn HttpClient>, default_headers: HeaderMap) -> Self {
Self {
http_client,
default_headers,
}
}
}
impl StreamableHttpClient for StreamableHttpClientAdapter {
type Error = StreamableHttpClientAdapterError;
async fn post_message(
&self,
uri: Arc<str>,
message: ClientJsonRpcMessage,
session_id: Option<Arc<str>>,
auth_token: Option<String>,
) -> std::result::Result<StreamableHttpPostResponse, StreamableHttpError<Self::Error>> {
let mut headers = self.default_headers.clone();
insert_header(
&mut headers,
ACCEPT,
[EVENT_STREAM_MIME_TYPE, JSON_MIME_TYPE].join(", "),
StreamableHttpClientAdapterError::Header,
)?;
insert_header(
&mut headers,
CONTENT_TYPE,
JSON_MIME_TYPE.to_string(),
StreamableHttpClientAdapterError::Header,
)?;
if let Some(auth_token) = auth_token {
insert_header(
&mut headers,
AUTHORIZATION,
format!("Bearer {auth_token}"),
StreamableHttpClientAdapterError::Header,
)?;
}
if let Some(session_id_value) = session_id.as_ref() {
insert_header(
&mut headers,
HeaderName::from_static("mcp-session-id"),
session_id_value.to_string(),
StreamableHttpClientAdapterError::Header,
)?;
}
let body = serde_json::to_vec(&message).map_err(StreamableHttpError::Deserialize)?;
let (response, mut body_stream) = self
.http_client
.http_request_stream(HttpRequestParams {
method: "POST".to_string(),
url: uri.to_string(),
headers: protocol_headers(&headers),
body: Some(body.into()),
timeout_ms: None,
request_id: "buffered-request".to_string(),
stream_response: true,
})
.await
.map_err(StreamableHttpClientAdapterError::from)
.map_err(StreamableHttpError::Client)?;
if response.status == StatusCode::NOT_FOUND.as_u16() && session_id.is_some() {
return Err(StreamableHttpError::Client(
StreamableHttpClientAdapterError::SessionExpired404,
));
}
if response.status == StatusCode::UNAUTHORIZED.as_u16()
&& let Some(header) =
response_header(&response.headers, reqwest::header::WWW_AUTHENTICATE)
{
return Err(StreamableHttpError::AuthRequired(AuthRequiredError {
www_authenticate_header: header,
}));
}
if matches!(
StatusCode::from_u16(response.status).ok(),
Some(StatusCode::ACCEPTED | StatusCode::NO_CONTENT)
) {
return Ok(StreamableHttpPostResponse::Accepted);
}
let content_type = response_header(&response.headers, CONTENT_TYPE);
let session_id = response_header(&response.headers, HEADER_SESSION_ID);
match content_type.as_deref() {
Some(content_type) if content_type.starts_with(EVENT_STREAM_MIME_TYPE) => {
let event_stream = sse_stream_from_body(body_stream);
Ok(StreamableHttpPostResponse::Sse(event_stream, session_id))
}
Some(content_type) if content_type.starts_with(JSON_MIME_TYPE) => {
let body = collect_body(&mut body_stream).await?;
let message: ServerJsonRpcMessage =
serde_json::from_slice(&body).map_err(StreamableHttpError::Deserialize)?;
Ok(StreamableHttpPostResponse::Json(message, session_id))
}
_ => {
let body = collect_body(&mut body_stream).await?;
let content_type = content_type.unwrap_or_else(|| "missing-content-type".into());
Err(StreamableHttpError::UnexpectedContentType(Some(format!(
"{content_type}; body: {}",
body_preview(String::from_utf8_lossy(&body).to_string())
))))
}
}
}
async fn delete_session(
&self,
uri: Arc<str>,
session: Arc<str>,
auth_token: Option<String>,
) -> std::result::Result<(), StreamableHttpError<Self::Error>> {
let mut headers = self.default_headers.clone();
if let Some(auth_token) = auth_token {
insert_header(
&mut headers,
AUTHORIZATION,
format!("Bearer {auth_token}"),
StreamableHttpClientAdapterError::Header,
)?;
}
insert_header(
&mut headers,
HeaderName::from_static("mcp-session-id"),
session.to_string(),
StreamableHttpClientAdapterError::Header,
)?;
let response = self
.http_client
.http_request(HttpRequestParams {
method: "DELETE".to_string(),
url: uri.to_string(),
headers: protocol_headers(&headers),
body: None,
timeout_ms: None,
request_id: "buffered-request".to_string(),
stream_response: false,
})
.await
.map_err(StreamableHttpClientAdapterError::from)
.map_err(StreamableHttpError::Client)?;
if response.status == StatusCode::METHOD_NOT_ALLOWED.as_u16() {
return Ok(());
}
if !status_is_success(response.status) {
return Err(StreamableHttpError::UnexpectedServerResponse(
format!("DELETE returned HTTP {}", response.status).into(),
));
}
Ok(())
}
async fn get_stream(
&self,
uri: Arc<str>,
session_id: Arc<str>,
last_event_id: Option<String>,
auth_token: Option<String>,
) -> std::result::Result<
BoxStream<'static, std::result::Result<Sse, sse_stream::Error>>,
StreamableHttpError<Self::Error>,
> {
let mut headers = self.default_headers.clone();
insert_header(
&mut headers,
ACCEPT,
[EVENT_STREAM_MIME_TYPE, JSON_MIME_TYPE].join(", "),
StreamableHttpClientAdapterError::Header,
)?;
insert_header(
&mut headers,
HeaderName::from_static("mcp-session-id"),
session_id.to_string(),
StreamableHttpClientAdapterError::Header,
)?;
if let Some(last_event_id) = last_event_id {
insert_header(
&mut headers,
HeaderName::from_static("last-event-id"),
last_event_id,
StreamableHttpClientAdapterError::Header,
)?;
}
if let Some(auth_token) = auth_token {
insert_header(
&mut headers,
AUTHORIZATION,
format!("Bearer {auth_token}"),
StreamableHttpClientAdapterError::Header,
)?;
}
let (response, body_stream) = self
.http_client
.http_request_stream(HttpRequestParams {
method: "GET".to_string(),
url: uri.to_string(),
headers: protocol_headers(&headers),
body: None,
timeout_ms: None,
request_id: "buffered-request".to_string(),
stream_response: true,
})
.await
.map_err(StreamableHttpClientAdapterError::from)
.map_err(StreamableHttpError::Client)?;
if response.status == StatusCode::METHOD_NOT_ALLOWED.as_u16() {
return Err(StreamableHttpError::ServerDoesNotSupportSse);
}
if response.status == StatusCode::NOT_FOUND.as_u16() {
return Err(StreamableHttpError::Client(
StreamableHttpClientAdapterError::SessionExpired404,
));
}
if !status_is_success(response.status) {
return Err(StreamableHttpError::UnexpectedServerResponse(
format!("GET returned HTTP {}", response.status).into(),
));
}
match response_header(&response.headers, CONTENT_TYPE).as_deref() {
Some(content_type) if is_streamable_http_content_type(content_type) => {}
Some(content_type) => {
return Err(StreamableHttpError::UnexpectedContentType(Some(
content_type.to_string(),
)));
}
None => {
return Err(StreamableHttpError::UnexpectedContentType(None));
}
}
Ok(sse_stream_from_body(body_stream))
}
}
fn body_preview(body: impl Into<String>) -> String {
let mut body_preview = body.into();
let body_len = body_preview.len();
if body_len > NON_JSON_RESPONSE_BODY_PREVIEW_BYTES {
let mut boundary = NON_JSON_RESPONSE_BODY_PREVIEW_BYTES;
while !body_preview.is_char_boundary(boundary) {
boundary = boundary.saturating_sub(1);
}
body_preview.truncate(boundary);
body_preview.push_str(&format!(
"... (truncated {} bytes)",
body_len.saturating_sub(boundary)
));
}
body_preview
}
fn insert_header<Error>(
headers: &mut HeaderMap,
name: HeaderName,
value: String,
map_error: impl FnOnce(String) -> Error,
) -> std::result::Result<(), StreamableHttpError<Error>>
where
Error: std::error::Error + Send + Sync + 'static,
{
let value = reqwest::header::HeaderValue::from_str(&value)
.map_err(|error| StreamableHttpError::Client(map_error(error.to_string())))?;
headers.insert(name, value);
Ok(())
}
fn is_streamable_http_content_type(content_type: &str) -> bool {
content_type
.as_bytes()
.starts_with(EVENT_STREAM_MIME_TYPE.as_bytes())
|| content_type
.as_bytes()
.starts_with(JSON_MIME_TYPE.as_bytes())
}
fn protocol_headers(headers: &HeaderMap) -> Vec<HttpHeader> {
headers
.iter()
.filter_map(|(name, value)| {
Some(HttpHeader {
name: name.as_str().to_string(),
value: value.to_str().ok()?.to_string(),
})
})
.collect()
}
fn response_header(headers: &[HttpHeader], name: impl AsRef<str>) -> Option<String> {
let name = name.as_ref();
headers
.iter()
.find(|header| header.name.eq_ignore_ascii_case(name))
.map(|header| header.value.clone())
}
fn status_is_success(status: u16) -> bool {
StatusCode::from_u16(status).is_ok_and(|status| status.is_success())
}
async fn collect_body(
body_stream: &mut HttpResponseBodyStream,
) -> std::result::Result<Vec<u8>, StreamableHttpError<StreamableHttpClientAdapterError>> {
let mut body = Vec::new();
while let Some(chunk) = body_stream
.recv()
.await
.map_err(StreamableHttpClientAdapterError::from)
.map_err(StreamableHttpError::Client)?
{
body.extend_from_slice(&chunk);
}
Ok(body)
}
fn sse_stream_from_body(
body_stream: HttpResponseBodyStream,
) -> BoxStream<'static, std::result::Result<Sse, sse_stream::Error>> {
SseStream::from_byte_stream(stream::unfold(body_stream, |mut body_stream| async move {
match body_stream.recv().await {
Ok(Some(bytes)) => Some((Ok(Bytes::from(bytes)), body_stream)),
Ok(None) => None,
Err(error) => Some((Err(io::Error::other(error)), body_stream)),
}
}))
.boxed()
}
+1
View File
@@ -1,6 +1,7 @@
mod auth_status;
mod elicitation_client_service;
mod executor_process_transport;
mod http_client_adapter;
mod logging_client_handler;
mod oauth;
mod perform_oauth_login;
+13 -8
View File
@@ -12,6 +12,7 @@
use std::collections::HashMap;
use std::ffi::OsString;
use std::path::Path;
/// Resolves a program to its executable path on Unix systems.
///
@@ -19,7 +20,11 @@ use std::ffi::OsString;
/// the kernel's shebang (`#!`) mechanism, so this function simply returns
/// the program name unchanged.
#[cfg(unix)]
pub fn resolve(program: OsString, _env: &HashMap<OsString, OsString>) -> std::io::Result<OsString> {
pub fn resolve(
program: OsString,
_env: &HashMap<OsString, OsString>,
_cwd: &Path,
) -> std::io::Result<OsString> {
Ok(program)
}
@@ -33,16 +38,16 @@ pub fn resolve(program: OsString, _env: &HashMap<OsString, OsString>) -> std::io
/// This enables tools like `npx`, `pnpm`, and `yarn` to work correctly on Windows
/// without requiring users to specify full paths or extensions in their configuration.
#[cfg(windows)]
pub fn resolve(program: OsString, env: &HashMap<OsString, OsString>) -> std::io::Result<OsString> {
// Get current directory for relative path resolution
let cwd = std::env::current_dir()
.map_err(|e| std::io::Error::other(format!("Failed to get current directory: {e}")))?;
pub fn resolve(
program: OsString,
env: &HashMap<OsString, OsString>,
cwd: &Path,
) -> std::io::Result<OsString> {
// Extract PATH from environment for search locations
let search_path = env.get(std::ffi::OsStr::new("PATH"));
// Attempt resolution via which crate
match which::which_in(&program, search_path, &cwd) {
match which::which_in(&program, search_path, cwd) {
Ok(resolved) => {
tracing::debug!("Resolved {program:?} to {resolved:?}");
Ok(resolved.into_os_string())
@@ -119,7 +124,7 @@ mod tests {
let program = OsString::from(&env.program_name);
// Apply platform-specific resolution
let resolved = resolve(program, &env.mcp_env)?;
let resolved = resolve(program, &env.mcp_env, std::env::current_dir()?.as_path())?;
// Verify resolved path executes successfully
let mut cmd = Command::new(resolved);
+30 -248
View File
@@ -1,4 +1,3 @@
use std::borrow::Cow;
use std::collections::HashMap;
use std::ffi::OsString;
use std::future::Future;
@@ -14,16 +13,12 @@ use anyhow::Result;
use anyhow::anyhow;
use codex_client::build_reqwest_client_with_custom_ca;
use codex_config::types::McpServerEnvVar;
use codex_exec_server::HttpClient;
use futures::FutureExt;
use futures::StreamExt;
use futures::future::BoxFuture;
use futures::stream::BoxStream;
use oauth2::TokenResponse;
use reqwest::header::ACCEPT;
use reqwest::header::AUTHORIZATION;
use reqwest::header::CONTENT_TYPE;
use reqwest::header::HeaderMap;
use reqwest::header::WWW_AUTHENTICATE;
use rmcp::model::CallToolRequestParams;
use rmcp::model::CallToolResult;
use rmcp::model::ClientNotification;
@@ -52,16 +47,11 @@ use rmcp::transport::StreamableHttpClientTransport;
use rmcp::transport::auth::AuthClient;
use rmcp::transport::auth::AuthError;
use rmcp::transport::auth::OAuthState;
use rmcp::transport::streamable_http_client::AuthRequiredError;
use rmcp::transport::streamable_http_client::StreamableHttpClient;
use rmcp::transport::streamable_http_client::StreamableHttpClientTransportConfig;
use rmcp::transport::streamable_http_client::StreamableHttpError;
use rmcp::transport::streamable_http_client::StreamableHttpPostResponse;
use serde::Deserialize;
use serde::Serialize;
use serde_json::Value;
use sse_stream::Sse;
use sse_stream::SseStream;
use tokio::sync::Mutex;
use tokio::sync::Semaphore;
use tokio::sync::watch;
@@ -69,6 +59,8 @@ use tokio::time;
use tracing::warn;
use crate::elicitation_client_service::ElicitationClientService;
use crate::http_client_adapter::StreamableHttpClientAdapter;
use crate::http_client_adapter::StreamableHttpClientAdapterError;
use crate::load_oauth_tokens;
use crate::oauth::OAuthPersistor;
use crate::oauth::StoredOAuthTokens;
@@ -79,239 +71,15 @@ use crate::utils::apply_default_headers;
use crate::utils::build_default_headers;
use codex_config::types::OAuthCredentialsStoreMode;
const EVENT_STREAM_MIME_TYPE: &str = "text/event-stream";
const JSON_MIME_TYPE: &str = "application/json";
const HEADER_LAST_EVENT_ID: &str = "Last-Event-Id";
const HEADER_SESSION_ID: &str = "Mcp-Session-Id";
const NON_JSON_RESPONSE_BODY_PREVIEW_BYTES: usize = 8_192;
#[derive(Clone)]
struct StreamableHttpResponseClient {
inner: reqwest::Client,
}
impl StreamableHttpResponseClient {
fn new(inner: reqwest::Client) -> Self {
Self { inner }
}
fn reqwest_error(
error: reqwest::Error,
) -> StreamableHttpError<StreamableHttpResponseClientError> {
StreamableHttpError::Client(StreamableHttpResponseClientError::from(error))
}
}
fn build_http_client(default_headers: &HeaderMap) -> Result<reqwest::Client> {
let builder = apply_default_headers(reqwest::Client::builder(), default_headers);
Ok(build_reqwest_client_with_custom_ca(builder)?)
}
#[derive(Debug, thiserror::Error)]
enum StreamableHttpResponseClientError {
#[error("streamable HTTP session expired with 404 Not Found")]
SessionExpired404,
#[error(transparent)]
Reqwest(#[from] reqwest::Error),
}
impl StreamableHttpClient for StreamableHttpResponseClient {
type Error = StreamableHttpResponseClientError;
async fn post_message(
&self,
uri: Arc<str>,
message: rmcp::model::ClientJsonRpcMessage,
session_id: Option<Arc<str>>,
auth_token: Option<String>,
) -> std::result::Result<StreamableHttpPostResponse, StreamableHttpError<Self::Error>> {
let mut request = self
.inner
.post(uri.as_ref())
.header(ACCEPT, [EVENT_STREAM_MIME_TYPE, JSON_MIME_TYPE].join(", "));
if let Some(auth_header) = auth_token {
request = request.bearer_auth(auth_header);
}
if let Some(session_id_value) = session_id.as_ref() {
request = request.header(HEADER_SESSION_ID, session_id_value.as_ref());
}
let response = request
.json(&message)
.send()
.await
.map_err(StreamableHttpResponseClient::reqwest_error)?;
if response.status() == reqwest::StatusCode::NOT_FOUND && session_id.is_some() {
return Err(StreamableHttpError::Client(
StreamableHttpResponseClientError::SessionExpired404,
));
}
if response.status() == reqwest::StatusCode::UNAUTHORIZED
&& let Some(header) = response.headers().get(WWW_AUTHENTICATE)
{
let header = header
.to_str()
.map_err(|_| {
StreamableHttpError::UnexpectedServerResponse(Cow::Borrowed(
"invalid www-authenticate header value",
))
})?
.to_string();
return Err(StreamableHttpError::AuthRequired(AuthRequiredError {
www_authenticate_header: header,
}));
}
let status = response.status();
if matches!(
status,
reqwest::StatusCode::ACCEPTED | reqwest::StatusCode::NO_CONTENT
) {
return Ok(StreamableHttpPostResponse::Accepted);
}
let content_type = response
.headers()
.get(CONTENT_TYPE)
.and_then(|value| value.to_str().ok())
.map(str::to_string);
let session_id = response
.headers()
.get(HEADER_SESSION_ID)
.and_then(|value| value.to_str().ok())
.map(str::to_string);
match content_type.as_deref() {
Some(ct) if ct.as_bytes().starts_with(EVENT_STREAM_MIME_TYPE.as_bytes()) => {
let event_stream = SseStream::from_byte_stream(response.bytes_stream()).boxed();
Ok(StreamableHttpPostResponse::Sse(event_stream, session_id))
}
Some(ct) if ct.as_bytes().starts_with(JSON_MIME_TYPE.as_bytes()) => {
let message = response
.json()
.await
.map_err(StreamableHttpResponseClient::reqwest_error)?;
Ok(StreamableHttpPostResponse::Json(message, session_id))
}
_ => {
let body = response
.text()
.await
.map_err(StreamableHttpResponseClient::reqwest_error)?;
let mut body_preview = body;
let body_len = body_preview.len();
if body_len > NON_JSON_RESPONSE_BODY_PREVIEW_BYTES {
let mut boundary = NON_JSON_RESPONSE_BODY_PREVIEW_BYTES;
while !body_preview.is_char_boundary(boundary) {
boundary = boundary.saturating_sub(1);
}
body_preview.truncate(boundary);
body_preview.push_str(&format!(
"... (truncated {} bytes)",
body_len.saturating_sub(boundary)
));
}
let content_type = content_type.unwrap_or_else(|| "missing-content-type".into());
Err(StreamableHttpError::UnexpectedContentType(Some(format!(
"{content_type}; body: {body_preview}"
))))
}
}
}
async fn delete_session(
&self,
uri: Arc<str>,
session: Arc<str>,
auth_token: Option<String>,
) -> std::result::Result<(), StreamableHttpError<Self::Error>> {
let mut request_builder = self.inner.delete(uri.as_ref());
if let Some(auth_header) = auth_token {
request_builder = request_builder.bearer_auth(auth_header);
}
let response = request_builder
.header(HEADER_SESSION_ID, session.as_ref())
.send()
.await
.map_err(StreamableHttpResponseClient::reqwest_error)?;
if response.status() == reqwest::StatusCode::METHOD_NOT_ALLOWED {
return Ok(());
}
response
.error_for_status()
.map_err(StreamableHttpResponseClient::reqwest_error)?;
Ok(())
}
async fn get_stream(
&self,
uri: Arc<str>,
session_id: Arc<str>,
last_event_id: Option<String>,
auth_token: Option<String>,
) -> std::result::Result<
BoxStream<'static, std::result::Result<Sse, sse_stream::Error>>,
StreamableHttpError<Self::Error>,
> {
let mut request_builder = self
.inner
.get(uri.as_ref())
.header(ACCEPT, [EVENT_STREAM_MIME_TYPE, JSON_MIME_TYPE].join(", "))
.header(HEADER_SESSION_ID, session_id.as_ref());
if let Some(last_event_id) = last_event_id {
request_builder = request_builder.header(HEADER_LAST_EVENT_ID, last_event_id);
}
if let Some(auth_header) = auth_token {
request_builder = request_builder.bearer_auth(auth_header);
}
let response = request_builder
.send()
.await
.map_err(StreamableHttpResponseClient::reqwest_error)?;
if response.status() == reqwest::StatusCode::METHOD_NOT_ALLOWED {
return Err(StreamableHttpError::ServerDoesNotSupportSse);
}
if response.status() == reqwest::StatusCode::NOT_FOUND {
return Err(StreamableHttpError::Client(
StreamableHttpResponseClientError::SessionExpired404,
));
}
let response = response
.error_for_status()
.map_err(StreamableHttpResponseClient::reqwest_error)?;
match response.headers().get(CONTENT_TYPE) {
Some(ct)
if ct.as_bytes().starts_with(EVENT_STREAM_MIME_TYPE.as_bytes())
|| ct.as_bytes().starts_with(JSON_MIME_TYPE.as_bytes()) => {}
Some(ct) => {
return Err(StreamableHttpError::UnexpectedContentType(Some(
String::from_utf8_lossy(ct.as_bytes()).to_string(),
)));
}
None => {
return Err(StreamableHttpError::UnexpectedContentType(None));
}
}
let event_stream = SseStream::from_byte_stream(response.bytes_stream()).boxed();
Ok(event_stream)
}
}
enum PendingTransport {
Stdio {
transport: StdioServerTransport,
},
StreamableHttp {
transport: StreamableHttpClientTransport<StreamableHttpResponseClient>,
transport: StreamableHttpClientTransport<StreamableHttpClientAdapter>,
},
StreamableHttpWithOAuth {
transport: StreamableHttpClientTransport<AuthClient<StreamableHttpResponseClient>>,
transport: StreamableHttpClientTransport<AuthClient<StreamableHttpClientAdapter>>,
oauth_persistor: OAuthPersistor,
},
}
@@ -339,6 +107,7 @@ enum TransportRecipe {
http_headers: Option<HashMap<String, String>>,
env_http_headers: Option<HashMap<String, String>>,
store_mode: OAuthCredentialsStoreMode,
http_client: Arc<dyn HttpClient>,
},
}
@@ -536,6 +305,7 @@ impl RmcpClient {
http_headers: Option<HashMap<String, String>>,
env_http_headers: Option<HashMap<String, String>>,
store_mode: OAuthCredentialsStoreMode,
http_client: Arc<dyn HttpClient>,
) -> Result<Self> {
let transport_recipe = TransportRecipe::StreamableHttp {
server_name: server_name.to_string(),
@@ -544,6 +314,7 @@ impl RmcpClient {
http_headers,
env_http_headers,
store_mode,
http_client,
};
let transport = Self::create_pending_transport(&transport_recipe).await?;
Ok(Self {
@@ -895,6 +666,7 @@ impl RmcpClient {
http_headers,
env_http_headers,
store_mode,
http_client,
} => {
let default_headers =
build_default_headers(http_headers.clone(), env_http_headers.clone())?;
@@ -919,6 +691,7 @@ impl RmcpClient {
initial_tokens.clone(),
*store_mode,
default_headers.clone(),
Arc::clone(http_client),
)
.await
{
@@ -945,9 +718,11 @@ impl RmcpClient {
let http_config =
StreamableHttpClientTransportConfig::with_uri(url.clone())
.auth_header(access_token);
let http_client = build_http_client(&default_headers)?;
let transport = StreamableHttpClientTransport::with_client(
StreamableHttpResponseClient::new(http_client),
StreamableHttpClientAdapter::new(
Arc::clone(http_client),
default_headers,
),
http_config,
);
Ok(PendingTransport::StreamableHttp { transport })
@@ -961,10 +736,8 @@ impl RmcpClient {
http_config = http_config.auth_header(bearer_token);
}
let http_client = build_http_client(&default_headers)?;
let transport = StreamableHttpClientTransport::with_client(
StreamableHttpResponseClient::new(http_client),
StreamableHttpClientAdapter::new(Arc::clone(http_client), default_headers),
http_config,
);
Ok(PendingTransport::StreamableHttp { transport })
@@ -1084,12 +857,12 @@ impl RmcpClient {
error
.error
.downcast_ref::<StreamableHttpError<StreamableHttpResponseClientError>>()
.downcast_ref::<StreamableHttpError<StreamableHttpClientAdapterError>>()
.is_some_and(|error| {
matches!(
error,
StreamableHttpError::Client(
StreamableHttpResponseClientError::SessionExpired404
StreamableHttpClientAdapterError::SessionExpired404
)
)
})
@@ -1156,12 +929,18 @@ async fn create_oauth_transport_and_runtime(
initial_tokens: StoredOAuthTokens,
credentials_store: OAuthCredentialsStoreMode,
default_headers: HeaderMap,
http_client: Arc<dyn HttpClient>,
) -> Result<(
StreamableHttpClientTransport<AuthClient<StreamableHttpResponseClient>>,
StreamableHttpClientTransport<AuthClient<StreamableHttpClientAdapter>>,
OAuthPersistor,
)> {
let http_client = build_http_client(&default_headers)?;
let mut oauth_state = OAuthState::new(url.to_string(), Some(http_client.clone())).await?;
let builder = apply_default_headers(reqwest::Client::builder(), &default_headers);
let oauth_metadata_client = build_reqwest_client_with_custom_ca(builder)?;
// TODO(aibrahim): teach OAuth bootstrap and refresh to use the same
// shared HTTP client abstraction instead of always creating the local
// reqwest metadata client here.
let mut oauth_state =
OAuthState::new(url.to_string(), Some(oauth_metadata_client.clone())).await?;
oauth_state
.set_credentials(
@@ -1178,7 +957,10 @@ async fn create_oauth_transport_and_runtime(
}
};
let auth_client = AuthClient::new(StreamableHttpResponseClient::new(http_client), manager);
let auth_client = AuthClient::new(
StreamableHttpClientAdapter::new(http_client, default_headers),
manager,
);
let auth_manager = auth_client.auth_manager.clone();
let transport = StreamableHttpClientTransport::with_client(
@@ -162,14 +162,27 @@ impl StdioServerCommand {
/// process spawns the configured command and rmcp talks to the child's local
/// stdin/stdout pipes directly.
#[derive(Clone)]
pub struct LocalStdioServerLauncher;
pub struct LocalStdioServerLauncher {
fallback_cwd: PathBuf,
}
impl LocalStdioServerLauncher {
/// Creates a local stdio launcher.
///
/// `fallback_cwd` is used when the MCP server config omits `cwd`, so
/// relative commands resolve from the caller's runtime working directory.
pub fn new(fallback_cwd: PathBuf) -> Self {
Self { fallback_cwd }
}
}
impl StdioServerLauncher for LocalStdioServerLauncher {
fn launch(
&self,
command: StdioServerCommand,
) -> BoxFuture<'static, io::Result<StdioServerTransport>> {
async move { Self::launch_server(command) }.boxed()
let fallback_cwd = self.fallback_cwd.clone();
async move { Self::launch_server(command, fallback_cwd) }.boxed()
}
}
@@ -193,7 +206,10 @@ mod private {
impl private::Sealed for LocalStdioServerLauncher {}
impl LocalStdioServerLauncher {
fn launch_server(command: StdioServerCommand) -> io::Result<StdioServerTransport> {
fn launch_server(
command: StdioServerCommand,
fallback_cwd: PathBuf,
) -> io::Result<StdioServerTransport> {
let StdioServerCommand {
program,
args,
@@ -203,22 +219,21 @@ impl LocalStdioServerLauncher {
} = command;
let program_name = program.to_string_lossy().into_owned();
let envs = create_env_for_mcp_server(env, &env_vars).map_err(io::Error::other)?;
let cwd = cwd.unwrap_or(fallback_cwd);
let resolved_program =
program_resolver::resolve(program, &envs).map_err(io::Error::other)?;
program_resolver::resolve(program, &envs, &cwd).map_err(io::Error::other)?;
let mut command = Command::new(resolved_program);
command
.kill_on_drop(true)
.stdin(Stdio::piped())
.stdout(Stdio::piped())
.current_dir(cwd)
.env_clear()
.envs(envs)
.args(args);
#[cfg(unix)]
command.process_group(0);
if let Some(cwd) = cwd {
command.current_dir(cwd);
}
let (transport, stderr) = TokioChildProcess::builder(command)
.stderr(Stdio::piped())
@@ -80,7 +80,7 @@ async fn drop_kills_wrapper_process_group() -> Result<()> {
)])),
&[],
/*cwd*/ None,
Arc::new(LocalStdioServerLauncher),
Arc::new(LocalStdioServerLauncher::new(std::env::current_dir()?)),
)
.await?;
+1 -1
View File
@@ -63,7 +63,7 @@ async fn rmcp_client_can_list_and_read_resources() -> anyhow::Result<()> {
/*env*/ None,
&[],
/*cwd*/ None,
Arc::new(LocalStdioServerLauncher),
Arc::new(LocalStdioServerLauncher::new(std::env::current_dir()?)),
)
.await?;
@@ -1,190 +1,12 @@
use std::net::TcpListener;
use std::path::PathBuf;
use std::time::Duration;
use std::time::Instant;
mod streamable_http_test_support;
use codex_config::types::OAuthCredentialsStoreMode;
use codex_rmcp_client::ElicitationAction;
use codex_rmcp_client::ElicitationResponse;
use codex_rmcp_client::RmcpClient;
use codex_utils_cargo_bin::CargoBinError;
use futures::FutureExt as _;
use pretty_assertions::assert_eq;
use rmcp::model::CallToolResult;
use rmcp::model::ClientCapabilities;
use rmcp::model::ElicitationCapability;
use rmcp::model::FormElicitationCapability;
use rmcp::model::Implementation;
use rmcp::model::InitializeRequestParams;
use rmcp::model::ProtocolVersion;
use serde_json::json;
use tokio::net::TcpStream;
use tokio::process::Child;
use tokio::process::Command;
use tokio::time::sleep;
const SESSION_POST_FAILURE_CONTROL_PATH: &str = "/test/control/session-post-failure";
fn streamable_http_server_bin() -> Result<PathBuf, CargoBinError> {
codex_utils_cargo_bin::cargo_bin("test_streamable_http_server")
}
fn init_params() -> InitializeRequestParams {
InitializeRequestParams {
meta: None,
capabilities: ClientCapabilities {
experimental: None,
extensions: None,
roots: None,
sampling: None,
elicitation: Some(ElicitationCapability {
form: Some(FormElicitationCapability {
schema_validation: None,
}),
url: None,
}),
tasks: None,
},
client_info: Implementation {
name: "codex-test".into(),
version: "0.0.0-test".into(),
title: Some("Codex rmcp recovery test".into()),
description: None,
icons: None,
website_url: None,
},
protocol_version: ProtocolVersion::V_2025_06_18,
}
}
fn expected_echo_result(message: &str) -> CallToolResult {
CallToolResult {
content: Vec::new(),
structured_content: Some(json!({
"echo": format!("ECHOING: {message}"),
"env": null,
})),
is_error: Some(false),
meta: None,
}
}
async fn create_client(base_url: &str) -> anyhow::Result<RmcpClient> {
let client = RmcpClient::new_streamable_http_client(
"test-streamable-http",
&format!("{base_url}/mcp"),
Some("test-bearer".to_string()),
/*http_headers*/ None,
/*env_http_headers*/ None,
OAuthCredentialsStoreMode::File,
)
.await?;
client
.initialize(
init_params(),
Some(Duration::from_secs(5)),
Box::new(|_, _| {
async {
Ok(ElicitationResponse {
action: ElicitationAction::Accept,
content: Some(json!({})),
meta: None,
})
}
.boxed()
}),
)
.await?;
Ok(client)
}
async fn call_echo_tool(client: &RmcpClient, message: &str) -> anyhow::Result<CallToolResult> {
client
.call_tool(
"echo".to_string(),
Some(json!({ "message": message })),
/*meta*/ None,
Some(Duration::from_secs(5)),
)
.await
}
async fn arm_session_post_failure(
base_url: &str,
status: u16,
remaining: usize,
) -> anyhow::Result<()> {
let response = reqwest::Client::new()
.post(format!("{base_url}{SESSION_POST_FAILURE_CONTROL_PATH}"))
.json(&json!({
"status": status,
"remaining": remaining,
}))
.send()
.await?;
assert_eq!(response.status(), reqwest::StatusCode::NO_CONTENT);
Ok(())
}
async fn spawn_streamable_http_server() -> anyhow::Result<(Child, String)> {
let listener = TcpListener::bind("127.0.0.1:0")?;
let port = listener.local_addr()?.port();
drop(listener);
let bind_addr = format!("127.0.0.1:{port}");
let base_url = format!("http://{bind_addr}");
let mut child = Command::new(streamable_http_server_bin()?)
.kill_on_drop(true)
.env("MCP_STREAMABLE_HTTP_BIND_ADDR", &bind_addr)
.spawn()?;
wait_for_streamable_http_server(&mut child, &bind_addr, Duration::from_secs(5)).await?;
Ok((child, base_url))
}
async fn wait_for_streamable_http_server(
server_child: &mut Child,
address: &str,
timeout: Duration,
) -> anyhow::Result<()> {
let deadline = Instant::now() + timeout;
loop {
if let Some(status) = server_child.try_wait()? {
return Err(anyhow::anyhow!(
"streamable HTTP server exited early with status {status}"
));
}
let remaining = deadline.saturating_duration_since(Instant::now());
if remaining.is_zero() {
return Err(anyhow::anyhow!(
"timed out waiting for streamable HTTP server at {address}: deadline reached"
));
}
match tokio::time::timeout(remaining, TcpStream::connect(address)).await {
Ok(Ok(_)) => return Ok(()),
Ok(Err(error)) => {
if Instant::now() >= deadline {
return Err(anyhow::anyhow!(
"timed out waiting for streamable HTTP server at {address}: {error}"
));
}
}
Err(_) => {
return Err(anyhow::anyhow!(
"timed out waiting for streamable HTTP server at {address}: connect call timed out"
));
}
}
sleep(Duration::from_millis(50)).await;
}
}
use streamable_http_test_support::arm_session_post_failure;
use streamable_http_test_support::call_echo_tool;
use streamable_http_test_support::create_client;
use streamable_http_test_support::expected_echo_result;
use streamable_http_test_support::spawn_streamable_http_server;
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
async fn streamable_http_404_session_expiry_recovers_and_retries_once() -> anyhow::Result<()> {
@@ -0,0 +1,37 @@
//! Integration coverage for the remote Streamable HTTP RMCP path.
//!
//! These tests exercise the orchestrator-side RMCP adapter against a real
//! `exec-server` process so HTTP requests go through the remote runtime path
//! instead of direct local `reqwest` calls.
mod streamable_http_test_support;
use pretty_assertions::assert_eq;
use streamable_http_test_support::call_echo_tool;
use streamable_http_test_support::create_remote_client;
use streamable_http_test_support::expected_echo_result;
use streamable_http_test_support::spawn_exec_server;
use streamable_http_test_support::spawn_streamable_http_server;
/// What this tests: the RMCP remote Streamable HTTP adapter can initialize
/// a server and call a tool while every MCP HTTP request goes through a real
/// exec-server process instead of a direct reqwest transport.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn streamable_http_remote_client_round_trips_through_exec_server() -> anyhow::Result<()> {
// Phase 1: start the MCP Streamable HTTP test server and a local
// exec-server process that will own the HTTP network calls.
let (_server, base_url) = spawn_streamable_http_server().await?;
let exec_server = spawn_exec_server().await?;
// Phase 2: create and initialize the RMCP client using the executor-backed
// Streamable HTTP transport.
let client = create_remote_client(&base_url, exec_server.client.clone()).await?;
// Phase 3: prove the initialized client can complete a tool call and
// preserve the normal RMCP response shape.
let result = call_echo_tool(&client, "remote").await?;
assert_eq!(result, expected_echo_result("remote"));
Ok(())
}
@@ -0,0 +1,314 @@
//! Shared helpers for Streamable HTTP RMCP integration tests.
//!
//! This support module starts the test HTTP server, launches a real
//! `exec-server` when remote coverage is needed, and provides small helpers for
//! creating RMCP clients and asserting round-trip behavior.
// This support module is included by multiple integration-test crates. Each
// crate uses a different subset of the helpers, so dead-code warnings would
// otherwise depend on which test file compiled the module.
#![allow(dead_code)]
use std::net::TcpListener;
use std::path::PathBuf;
use std::process::Stdio;
use std::sync::Arc;
use std::time::Duration;
use std::time::Instant;
use anyhow::Context as _;
use codex_config::types::OAuthCredentialsStoreMode;
use codex_exec_server::Environment;
use codex_exec_server::ExecServerClient;
use codex_exec_server::RemoteExecServerConnectArgs;
use codex_rmcp_client::ElicitationAction;
use codex_rmcp_client::ElicitationResponse;
use codex_rmcp_client::RmcpClient;
use codex_utils_cargo_bin::CargoBinError;
use futures::FutureExt as _;
use pretty_assertions::assert_eq;
use rmcp::model::CallToolResult;
use rmcp::model::ClientCapabilities;
use rmcp::model::ElicitationCapability;
use rmcp::model::FormElicitationCapability;
use rmcp::model::Implementation;
use rmcp::model::InitializeRequestParams;
use rmcp::model::ProtocolVersion;
use serde_json::json;
use tempfile::TempDir;
use tokio::io::AsyncBufReadExt;
use tokio::io::BufReader;
use tokio::net::TcpStream;
use tokio::process::Child;
use tokio::process::Command;
use tokio::time::sleep;
const SESSION_POST_FAILURE_CONTROL_PATH: &str = "/test/control/session-post-failure";
fn streamable_http_server_bin() -> Result<PathBuf, CargoBinError> {
codex_utils_cargo_bin::cargo_bin("test_streamable_http_server")
}
fn init_params() -> InitializeRequestParams {
InitializeRequestParams {
meta: None,
capabilities: ClientCapabilities {
experimental: None,
extensions: None,
roots: None,
sampling: None,
elicitation: Some(ElicitationCapability {
form: Some(FormElicitationCapability {
schema_validation: None,
}),
url: None,
}),
tasks: None,
},
client_info: Implementation {
name: "codex-test".into(),
version: "0.0.0-test".into(),
title: Some("Codex rmcp recovery test".into()),
description: None,
icons: None,
website_url: None,
},
protocol_version: ProtocolVersion::V_2025_06_18,
}
}
pub(crate) fn expected_echo_result(message: &str) -> CallToolResult {
CallToolResult {
content: Vec::new(),
structured_content: Some(json!({
"echo": format!("ECHOING: {message}"),
"env": null,
})),
is_error: Some(false),
meta: None,
}
}
pub(crate) async fn create_client(base_url: &str) -> anyhow::Result<RmcpClient> {
let client = RmcpClient::new_streamable_http_client(
"test-streamable-http",
&format!("{base_url}/mcp"),
Some("test-bearer".to_string()),
/*http_headers*/ None,
/*env_http_headers*/ None,
OAuthCredentialsStoreMode::File,
Environment::default_for_tests().get_http_client(),
)
.await?;
client
.initialize(
init_params(),
Some(Duration::from_secs(5)),
Box::new(|_, _| {
async {
Ok(ElicitationResponse {
action: ElicitationAction::Accept,
content: Some(json!({})),
meta: None,
})
}
.boxed()
}),
)
.await?;
Ok(client)
}
/// Creates a Streamable HTTP RMCP client that sends traffic through the remote
/// runtime HTTP API.
pub(crate) async fn create_remote_client(
base_url: &str,
http_client: ExecServerClient,
) -> anyhow::Result<RmcpClient> {
let client = RmcpClient::new_streamable_http_client(
"test-streamable-http-remote",
&format!("{base_url}/mcp"),
Some("test-bearer".to_string()),
/*http_headers*/ None,
/*env_http_headers*/ None,
OAuthCredentialsStoreMode::File,
Arc::new(http_client),
)
.await?;
client
.initialize(
init_params(),
Some(Duration::from_secs(5)),
Box::new(|_, _| {
async {
Ok(ElicitationResponse {
action: ElicitationAction::Accept,
content: Some(json!({})),
meta: None,
})
}
.boxed()
}),
)
.await?;
Ok(client)
}
pub(crate) async fn call_echo_tool(
client: &RmcpClient,
message: &str,
) -> anyhow::Result<CallToolResult> {
client
.call_tool(
"echo".to_string(),
Some(json!({ "message": message })),
/*meta*/ None,
Some(Duration::from_secs(5)),
)
.await
}
pub(crate) async fn arm_session_post_failure(
base_url: &str,
status: u16,
remaining: usize,
) -> anyhow::Result<()> {
let response = reqwest::Client::new()
.post(format!("{base_url}{SESSION_POST_FAILURE_CONTROL_PATH}"))
.json(&json!({
"status": status,
"remaining": remaining,
}))
.send()
.await?;
assert_eq!(response.status(), reqwest::StatusCode::NO_CONTENT);
Ok(())
}
pub(crate) async fn spawn_streamable_http_server() -> anyhow::Result<(Child, String)> {
let listener = TcpListener::bind("127.0.0.1:0")?;
let port = listener.local_addr()?.port();
drop(listener);
let bind_addr = format!("127.0.0.1:{port}");
let base_url = format!("http://{bind_addr}");
let mut child = Command::new(streamable_http_server_bin()?)
.kill_on_drop(true)
.env("MCP_STREAMABLE_HTTP_BIND_ADDR", &bind_addr)
.spawn()?;
wait_for_streamable_http_server(&mut child, &bind_addr, Duration::from_secs(5)).await?;
Ok((child, base_url))
}
/// Owns the exec-server process used by the remote-client integration test.
pub(crate) struct ExecServerProcess {
_codex_home: TempDir,
child: Child,
pub(crate) client: ExecServerClient,
}
impl Drop for ExecServerProcess {
/// Stops the local exec-server process best-effort when the test exits.
fn drop(&mut self) {
let _ = self.child.start_kill();
}
}
/// Starts a local exec-server and connects an initialized `ExecServerClient`.
pub(crate) async fn spawn_exec_server() -> anyhow::Result<ExecServerProcess> {
let codex_home = TempDir::new()?;
let mut child = Command::new(codex_utils_cargo_bin::cargo_bin("codex")?)
.args(["exec-server", "--listen", "ws://127.0.0.1:0"])
.stdin(Stdio::null())
.stdout(Stdio::piped())
.stderr(Stdio::inherit())
.kill_on_drop(true)
.env("CODEX_HOME", codex_home.path())
.spawn()?;
let websocket_url = read_exec_server_listen_url(&mut child).await?;
let client = ExecServerClient::connect_websocket(RemoteExecServerConnectArgs::new(
websocket_url,
"rmcp-client-remote-http-test".to_string(),
))
.await?;
Ok(ExecServerProcess {
_codex_home: codex_home,
child,
client,
})
}
/// Reads the websocket URL printed by `codex exec-server --listen`.
async fn read_exec_server_listen_url(child: &mut Child) -> anyhow::Result<String> {
let stdout = child
.stdout
.take()
.context("failed to capture exec-server stdout")?;
let mut lines = BufReader::new(stdout).lines();
let deadline = Instant::now() + Duration::from_secs(10);
loop {
let remaining = deadline.saturating_duration_since(Instant::now());
if remaining.is_zero() {
anyhow::bail!("timed out waiting for exec-server listen URL");
}
let line = tokio::time::timeout(remaining, lines.next_line())
.await
.context("timed out waiting for exec-server stdout")??
.context("exec-server stdout closed before emitting listen URL")?;
let listen_url = line.trim();
if listen_url.starts_with("ws://") {
return Ok(listen_url.to_string());
}
}
}
async fn wait_for_streamable_http_server(
server_child: &mut Child,
address: &str,
timeout: Duration,
) -> anyhow::Result<()> {
let deadline = Instant::now() + timeout;
loop {
if let Some(status) = server_child.try_wait()? {
return Err(anyhow::anyhow!(
"streamable HTTP server exited early with status {status}"
));
}
let remaining = deadline.saturating_duration_since(Instant::now());
if remaining.is_zero() {
return Err(anyhow::anyhow!(
"timed out waiting for streamable HTTP server at {address}: deadline reached"
));
}
match tokio::time::timeout(remaining, TcpStream::connect(address)).await {
Ok(Ok(_)) => return Ok(()),
Ok(Err(error)) => {
if Instant::now() >= deadline {
return Err(anyhow::anyhow!(
"timed out waiting for streamable HTTP server at {address}: {error}"
));
}
}
Err(_) => {
return Err(anyhow::anyhow!(
"timed out waiting for streamable HTTP server at {address}: connect call timed out"
));
}
}
sleep(Duration::from_millis(50)).await;
}
}
+5 -5
View File
@@ -300,7 +300,7 @@ fn default_exec_approval_decisions(
}
#[derive(Clone, Debug, PartialEq, Eq)]
struct GuardianApprovalsMode {
struct AutoReviewMode {
approval_policy: AskForApproval,
approvals_reviewer: ApprovalsReviewer,
sandbox_policy: SandboxPolicy,
@@ -309,11 +309,11 @@ struct GuardianApprovalsMode {
/// Enabling the Auto-review experiment in the TUI should also switch the
/// current `/approvals` settings to the matching Auto-review mode. Users
/// can still change `/approvals` afterward; this just assumes that opting into
/// the experiment means they want guardian review enabled immediately.
fn guardian_approvals_mode() -> GuardianApprovalsMode {
GuardianApprovalsMode {
/// the experiment means they want Auto-review enabled immediately.
fn auto_review_mode() -> AutoReviewMode {
AutoReviewMode {
approval_policy: AskForApproval::OnRequest,
approvals_reviewer: ApprovalsReviewer::GuardianSubagent,
approvals_reviewer: ApprovalsReviewer::AutoReview,
sandbox_policy: SandboxPolicy::new_workspace_write_policy(),
}
}
@@ -183,6 +183,7 @@ impl PendingAppServerRequests {
response.permissions.clone(),
),
scope: response.scope.into(),
strict_auto_review: response.strict_auto_review.then_some(true),
})
.map_err(|err| {
format!("failed to serialize permissions approval response: {err}")
@@ -543,6 +544,7 @@ mod tests {
)),
},
scope: codex_protocol::request_permissions::PermissionGrantScope::Session,
strict_auto_review: false,
},
})
.expect("permissions response should serialize")
@@ -564,6 +566,7 @@ mod tests {
}),
},
scope: PermissionGrantScope::Session,
strict_auto_review: None,
}
);
+11 -15
View File
@@ -126,7 +126,7 @@ impl App {
return;
}
let guardian_approvals_preset = guardian_approvals_mode();
let auto_review_preset = auto_review_mode();
let mut next_config = self.config.clone();
let active_profile = self.active_profile.clone();
let scoped_segments = |key: &str| {
@@ -202,16 +202,12 @@ impl App {
// Persist the reviewer setting so future sessions keep the
// experiment's matching `/approvals` mode until the user
// changes it explicitly.
feature_config.approvals_reviewer =
guardian_approvals_preset.approvals_reviewer;
feature_config.approvals_reviewer = auto_review_preset.approvals_reviewer;
feature_edits.push(ConfigEdit::SetPath {
segments: scoped_segments("approvals_reviewer"),
value: guardian_approvals_preset
.approvals_reviewer
.to_string()
.into(),
value: auto_review_preset.approvals_reviewer.to_string().into(),
});
if previous_approvals_reviewer != guardian_approvals_preset.approvals_reviewer {
if previous_approvals_reviewer != auto_review_preset.approvals_reviewer {
permissions_history_label = Some("Auto-review");
}
} else if !effective_enabled {
@@ -234,17 +230,17 @@ impl App {
// makes guardian review observable in the current thread.
if !self.try_set_approval_policy_on_config(
&mut feature_config,
guardian_approvals_preset.approval_policy,
auto_review_preset.approval_policy,
"Failed to enable Auto-review",
"failed to set guardian approvals approval policy on staged config",
"failed to set auto-review approval policy on staged config",
) {
continue;
}
if !self.try_set_sandbox_policy_on_config(
&mut feature_config,
guardian_approvals_preset.sandbox_policy.clone(),
auto_review_preset.sandbox_policy.clone(),
"Failed to enable Auto-review",
"failed to set guardian approvals sandbox policy on staged config",
"failed to set auto-review sandbox policy on staged config",
) {
continue;
}
@@ -258,8 +254,8 @@ impl App {
value: "workspace-write".into(),
},
]);
approval_policy_override = Some(guardian_approvals_preset.approval_policy);
sandbox_policy_override = Some(guardian_approvals_preset.sandbox_policy.clone());
approval_policy_override = Some(auto_review_preset.approval_policy);
sandbox_policy_override = Some(auto_review_preset.sandbox_policy.clone());
}
next_config = feature_config;
feature_updates_to_apply.push((feature, effective_enabled));
@@ -305,7 +301,7 @@ impl App {
{
tracing::error!(
error = %err,
"failed to set guardian approvals sandbox policy on chat config"
"failed to set auto-review sandbox policy on chat config"
);
self.chat_widget
.add_error_message(format!("Failed to enable Auto-review: {err}"));
+32 -35
View File
@@ -1598,11 +1598,11 @@ async fn reset_memories_clears_local_memory_directories() -> Result<()> {
}
#[tokio::test]
async fn update_feature_flags_enabling_guardian_selects_guardian_approvals() -> Result<()> {
async fn update_feature_flags_enabling_guardian_selects_auto_review() -> Result<()> {
let (mut app, mut app_event_rx, mut op_rx) = make_test_app_with_channels().await;
let codex_home = tempdir()?;
app.config.codex_home = codex_home.path().to_path_buf().abs();
let guardian_approvals = guardian_approvals_mode();
let auto_review = auto_review_mode();
app.update_feature_flags(vec![(Feature::GuardianApproval, true)])
.await;
@@ -1616,11 +1616,11 @@ async fn update_feature_flags_enabling_guardian_selects_guardian_approvals() ->
);
assert_eq!(
app.config.approvals_reviewer,
guardian_approvals.approvals_reviewer
auto_review.approvals_reviewer
);
assert_eq!(
app.config.permissions.approval_policy.value(),
guardian_approvals.approval_policy
auto_review.approval_policy
);
assert_eq!(
app.chat_widget
@@ -1628,7 +1628,7 @@ async fn update_feature_flags_enabling_guardian_selects_guardian_approvals() ->
.permissions
.approval_policy
.value(),
guardian_approvals.approval_policy
auto_review.approval_policy
);
assert_eq!(
app.chat_widget
@@ -1636,11 +1636,11 @@ async fn update_feature_flags_enabling_guardian_selects_guardian_approvals() ->
.permissions
.sandbox_policy
.get(),
&guardian_approvals.sandbox_policy
&auto_review.sandbox_policy
);
assert_eq!(
app.chat_widget.config_ref().approvals_reviewer,
guardian_approvals.approvals_reviewer
auto_review.approvals_reviewer
);
assert_eq!(app.runtime_approval_policy_override, None);
assert_eq!(app.runtime_sandbox_policy_override, None);
@@ -1648,9 +1648,9 @@ async fn update_feature_flags_enabling_guardian_selects_guardian_approvals() ->
op_rx.try_recv(),
Ok(Op::OverrideTurnContext {
cwd: None,
approval_policy: Some(guardian_approvals.approval_policy),
approvals_reviewer: Some(guardian_approvals.approvals_reviewer),
sandbox_policy: Some(guardian_approvals.sandbox_policy.clone()),
approval_policy: Some(auto_review.approval_policy),
approvals_reviewer: Some(auto_review.approvals_reviewer),
sandbox_policy: Some(auto_review.sandbox_policy.clone()),
permission_profile: None,
windows_sandbox_level: None,
model: None,
@@ -1700,9 +1700,9 @@ async fn update_feature_flags_disabling_guardian_clears_review_policy_and_restor
.set_enabled(Feature::GuardianApproval, /*enabled*/ true)?;
app.chat_widget
.set_feature_enabled(Feature::GuardianApproval, /*enabled*/ true);
app.config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
app.config.approvals_reviewer = ApprovalsReviewer::AutoReview;
app.chat_widget
.set_approvals_reviewer(ApprovalsReviewer::GuardianSubagent);
.set_approvals_reviewer(ApprovalsReviewer::AutoReview);
app.config
.permissions
.approval_policy
@@ -1779,7 +1779,7 @@ async fn update_feature_flags_enabling_guardian_overrides_explicit_manual_review
let (mut app, _app_event_rx, mut op_rx) = make_test_app_with_channels().await;
let codex_home = tempdir()?;
app.config.codex_home = codex_home.path().to_path_buf().abs();
let guardian_approvals = guardian_approvals_mode();
let auto_review = auto_review_mode();
let config_toml_path = codex_home.path().join("config.toml").abs();
let config_toml = "approvals_reviewer = \"user\"\n";
std::fs::write(config_toml_path.as_path(), config_toml)?;
@@ -1798,15 +1798,15 @@ async fn update_feature_flags_enabling_guardian_overrides_explicit_manual_review
assert!(app.config.features.enabled(Feature::GuardianApproval));
assert_eq!(
app.config.approvals_reviewer,
guardian_approvals.approvals_reviewer
auto_review.approvals_reviewer
);
assert_eq!(
app.chat_widget.config_ref().approvals_reviewer,
guardian_approvals.approvals_reviewer
auto_review.approvals_reviewer
);
assert_eq!(
app.config.permissions.approval_policy.value(),
guardian_approvals.approval_policy
auto_review.approval_policy
);
assert_eq!(
app.chat_widget
@@ -1814,15 +1814,15 @@ async fn update_feature_flags_enabling_guardian_overrides_explicit_manual_review
.permissions
.sandbox_policy
.get(),
&guardian_approvals.sandbox_policy
&auto_review.sandbox_policy
);
assert_eq!(
op_rx.try_recv(),
Ok(Op::OverrideTurnContext {
cwd: None,
approval_policy: Some(guardian_approvals.approval_policy),
approvals_reviewer: Some(guardian_approvals.approvals_reviewer),
sandbox_policy: Some(guardian_approvals.sandbox_policy.clone()),
approval_policy: Some(auto_review.approval_policy),
approvals_reviewer: Some(auto_review.approvals_reviewer),
sandbox_policy: Some(auto_review.sandbox_policy.clone()),
permission_profile: None,
windows_sandbox_level: None,
model: None,
@@ -1908,7 +1908,7 @@ async fn update_feature_flags_enabling_guardian_in_profile_sets_profile_auto_rev
let (mut app, _app_event_rx, mut op_rx) = make_test_app_with_channels().await;
let codex_home = tempdir()?;
app.config.codex_home = codex_home.path().to_path_buf().abs();
let guardian_approvals = guardian_approvals_mode();
let auto_review = auto_review_mode();
app.active_profile = Some("guardian".to_string());
let config_toml_path = codex_home.path().join("config.toml").abs();
let config_toml = "profile = \"guardian\"\napprovals_reviewer = \"user\"\n";
@@ -1928,19 +1928,19 @@ async fn update_feature_flags_enabling_guardian_in_profile_sets_profile_auto_rev
assert!(app.config.features.enabled(Feature::GuardianApproval));
assert_eq!(
app.config.approvals_reviewer,
guardian_approvals.approvals_reviewer
auto_review.approvals_reviewer
);
assert_eq!(
app.chat_widget.config_ref().approvals_reviewer,
guardian_approvals.approvals_reviewer
auto_review.approvals_reviewer
);
assert_eq!(
op_rx.try_recv(),
Ok(Op::OverrideTurnContext {
cwd: None,
approval_policy: Some(guardian_approvals.approval_policy),
approvals_reviewer: Some(guardian_approvals.approvals_reviewer),
sandbox_policy: Some(guardian_approvals.sandbox_policy.clone()),
approval_policy: Some(auto_review.approval_policy),
approvals_reviewer: Some(auto_review.approvals_reviewer),
sandbox_policy: Some(auto_review.sandbox_policy.clone()),
permission_profile: None,
windows_sandbox_level: None,
model: None,
@@ -2003,9 +2003,9 @@ guardian_approval = true
.set_enabled(Feature::GuardianApproval, /*enabled*/ true)?;
app.chat_widget
.set_feature_enabled(Feature::GuardianApproval, /*enabled*/ true);
app.config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
app.config.approvals_reviewer = ApprovalsReviewer::AutoReview;
app.chat_widget
.set_approvals_reviewer(ApprovalsReviewer::GuardianSubagent);
.set_approvals_reviewer(ApprovalsReviewer::AutoReview);
app.update_feature_flags(vec![(Feature::GuardianApproval, false)])
.await;
@@ -2083,9 +2083,9 @@ async fn update_feature_flags_disabling_guardian_in_profile_keeps_inherited_non_
.set_enabled(Feature::GuardianApproval, /*enabled*/ true)?;
app.chat_widget
.set_feature_enabled(Feature::GuardianApproval, /*enabled*/ true);
app.config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
app.config.approvals_reviewer = ApprovalsReviewer::AutoReview;
app.chat_widget
.set_approvals_reviewer(ApprovalsReviewer::GuardianSubagent);
.set_approvals_reviewer(ApprovalsReviewer::AutoReview);
app.update_feature_flags(vec![(Feature::GuardianApproval, false)])
.await;
@@ -2097,13 +2097,10 @@ async fn update_feature_flags_disabling_guardian_in_profile_keeps_inherited_non_
.features
.enabled(Feature::GuardianApproval)
);
assert_eq!(
app.config.approvals_reviewer,
ApprovalsReviewer::GuardianSubagent
);
assert_eq!(app.config.approvals_reviewer, ApprovalsReviewer::AutoReview);
assert_eq!(
app.chat_widget.config_ref().approvals_reviewer,
ApprovalsReviewer::GuardianSubagent
ApprovalsReviewer::AutoReview
);
assert!(
op_rx.try_recv().is_err(),
@@ -278,8 +278,8 @@ impl ApprovalOverlay {
permissions,
..
},
ApprovalDecision::Review(decision),
) => self.handle_permissions_decision(call_id, permissions, decision.clone()),
ApprovalDecision::Permissions(decision),
) => self.handle_permissions_decision(call_id, permissions, *decision),
(ApprovalRequest::ApplyPatch { id, .. }, ApprovalDecision::Review(decision)) => {
self.handle_patch_decision(id, decision.clone());
}
@@ -322,27 +322,31 @@ impl ApprovalOverlay {
&self,
call_id: &str,
permissions: &RequestPermissionProfile,
decision: ReviewDecision,
decision: PermissionsDecision,
) {
let Some(request) = self.current_request.as_ref() else {
return;
};
let granted_permissions = match decision {
ReviewDecision::Approved | ReviewDecision::ApprovedForSession => permissions.clone(),
ReviewDecision::Denied | ReviewDecision::TimedOut | ReviewDecision::Abort => {
Default::default()
}
ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::NetworkPolicyAmendment { .. } => Default::default(),
PermissionsDecision::GrantForTurn
| PermissionsDecision::GrantForTurnWithStrictAutoReview
| PermissionsDecision::GrantForSession => permissions.clone(),
PermissionsDecision::Deny => Default::default(),
};
let scope = if matches!(decision, ReviewDecision::ApprovedForSession) {
let scope = if matches!(decision, PermissionsDecision::GrantForSession) {
PermissionGrantScope::Session
} else {
PermissionGrantScope::Turn
};
let strict_auto_review = matches!(
decision,
PermissionsDecision::GrantForTurnWithStrictAutoReview
);
if request.thread_label().is_none() {
let message = if granted_permissions.is_empty() {
"You did not grant additional permissions"
} else if strict_auto_review {
"You granted additional permissions with strict auto review"
} else if matches!(scope, PermissionGrantScope::Session) {
"You granted additional permissions for this session"
} else {
@@ -359,6 +363,7 @@ impl ApprovalOverlay {
codex_protocol::request_permissions::RequestPermissionsResponse {
permissions: granted_permissions,
scope,
strict_auto_review,
},
);
}
@@ -482,7 +487,11 @@ impl BottomPaneView for ApprovalOverlay {
permissions,
..
} => {
self.handle_permissions_decision(call_id, permissions, ReviewDecision::Abort);
self.handle_permissions_decision(
call_id,
permissions,
PermissionsDecision::Deny,
);
}
ApprovalRequest::ApplyPatch { id, .. } => {
self.handle_patch_decision(id, ReviewDecision::Abort);
@@ -679,9 +688,18 @@ fn build_header(request: &ApprovalRequest) -> Box<dyn Renderable> {
#[derive(Clone)]
enum ApprovalDecision {
Review(ReviewDecision),
Permissions(PermissionsDecision),
McpElicitation(ElicitationAction),
}
#[derive(Clone, Copy)]
enum PermissionsDecision {
GrantForTurn,
GrantForTurnWithStrictAutoReview,
GrantForSession,
Deny,
}
#[derive(Clone)]
struct ApprovalOption {
label: String,
@@ -901,20 +919,28 @@ fn patch_options() -> Vec<ApprovalOption> {
fn permissions_options() -> Vec<ApprovalOption> {
vec![
ApprovalOption {
label: "Yes, grant these permissions".to_string(),
decision: ApprovalDecision::Review(ReviewDecision::Approved),
label: "Yes, grant these permissions for this turn".to_string(),
decision: ApprovalDecision::Permissions(PermissionsDecision::GrantForTurn),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))],
},
ApprovalOption {
label: "Yes, grant for this turn with strict auto review".to_string(),
decision: ApprovalDecision::Permissions(
PermissionsDecision::GrantForTurnWithStrictAutoReview,
),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('r'))],
},
ApprovalOption {
label: "Yes, grant these permissions for this session".to_string(),
decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession),
decision: ApprovalDecision::Permissions(PermissionsDecision::GrantForSession),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))],
},
ApprovalOption {
label: "No, continue without permissions".to_string(),
decision: ApprovalDecision::Review(ReviewDecision::Denied),
decision: ApprovalDecision::Permissions(PermissionsDecision::Deny),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))],
},
@@ -1347,7 +1373,8 @@ mod tests {
assert_eq!(
labels,
vec![
"Yes, grant these permissions".to_string(),
"Yes, grant these permissions for this turn".to_string(),
"Yes, grant for this turn with strict auto review".to_string(),
"Yes, grant these permissions for this session".to_string(),
"No, continue without permissions".to_string(),
]
@@ -1410,6 +1437,34 @@ mod tests {
);
}
#[test]
fn permissions_strict_auto_review_shortcut_submits_turn_scope_with_strict_review() {
let (tx, mut rx) = unbounded_channel::<AppEvent>();
let tx = AppEventSender::new(tx);
let mut view =
ApprovalOverlay::new(make_permissions_request(), tx, Features::with_defaults());
view.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::NONE));
let mut saw_op = false;
while let Ok(ev) = rx.try_recv() {
if let AppEvent::SubmitThreadOp {
op: Op::RequestPermissionsResponse { response, .. },
..
} = ev
{
assert_eq!(response.scope, PermissionGrantScope::Turn);
assert!(response.strict_auto_review);
saw_op = true;
break;
}
}
assert!(
saw_op,
"expected permission approval decision to emit a strict auto review response"
);
}
#[test]
fn additional_permissions_prompt_shows_permission_rule_line() {
let (tx, _rx) = unbounded_channel::<AppEvent>();
@@ -9,8 +9,9 @@ expression: "normalize_snapshot_paths(render_overlay_lines(&view, 120))"
Permission rule: network; read `/tmp/readme.txt`; write `/tmp/out.txt`
› 1. Yes, grant these permissions (y)
2. Yes, grant these permissions for this session (a)
3. No, continue without permissions (n)
› 1. Yes, grant these permissions for this turn (y)
2. Yes, grant for this turn with strict auto review (r)
3. Yes, grant these permissions for this session (a)
4. No, continue without permissions (n)
Press enter to confirm or esc to cancel
+2 -2
View File
@@ -9043,7 +9043,7 @@ impl ChatWidget {
"Same workspace-write permissions as Default, but eligible `on-request` approvals are routed through the auto-reviewer subagent."
.to_string(),
),
is_current: current_review_policy == ApprovalsReviewer::GuardianSubagent
is_current: current_review_policy == ApprovalsReviewer::AutoReview
&& Self::preset_matches_current(
current_approval,
current_sandbox,
@@ -9053,7 +9053,7 @@ impl ChatWidget {
preset.approval,
preset.sandbox.clone(),
"Auto-review".to_string(),
ApprovalsReviewer::GuardianSubagent,
ApprovalsReviewer::AutoReview,
),
dismiss_on_select: true,
disabled_reason: approval_disabled_reason
@@ -386,7 +386,7 @@ async fn permissions_selection_emits_history_cell_when_current_is_selected() {
}
#[tokio::test]
async fn permissions_selection_hides_guardian_approvals_when_feature_disabled() {
async fn permissions_selection_hides_auto_review_when_feature_disabled() {
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
#[cfg(target_os = "windows")]
{
@@ -401,12 +401,12 @@ async fn permissions_selection_hides_guardian_approvals_when_feature_disabled()
assert!(
!popup.contains("Auto-review"),
"expected Auto-review to stay hidden until the experimental feature is enabled: {popup}"
"expected Auto-review to stay hidden until the feature is enabled: {popup}"
);
}
#[tokio::test]
async fn permissions_selection_hides_guardian_approvals_when_feature_disabled_even_if_auto_review_is_active()
async fn permissions_selection_hides_auto_review_when_feature_disabled_even_if_auto_review_is_active()
{
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
#[cfg(target_os = "windows")]
@@ -416,7 +416,7 @@ async fn permissions_selection_hides_guardian_approvals_when_feature_disabled_ev
}
chat.set_feature_enabled(Feature::GuardianApproval, /*enabled*/ false);
chat.config.notices.hide_full_access_warning = Some(true);
chat.config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
chat.config.approvals_reviewer = ApprovalsReviewer::AutoReview;
chat.config
.permissions
.approval_policy
@@ -433,12 +433,12 @@ async fn permissions_selection_hides_guardian_approvals_when_feature_disabled_ev
assert!(
!popup.contains("Auto-review"),
"expected Auto-review to stay hidden when the experimental feature is disabled: {popup}"
"expected Auto-review to stay hidden when the feature is disabled: {popup}"
);
}
#[tokio::test]
async fn permissions_selection_marks_guardian_approvals_current_after_session_configured() {
async fn permissions_selection_marks_auto_review_current_after_session_configured() {
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
#[cfg(target_os = "windows")]
{
@@ -461,7 +461,7 @@ async fn permissions_selection_marks_guardian_approvals_current_after_session_co
model_provider_id: "test-provider".to_string(),
service_tier: None,
approval_policy: AskForApproval::OnRequest,
approvals_reviewer: ApprovalsReviewer::GuardianSubagent,
approvals_reviewer: ApprovalsReviewer::AutoReview,
sandbox_policy: SandboxPolicy::new_workspace_write_policy(),
permission_profile: None,
cwd: test_project_path().abs(),
@@ -484,8 +484,7 @@ async fn permissions_selection_marks_guardian_approvals_current_after_session_co
}
#[tokio::test]
async fn permissions_selection_marks_guardian_approvals_current_with_custom_workspace_write_details()
{
async fn permissions_selection_marks_auto_review_current_with_custom_workspace_write_details() {
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
#[cfg(target_os = "windows")]
{
@@ -510,7 +509,7 @@ async fn permissions_selection_marks_guardian_approvals_current_with_custom_work
model_provider_id: "test-provider".to_string(),
service_tier: None,
approval_policy: AskForApproval::OnRequest,
approvals_reviewer: ApprovalsReviewer::GuardianSubagent,
approvals_reviewer: ApprovalsReviewer::AutoReview,
sandbox_policy: SandboxPolicy::WorkspaceWrite {
writable_roots: vec![extra_root],
read_only_access: ReadOnlyAccess::FullAccess,
@@ -539,7 +538,7 @@ async fn permissions_selection_marks_guardian_approvals_current_with_custom_work
}
#[tokio::test]
async fn permissions_selection_can_disable_guardian_approvals() {
async fn permissions_selection_can_disable_auto_review() {
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
#[cfg(target_os = "windows")]
{
@@ -632,7 +631,7 @@ async fn permissions_selection_sends_approvals_reviewer_in_override_turn_context
Op::OverrideTurnContext {
cwd: None,
approval_policy: Some(AskForApproval::OnRequest),
approvals_reviewer: Some(ApprovalsReviewer::GuardianSubagent),
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
sandbox_policy: Some(SandboxPolicy::new_workspace_write_policy()),
permission_profile: None,
windows_sandbox_level: None,
@@ -1,5 +1,6 @@
use super::*;
use codex_app_server_protocol::AppInfo;
use codex_features::Stage;
use pretty_assertions::assert_eq;
#[tokio::test]
@@ -1747,31 +1748,22 @@ async fn experimental_popup_shows_js_repl_node_requirement() {
}
#[tokio::test]
async fn experimental_popup_includes_guardian_approval() {
async fn experimental_popup_omits_stable_guardian_approval() {
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
let guardian_stage = FEATURES
.iter()
.find(|spec| spec.id == Feature::GuardianApproval)
.map(|spec| spec.stage)
.expect("expected guardian approval feature metadata");
let guardian_name = guardian_stage
.experimental_menu_name()
.expect("expected guardian approval experimental menu name");
let guardian_description = guardian_stage
.experimental_menu_description()
.expect("expected guardian approval experimental description");
assert_eq!(guardian_stage, Stage::Stable);
chat.open_experimental_popup();
let popup = render_bottom_popup(&chat, /*width*/ 120);
let normalized_popup = popup.split_whitespace().collect::<Vec<_>>().join(" ");
assert!(
popup.contains(guardian_name),
"expected auto-review entry in experimental popup, got:\n{popup}"
);
assert!(
normalized_popup.contains(guardian_description),
"expected auto-review description in experimental popup, got:\n{popup}"
!popup.contains("Auto-review"),
"expected stable auto-review feature to be omitted from experimental popup, got:\n{popup}"
);
}
+4 -4
View File
@@ -586,7 +586,7 @@ mod tests {
Some(RequirementSource::CloudRequirements),
),
approvals_reviewer: ConstrainedWithSource::new(
Constrained::allow_any(ApprovalsReviewer::GuardianSubagent),
Constrained::allow_any(ApprovalsReviewer::AutoReview),
Some(RequirementSource::LegacyManagedConfigTomlFromMdm),
),
sandbox_policy: ConstrainedWithSource::new(
@@ -647,7 +647,7 @@ mod tests {
let requirements_toml = ConfigRequirementsToml {
allowed_approval_policies: Some(vec![AskForApproval::OnRequest]),
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]),
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]),
allowed_sandbox_modes: Some(vec![SandboxModeRequirement::ReadOnly]),
remote_sandbox_config: None,
allowed_web_search_modes: Some(vec![WebSearchModeRequirement::Cached]),
@@ -731,13 +731,13 @@ mod tests {
fn debug_config_output_lists_approvals_reviewer_as_requirement() {
let requirements = ConfigRequirements {
approvals_reviewer: ConstrainedWithSource::new(
Constrained::allow_any(ApprovalsReviewer::GuardianSubagent),
Constrained::allow_any(ApprovalsReviewer::AutoReview),
Some(RequirementSource::LegacyManagedConfigTomlFromMdm),
),
..ConfigRequirements::default()
};
let requirements_toml = ConfigRequirementsToml {
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]),
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]),
..ConfigRequirementsToml::default()
};
let stack = ConfigLayerStack::new(Vec::new(), requirements, requirements_toml)
+11 -4
View File
@@ -18,11 +18,11 @@ PLATFORMS = [
WINDOWS_RUSTC_LINK_FLAGS = select({
"@rules_rs//rs/experimental/platforms/constraints:windows_gnullvm": [
"-C",
"link-arg=-Wl,--stack,8388608",
"link-arg=-Wl,--stack,8388608", # 8 MiB
],
"@rules_rs//rs/experimental/platforms/constraints:windows_msvc": [
"-C",
"link-arg=/STACK:8388608",
"link-arg=/STACK:8388608", # 8 MiB
"-C",
"link-arg=/NODEFAULTLIB:libucrt.lib",
"-C",
@@ -43,6 +43,13 @@ MACOS_WEBRTC_RUSTC_LINK_FLAGS = select({
"//conditions:default": [],
})
RUST_LIBTEST_ENV = {
# Rust's libtest harness runs test bodies on std-spawned threads, whose
# default 2 MiB stack can be too small for large async test futures on
# Windows. Keep this aligned with the Windows test binary stack reserve.
"RUST_MIN_STACK": "8388608", # 8 MiB
}
def multiplatform_binaries(name, platforms = PLATFORMS):
for platform in platforms:
platform_data(
@@ -187,7 +194,7 @@ def codex_rust_crate(
extra_binaries: Additional binary labels to surface as test data and
`CARGO_BIN_EXE_*` environment variables. These are only needed for binaries from a different crate.
"""
test_env = {
test_env = RUST_LIBTEST_ENV | {
# The launcher resolves an absolute workspace root at runtime so
# manifest-only platforms like macOS still point Insta at the real
# `codex-rs` checkout.
@@ -359,7 +366,7 @@ def codex_rust_crate(
# Important: do not merge `test_env` here. Its unit-test-only
# `INSTA_WORKSPACE_ROOT="codex-rs"` is tuned for unit tests that
# execute from the repo root and can misplace integration snapshots.
env = cargo_env,
env = RUST_LIBTEST_ENV | cargo_env,
tags = test_tags,
**test_kwargs
)
+3 -1
View File
@@ -1,6 +1,8 @@
set working-directory := "codex-rs"
set positional-arguments
rust_min_stack := "8388608" # 8 MiB
# Display help
help:
just -l
@@ -49,7 +51,7 @@ install:
# Prefer this for routine local runs. Workspace crate features are banned, so
# there should be no need to add `--all-features`.
test:
cargo nextest run --no-fail-fast
RUST_MIN_STACK={{ rust_min_stack }} cargo nextest run --no-fail-fast
# Build and run Codex from source using Bazel.
# Note we have to use the combination of `[no-cd]` and `--run_under="cd $PWD &&"`