mirror of
https://github.com/openai/codex.git
synced 2026-10-25 15:56:38 +00:00
Surface environment startup failure reasons to the model (#44277)
## Why Failed environments were omitted from environment context, and `wait_for_environment` returned a generic failure or treated an already-failed environment as unselected. The model needs the failure reason to explain problems such as an empty repository. ## What changed - Preserve the selected environment and error when startup or configuration fails. - Include failed status and XML-escaped error details in environment context, and return the reason from `wait_for_environment` for failures detected before or during a wait. - Limit error details to 256 bytes per environment and 512 bytes total in context, preserving UTF-8 boundaries. Limit wait failure details to 256 bytes. ## Testing Add coverage for failures before a turn and while waiting, verifying that the model receives the reason and the turn completes. Add unit tests for XML escaping, incremental context updates, clearing errors on recovery, and UTF-8-safe truncation. GitOrigin-RevId: 093ab9b447c7b3243fbb53b665180db95479f2a7
This commit is contained in:
committed by
copyberry
parent
f5c5d9b2b0
commit
9caddc5cf5
@@ -5,6 +5,7 @@ use crate::context::environment_context::FileSystemContext;
|
||||
use crate::context::environment_context::NetworkContext;
|
||||
use crate::context::environment_context::push_xml_escaped_text;
|
||||
use crate::environment_selection::TurnEnvironmentSnapshot;
|
||||
use crate::environment_selection::TurnEnvironmentState;
|
||||
use crate::session::turn_context::TurnContext;
|
||||
use crate::shell::ShellType;
|
||||
use codex_features::Feature;
|
||||
@@ -114,6 +115,7 @@ impl WorldStateSection for EnvironmentsState {
|
||||
EnvironmentSnapshot {
|
||||
cwd: environment.cwd.inferred_native_path_string(),
|
||||
status: environment.status,
|
||||
error: environment.error.clone(),
|
||||
shell: environment.shell.clone(),
|
||||
is_primary: self.environments.len() > 1 && environment.is_primary,
|
||||
},
|
||||
@@ -321,6 +323,16 @@ fn push_environment_values(rendered: &mut String, environment: &EnvironmentState
|
||||
rendered.push_str(indent);
|
||||
rendered.push_str("<status>starting</status>\n");
|
||||
}
|
||||
if environment.status == EnvironmentStatus::Failed {
|
||||
rendered.push_str(indent);
|
||||
rendered.push_str("<status>failed</status>\n");
|
||||
}
|
||||
if let Some(error) = &environment.error {
|
||||
rendered.push_str(indent);
|
||||
rendered.push_str("<error>");
|
||||
push_xml_escaped_text(rendered, error);
|
||||
rendered.push_str("</error>\n");
|
||||
}
|
||||
if let Some(shell) = &environment.shell {
|
||||
rendered.push_str(indent);
|
||||
rendered.push_str("<shell>");
|
||||
@@ -347,6 +359,7 @@ struct EnvironmentState {
|
||||
cwd: PathUri,
|
||||
status: EnvironmentStatus,
|
||||
shell: Option<String>,
|
||||
error: Option<String>,
|
||||
is_primary: bool,
|
||||
}
|
||||
|
||||
@@ -367,6 +380,8 @@ struct EnvironmentSnapshot {
|
||||
cwd: String,
|
||||
status: EnvironmentStatus,
|
||||
shell: Option<String>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
error: Option<String>,
|
||||
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
|
||||
is_primary: bool,
|
||||
}
|
||||
@@ -375,6 +390,7 @@ impl EnvironmentSnapshot {
|
||||
fn has_same_diff_value(&self, other: &Self) -> bool {
|
||||
self.cwd == other.cwd
|
||||
&& self.status == other.status
|
||||
&& self.error == other.error
|
||||
&& self.is_primary == other.is_primary
|
||||
&& self
|
||||
.shell
|
||||
@@ -389,6 +405,7 @@ impl EnvironmentSnapshot {
|
||||
enum EnvironmentStatus {
|
||||
Starting,
|
||||
Available,
|
||||
Failed,
|
||||
}
|
||||
|
||||
async fn powershell_version(shell_path: &Path) -> Option<String> {
|
||||
@@ -441,6 +458,7 @@ fn environment_states(snapshot: &TurnEnvironmentSnapshot) -> BTreeMap<String, En
|
||||
EnvironmentState {
|
||||
cwd: environment.cwd().clone(),
|
||||
status: EnvironmentStatus::Available,
|
||||
error: None,
|
||||
shell: environment
|
||||
.shell
|
||||
.as_ref()
|
||||
@@ -456,10 +474,33 @@ fn environment_states(snapshot: &TurnEnvironmentSnapshot) -> BTreeMap<String, En
|
||||
.or_insert_with(|| EnvironmentState {
|
||||
cwd: environment.selection.cwd.clone(),
|
||||
status: EnvironmentStatus::Starting,
|
||||
error: None,
|
||||
shell: None,
|
||||
is_primary: false,
|
||||
});
|
||||
}
|
||||
// Bound the additional model context even when many selected environments fail.
|
||||
const MAX_ERROR_BYTES: usize = 256;
|
||||
const MAX_TOTAL_ERROR_BYTES: usize = 512;
|
||||
let mut remaining_error_bytes = MAX_TOTAL_ERROR_BYTES;
|
||||
for environment in &snapshot.environments {
|
||||
if let TurnEnvironmentState::Failed { selection, error } = environment {
|
||||
let detail = error
|
||||
[..error.floor_char_boundary(remaining_error_bytes.min(MAX_ERROR_BYTES))]
|
||||
.to_string();
|
||||
remaining_error_bytes -= detail.len();
|
||||
environments.insert(
|
||||
selection.environment_id.clone(),
|
||||
EnvironmentState {
|
||||
cwd: selection.cwd.clone(),
|
||||
status: EnvironmentStatus::Failed,
|
||||
shell: None,
|
||||
error: (!detail.is_empty()).then_some(detail),
|
||||
is_primary: false,
|
||||
},
|
||||
);
|
||||
}
|
||||
}
|
||||
environments
|
||||
}
|
||||
|
||||
|
||||
@@ -34,6 +34,7 @@ fn environment(id: &str, cwd: PathUri, shell: impl Into<String>) -> (String, Env
|
||||
EnvironmentState {
|
||||
cwd,
|
||||
status: EnvironmentStatus::Available,
|
||||
error: None,
|
||||
shell: Some(shell.into()),
|
||||
is_primary: false,
|
||||
},
|
||||
|
||||
@@ -105,6 +105,7 @@ fn snapshots() -> Result<()> {
|
||||
EnvironmentState {
|
||||
cwd: PathUri::parse("file:///repo")?,
|
||||
status: EnvironmentStatus::Available,
|
||||
error: None,
|
||||
shell: None,
|
||||
is_primary: false,
|
||||
},
|
||||
@@ -308,6 +309,7 @@ fn available(cwd: &str, shell: &str) -> Result<EnvironmentState> {
|
||||
Ok(EnvironmentState {
|
||||
cwd: PathUri::parse(cwd)?,
|
||||
status: EnvironmentStatus::Available,
|
||||
error: None,
|
||||
shell: Some(shell.to_string()),
|
||||
is_primary: false,
|
||||
})
|
||||
@@ -324,7 +326,91 @@ fn starting(cwd: &str) -> Result<EnvironmentState> {
|
||||
Ok(EnvironmentState {
|
||||
cwd: PathUri::parse(cwd)?,
|
||||
status: EnvironmentStatus::Starting,
|
||||
error: None,
|
||||
shell: None,
|
||||
is_primary: false,
|
||||
})
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn failure_context_is_escaped_incremental_and_cleared_on_recovery() -> Result<()> {
|
||||
let failed = EnvironmentsState {
|
||||
environments: [(
|
||||
"remote".to_string(),
|
||||
EnvironmentState {
|
||||
cwd: PathUri::parse("file:///workspace")?,
|
||||
status: EnvironmentStatus::Failed,
|
||||
error: Some("Repository <empty> & unavailable".to_string()),
|
||||
shell: None,
|
||||
is_primary: false,
|
||||
},
|
||||
)]
|
||||
.into_iter()
|
||||
.collect(),
|
||||
..Default::default()
|
||||
};
|
||||
insta::assert_snapshot!(failed.body(), @r#"
|
||||
|
||||
<environments>
|
||||
<environment id="remote">
|
||||
<cwd>/workspace</cwd>
|
||||
<status>failed</status>
|
||||
<error>Repository <empty> & unavailable</error>
|
||||
</environment>
|
||||
</environments>
|
||||
"#);
|
||||
assert!(
|
||||
failed
|
||||
.render_diff(PreviousSectionState::Known(&failed.snapshot()))
|
||||
.is_none()
|
||||
);
|
||||
let recovered = EnvironmentsState {
|
||||
environments: [(
|
||||
"remote".to_string(),
|
||||
available("file:///workspace", "bash")?,
|
||||
)]
|
||||
.into_iter()
|
||||
.collect(),
|
||||
..Default::default()
|
||||
};
|
||||
let update = recovered
|
||||
.render_diff(PreviousSectionState::Known(&failed.snapshot()))
|
||||
.expect("recovery must be visible to the model");
|
||||
assert!(!update.body().contains("<error>"));
|
||||
assert!(update.body().contains("<shell>bash</shell>"));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn failure_context_limits_total_detail_bytes_at_utf8_boundaries() {
|
||||
use codex_protocol::protocol::EnvironmentConfigState;
|
||||
use codex_protocol::protocol::TurnEnvironmentSelection;
|
||||
let snapshot = TurnEnvironmentSnapshot {
|
||||
environments: (0..4)
|
||||
.map(|index| TurnEnvironmentState::Failed {
|
||||
selection: TurnEnvironmentSelection {
|
||||
environment_id: format!("remote-{index}"),
|
||||
cwd: PathUri::parse("file:///workspace").unwrap(),
|
||||
workspace_roots: Vec::new(),
|
||||
config: EnvironmentConfigState::FromThread,
|
||||
},
|
||||
error: "界".repeat(300),
|
||||
})
|
||||
.collect(),
|
||||
};
|
||||
let states = environment_states(&snapshot);
|
||||
let details: Vec<_> = states
|
||||
.values()
|
||||
.map(|state| state.error.as_deref())
|
||||
.collect();
|
||||
let truncated = "界".repeat(85);
|
||||
assert_eq!(
|
||||
details,
|
||||
vec![
|
||||
Some(truncated.as_str()),
|
||||
Some(truncated.as_str()),
|
||||
None,
|
||||
None
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
@@ -818,11 +818,11 @@ impl ThreadEnvironments {
|
||||
let selected = self.environments.load_full();
|
||||
let mut environments = Vec::with_capacity(selected.len());
|
||||
for environment in selected.iter() {
|
||||
if matches!(
|
||||
environment.selection.config,
|
||||
EnvironmentConfigState::Failed(_)
|
||||
) {
|
||||
environments.push(TurnEnvironmentState::Failed);
|
||||
if let EnvironmentConfigState::Failed(error) = &environment.selection.config {
|
||||
environments.push(TurnEnvironmentState::Failed {
|
||||
selection: environment.selection.clone(),
|
||||
error: error.clone(),
|
||||
});
|
||||
continue;
|
||||
}
|
||||
let pending = matches!(
|
||||
@@ -857,7 +857,10 @@ pub(crate) enum TurnEnvironmentState {
|
||||
Ready(TurnEnvironment),
|
||||
Starting(StartingTurnEnvironment),
|
||||
/// Unavailable for execution, but still selected when evaluating permissions.
|
||||
Failed,
|
||||
Failed {
|
||||
selection: TurnEnvironmentSelection,
|
||||
error: String,
|
||||
},
|
||||
}
|
||||
|
||||
impl TurnEnvironmentState {
|
||||
@@ -870,7 +873,10 @@ impl TurnEnvironmentState {
|
||||
let mut selection = starting.selection;
|
||||
if matches!(selection.config, EnvironmentConfigState::Pending) {
|
||||
let Some(config) = environment.installed_config else {
|
||||
return Self::Failed;
|
||||
return Self::Failed {
|
||||
selection,
|
||||
error: "Environment configuration was not supplied.".to_string(),
|
||||
};
|
||||
};
|
||||
selection.config = EnvironmentConfigState::Ready(config);
|
||||
}
|
||||
@@ -896,7 +902,10 @@ impl TurnEnvironmentState {
|
||||
environment_id = %starting.selection.environment_id,
|
||||
"skipping failed turn environment: {err}"
|
||||
);
|
||||
Self::Failed
|
||||
Self::Failed {
|
||||
selection: starting.selection,
|
||||
error: err.to_string(),
|
||||
}
|
||||
}
|
||||
None => Self::Starting(starting),
|
||||
}
|
||||
@@ -925,7 +934,7 @@ impl TurnEnvironmentSnapshot {
|
||||
.iter()
|
||||
.map(|environment| match environment {
|
||||
TurnEnvironmentState::Ready(environment) => &environment.selection.config,
|
||||
TurnEnvironmentState::Starting(_) | TurnEnvironmentState::Failed => {
|
||||
TurnEnvironmentState::Starting(_) | TurnEnvironmentState::Failed { .. } => {
|
||||
&EnvironmentConfigState::Pending
|
||||
}
|
||||
}),
|
||||
@@ -947,7 +956,7 @@ impl TurnEnvironmentSnapshot {
|
||||
environment.resolution.clone().now_or_never(),
|
||||
)
|
||||
}
|
||||
TurnEnvironmentState::Failed => TurnEnvironmentState::Failed,
|
||||
TurnEnvironmentState::Failed { .. } => environment.clone(),
|
||||
})
|
||||
.collect();
|
||||
Self { environments }
|
||||
@@ -1022,7 +1031,7 @@ impl TurnEnvironmentSnapshot {
|
||||
{
|
||||
environment.selection.cwd.to_abs_path().ok()
|
||||
}
|
||||
TurnEnvironmentState::Starting(_) | TurnEnvironmentState::Failed => None,
|
||||
TurnEnvironmentState::Starting(_) | TurnEnvironmentState::Failed { .. } => None,
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -7,6 +7,7 @@ use serde::Deserialize;
|
||||
use serde_json::json;
|
||||
use std::collections::BTreeMap;
|
||||
|
||||
use crate::environment_selection::TurnEnvironmentState;
|
||||
use crate::function_tool::FunctionCallError;
|
||||
use crate::tools::context::ToolInvocation;
|
||||
use crate::tools::context::ToolPayload;
|
||||
@@ -128,6 +129,22 @@ impl ToolExecutor<ToolInvocation> for WaitForEnvironmentHandler {
|
||||
.turn_environments()
|
||||
.any(|environment| environment.selection.environment_id == environment_id);
|
||||
if !already_ready {
|
||||
if let Some(error) =
|
||||
step_context
|
||||
.environments
|
||||
.environments
|
||||
.iter()
|
||||
.find_map(|state| match state {
|
||||
TurnEnvironmentState::Failed { selection, error }
|
||||
if selection.environment_id == environment_id =>
|
||||
{
|
||||
Some(error)
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
{
|
||||
return Err(environment_failure(&environment_id, error));
|
||||
}
|
||||
let Some(environment) = step_context
|
||||
.environments
|
||||
.starting()
|
||||
@@ -139,11 +156,10 @@ impl ToolExecutor<ToolInvocation> for WaitForEnvironmentHandler {
|
||||
)));
|
||||
};
|
||||
|
||||
environment.wait_until_ready().await.map_err(|_| {
|
||||
FunctionCallError::RespondToModel(format!(
|
||||
"Environment `{environment_id}` failed to start and is unavailable. Continue without it."
|
||||
))
|
||||
})?;
|
||||
environment
|
||||
.wait_until_ready()
|
||||
.await
|
||||
.map_err(|error| environment_failure(&environment_id, &error.to_string()))?;
|
||||
}
|
||||
|
||||
Ok(boxed_tool_output(JsonToolOutput::new(json!({
|
||||
@@ -155,3 +171,12 @@ impl ToolExecutor<ToolInvocation> for WaitForEnvironmentHandler {
|
||||
}
|
||||
|
||||
impl CoreToolRuntime for WaitForEnvironmentHandler {}
|
||||
|
||||
// Keep failure output small while preserving a UTF-8 boundary.
|
||||
fn environment_failure(environment_id: &str, error: &str) -> FunctionCallError {
|
||||
const MAX_ERROR_BYTES: usize = 256;
|
||||
let reason = &error[..error.floor_char_boundary(MAX_ERROR_BYTES)];
|
||||
FunctionCallError::RespondToModel(format!(
|
||||
"Environment `{environment_id}` failed to start: {reason}"
|
||||
))
|
||||
}
|
||||
|
||||
@@ -4023,3 +4023,6 @@ async fn remote_test_env_copy_preserves_symlink_source() -> Result<()> {
|
||||
.await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[path = "remote_env_failure_tests.rs"]
|
||||
mod failure_tests;
|
||||
|
||||
@@ -0,0 +1,106 @@
|
||||
//! Provisioning failures reach the model without terminating its turn.
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
use test_case::test_case;
|
||||
|
||||
#[test_case(true; "failed_before_turn")]
|
||||
#[test_case(false; "failed_while_waiting")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn provisioning_failure_reaches_model_and_turn_continues(
|
||||
fail_before_turn: bool,
|
||||
) -> Result<()> {
|
||||
const REASON: &str = "This repository is empty. Push an initial commit, then retry.";
|
||||
const CALL_ID: &str = "wait-for-failed-environment";
|
||||
let server = start_mock_server().await;
|
||||
let response_mock = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_function_call(
|
||||
CALL_ID,
|
||||
"wait_for_environment",
|
||||
&json!({
|
||||
"environment_id": REMOTE_ENVIRONMENT_ID,
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_assistant_message(
|
||||
"message-1",
|
||||
"I can explain how to initialize the repository.",
|
||||
),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
let mut builder = test_codex_with_wait_for_environment().with_config(|config| {
|
||||
assert!(config.features.enable(Feature::DeferredExecutor).is_ok());
|
||||
});
|
||||
let test = expect_startup(builder.build(&server)).await;
|
||||
let manager = test.thread_manager.environment_manager();
|
||||
let provider = Arc::new(FailingNoiseConnectProvider::default());
|
||||
manager.materialize_pending_noise_environment(
|
||||
REMOTE_ENVIRONMENT_ID.to_string(),
|
||||
provider.clone(),
|
||||
)?;
|
||||
if fail_before_turn {
|
||||
manager.report_environment_provisioning_status(
|
||||
REMOTE_ENVIRONMENT_ID.to_string(),
|
||||
Err(REASON.to_string()),
|
||||
provider.clone(),
|
||||
)?;
|
||||
}
|
||||
test.codex
|
||||
.start_or_steer_turn(
|
||||
TurnInputRequest::user_input(vec![UserInput::Text {
|
||||
text: "inspect the repository".into(),
|
||||
text_elements: Vec::new(),
|
||||
}])
|
||||
.with_thread_settings(ThreadSettingsOverrides {
|
||||
environments: Some(TurnEnvironmentSelections::new(
|
||||
test.config.cwd.clone(),
|
||||
vec![TurnEnvironmentSelection {
|
||||
environment_id: REMOTE_ENVIRONMENT_ID.to_string(),
|
||||
cwd: PathUri::from_abs_path(&test.config.cwd),
|
||||
workspace_roots: vec![PathUri::from_abs_path(&test.config.cwd)],
|
||||
config: EnvironmentConfigState::FromThread,
|
||||
}],
|
||||
)),
|
||||
..Default::default()
|
||||
}),
|
||||
)
|
||||
.await?;
|
||||
wait_for_response_request_count(&response_mock, /*expected_count*/ 1).await;
|
||||
if !fail_before_turn {
|
||||
manager.report_environment_provisioning_status(
|
||||
REMOTE_ENVIRONMENT_ID.to_string(),
|
||||
Err(REASON.to_string()),
|
||||
provider,
|
||||
)?;
|
||||
}
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::TurnComplete(_))
|
||||
})
|
||||
.await;
|
||||
let requests = response_mock.requests();
|
||||
assert_eq!(requests.len(), 2);
|
||||
let (output, _) = requests[1]
|
||||
.function_call_output_content_and_success(CALL_ID)
|
||||
.context("the model should receive the failed wait result")?;
|
||||
assert!(
|
||||
output.as_deref().is_some_and(|text| text.contains(REASON)),
|
||||
"{output:?}"
|
||||
);
|
||||
let context_request = if fail_before_turn {
|
||||
&requests[0]
|
||||
} else {
|
||||
&requests[1]
|
||||
};
|
||||
let context = context_request.message_input_texts("user").join("\n");
|
||||
assert!(context.contains("<status>failed</status>"), "{context}");
|
||||
assert!(context.contains(REASON), "{context}");
|
||||
Ok(())
|
||||
}
|
||||
Reference in New Issue
Block a user