Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions codex-rs/codex-home/src/instructions/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,6 @@ impl CodexHomeUserInstructionsProvider {
continue;
}
};
if let Err(err) = std::str::from_utf8(&data) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this warning wasn't worth the complexity of carrying it around

warnings.push(format!(
"Global AGENTS.md instructions from `{}` contain invalid UTF-8: {err}. Invalid byte sequences were replaced.",
path.display()
));
}
let contents = String::from_utf8_lossy(&data);
let trimmed = contents.trim();
if !trimmed.is_empty() {
Expand Down
9 changes: 2 additions & 7 deletions codex-rs/codex-home/src/instructions/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ async fn recoverable_override_read_error_warns_and_falls_back_to_default() {
}

#[tokio::test]
async fn invalid_utf8_is_lossy_and_warned() {
async fn invalid_utf8_is_lossy() {
let home = TempDir::new().expect("temp dir");
let path = home.path().join(DEFAULT_AGENTS_MD_FILENAME);
let mut invalid_utf8 = b"global".to_vec();
Expand All @@ -135,18 +135,13 @@ async fn invalid_utf8_is_lossy_and_warned() {
fs::write(&path, &invalid_utf8).expect("write invalid utf-8");

let outcome = provider(&home).load_user_instructions().await;
let utf8_error = std::str::from_utf8(&invalid_utf8).expect_err("invalid utf-8");
let warning = format!(
"Global AGENTS.md instructions from `{}` contain invalid UTF-8: {utf8_error}. Invalid byte sequences were replaced.",
path.display(),
);
assert_eq!(
outcome,
expected(
&home,
DEFAULT_AGENTS_MD_FILENAME,
"global\u{fffd} doc",
vec![warning]
Vec::new()
)
);
}
22 changes: 12 additions & 10 deletions codex-rs/core/src/agent/control.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ use crate::agent::role::resolve_role_config;
use crate::agent::status::is_final;
use crate::codex_thread::ThreadConfigSnapshot;
use crate::config::Config;
use crate::environment_selection::TurnEnvironmentSnapshot;
use crate::session::emit_subagent_session_started;
use crate::session_prefix::format_subagent_context_line;
use crate::session_prefix::format_subagent_notification_message;
use crate::shell_snapshot::ShellSnapshot;
use crate::thread_manager::ResumeThreadWithHistoryOptions;
use crate::thread_manager::ThreadManagerState;
use crate::thread_rollout_truncation::truncate_rollout_to_last_n_fork_turns;
Expand Down Expand Up @@ -519,11 +519,11 @@ impl AgentControl {
.ok_or_else(|| CodexErr::UnsupportedOperation("thread manager dropped".to_string()))
}

async fn inherited_shell_snapshot_for_source(
async fn inherited_environments_for_source(
&self,
state: &Arc<ThreadManagerState>,
session_source: Option<&SessionSource>,
) -> Option<Arc<ShellSnapshot>> {
) -> Option<TurnEnvironmentSnapshot> {
let Some(SessionSource::SubAgent(SubAgentSource::ThreadSpawn {
parent_thread_id, ..
})) = session_source
Expand All @@ -532,13 +532,15 @@ impl AgentControl {
};

let parent_thread = state.get_thread(*parent_thread_id).await.ok()?;
let snapshot = parent_thread
.codex
.session
.services
.shell_snapshot
.load_full()?;
(!snapshot.is_failed()).then_some(snapshot)
Some(
parent_thread
.codex
.session
.services
.turn_environments
.snapshot()
.await,
Comment thread
pakrym-oai marked this conversation as resolved.
Comment on lines +540 to +542

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Add integration coverage for subagent environment inheritance

This agent logic now inherits the parent TurnEnvironmentSnapshot, but I found no core/suite test_codex coverage that spawns a subagent and verifies inherited turn environments/shell snapshots; please add it, per guidance.

Useful? React with 👍 / 👎.

)
}

async fn inherited_exec_policy_for_source(
Expand Down
2 changes: 1 addition & 1 deletion codex-rs/core/src/agent/control/residency_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ async fn spawn_v2_subagent(
/*forked_from_thread_id*/ None,
Some(ThreadSource::Subagent),
/*metrics_service_name*/ None,
/*inherited_shell_snapshot*/ None,
/*inherited_environments*/ None,
/*inherited_exec_policy*/ None,
/*environments*/ None,
)
Expand Down
24 changes: 12 additions & 12 deletions codex-rs/core/src/agent/control/spawn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use super::*;
const AGENT_NAMES: &str = include_str!("../agent_names.txt");

struct SpawnAgentThreadInheritance {
shell_snapshot: Option<Arc<ShellSnapshot>>,
environments: Option<TurnEnvironmentSnapshot>,
exec_policy: Option<Arc<crate::exec_policy::ExecPolicyManager>>,
}

Expand Down Expand Up @@ -156,8 +156,8 @@ impl AgentControl {
let parent_thread_id = initial_history
.get_resumed_parent_thread_id()
.or(stored_parent_thread_id);
let inherited_shell_snapshot = self
.inherited_shell_snapshot_for_source(&state, Some(&session_source))
let inherited_environments = self
.inherited_environments_for_source(&state, Some(&session_source))
.await;
let inherited_exec_policy = self
.inherited_exec_policy_for_source(&state, Some(&session_source), &config)
Expand All @@ -170,7 +170,7 @@ impl AgentControl {
agent_control: self.clone(),
session_source,
parent_thread_id,
inherited_shell_snapshot,
inherited_environments,
inherited_exec_policy,
})
.await
Expand Down Expand Up @@ -231,8 +231,8 @@ impl AgentControl {
};
let mut reservation = self.state.reserve_spawn_slot(reservation_max_threads)?;
let inheritance = SpawnAgentThreadInheritance {
shell_snapshot: self
.inherited_shell_snapshot_for_source(&state, session_source.as_ref())
environments: self
.inherited_environments_for_source(&state, session_source.as_ref())
.await,
exec_policy: self
.inherited_exec_policy_for_source(&state, session_source.as_ref(), &config)
Expand Down Expand Up @@ -283,7 +283,7 @@ impl AgentControl {
/*forked_from_thread_id*/ None,
/*thread_source*/ Some(ThreadSource::Subagent),
/*metrics_service_name*/ None,
inheritance.shell_snapshot,
inheritance.environments,
inheritance.exec_policy,
options.environments.clone(),
))
Expand Down Expand Up @@ -386,7 +386,7 @@ impl AgentControl {
multi_agent_version: MultiAgentVersion,
) -> CodexResult<crate::thread_manager::NewThread> {
let SpawnAgentThreadInheritance {
shell_snapshot: inherited_shell_snapshot,
environments: inherited_environments,
exec_policy: inherited_exec_policy,
} = inheritance;
if options.fork_parent_spawn_call_id.is_none() {
Expand Down Expand Up @@ -513,7 +513,7 @@ impl AgentControl {
/*thread_source*/ Some(ThreadSource::Subagent),
/*parent_thread_id*/ Some(parent_thread_id),
/*forked_from_thread_id*/ Some(parent_thread_id),
inherited_shell_snapshot,
inherited_environments,
inherited_exec_policy,
options.environments.clone(),
)
Expand Down Expand Up @@ -665,8 +665,8 @@ impl AgentControl {
other => (other, AgentMetadata::default()),
};
let notification_source = session_source.clone();
let inherited_shell_snapshot = self
.inherited_shell_snapshot_for_source(&state, Some(&session_source))
let inherited_environments = self
.inherited_environments_for_source(&state, Some(&session_source))
.await;
let inherited_exec_policy = self
.inherited_exec_policy_for_source(&state, Some(&session_source), &config)
Expand All @@ -679,7 +679,7 @@ impl AgentControl {
agent_control: self.clone(),
session_source,
parent_thread_id,
inherited_shell_snapshot,
inherited_environments,
inherited_exec_policy,
})
.await?;
Expand Down
20 changes: 2 additions & 18 deletions codex-rs/core/src/agents_md.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ const AGENTS_MD_SEPARATOR: &str = "\n\n--- project-doc ---\n\n";
/// Loads project AGENTS.md content and combines it with host-provided user
/// instructions.
pub(crate) async fn load_project_instructions(
config: &mut Config,
config: &Config,
user_instructions: Option<UserInstructions>,
environments: &TurnEnvironmentSnapshot,
) -> Option<LoadedAgentsMd> {
Expand Down Expand Up @@ -89,7 +89,7 @@ pub(crate) async fn load_project_instructions(
/// `Ok(None)`. Unexpected I/O failures bubble up as `Err` so callers can
/// decide how to handle them.
async fn read_agents_md(
config: &mut Config,
config: &Config,
fs: &dyn ExecutorFileSystem,
environment_id: &str,
cwd: &AbsolutePathBuf,
Expand Down Expand Up @@ -126,8 +126,6 @@ async fn read_agents_md(
Err(err) if err.kind() == io::ErrorKind::NotFound => continue,
Err(err) => return Err(err),
};
warn_invalid_utf8(&p, &data, "Project", &mut config.startup_warnings);

let size = data.len() as u64;
if size > remaining {
data.truncate(remaining as usize);
Expand Down Expand Up @@ -503,20 +501,6 @@ impl InstructionProvenance {
}
}

fn warn_invalid_utf8(
path: &AbsolutePathBuf,
data: &[u8],
source: &str,
startup_warnings: &mut Vec<String>,
) {
if let Err(err) = std::str::from_utf8(data) {
startup_warnings.push(format!(
"{source} AGENTS.md instructions from `{}` contain invalid UTF-8: {err}. Invalid byte sequences were replaced.",
path.display()
));
}
}

#[cfg(test)]
#[path = "agents_md_tests.rs"]
mod tests;
Loading
Loading