Skip to content

feat: Better OS Detection, starting with windows #6046

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+201 −53
Open

feat: Better OS Detection, starting with windows #6046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9f70971
feat: Better OS Detection, starting with windows
dylan-hurd-oai Oct 31, 2025
86d636c
core: exclude OS from env_context diffs; update tests for Windows/WSL…
dylan-hurd-oai Nov 4, 2025
c9c5d69
OnceLock
dylan-hurd-oai Nov 4, 2025
78aafa4
rebase fix
dylan-hurd-oai Nov 4, 2025
66858e4
fix tests
dylan-hurd-oai Nov 4, 2025
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
188 changes: 157 additions & 31 deletions codex-rs/core/src/environment_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ pub enum NetworkAccess {
Restricted,
Enabled,
}

#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
pub struct OperatingSystemInfo {
pub name: String,
pub version: String,
pub is_likely_windows_subsystem_for_linux: Option<bool>,
}

#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
#[serde(rename = "environment_context", rename_all = "snake_case")]
pub(crate) struct EnvironmentContext {
Expand All @@ -29,6 +37,7 @@ pub(crate) struct EnvironmentContext {
pub network_access: Option<NetworkAccess>,
pub writable_roots: Option<Vec<PathBuf>>,
pub shell: Option<Shell>,
pub operating_system: Option<OperatingSystemInfo>,
}

impl EnvironmentContext {
Expand Down Expand Up @@ -70,6 +79,7 @@ impl EnvironmentContext {
_ => None,
},
shell,
operating_system: Self::operating_system_info(),
}
}

Expand All @@ -83,6 +93,7 @@ impl EnvironmentContext {
sandbox_mode,
network_access,
writable_roots,
operating_system,
// should compare all fields except shell
shell: _,
} = other;
Expand All @@ -92,6 +103,7 @@ impl EnvironmentContext {
&& self.sandbox_mode == *sandbox_mode
&& self.network_access == *network_access
&& self.writable_roots == *writable_roots
&& self.operating_system == *operating_system
}

pub fn diff(before: &TurnContext, after: &TurnContext) -> Self {
Expand All @@ -110,7 +122,12 @@ impl EnvironmentContext {
} else {
None
};
EnvironmentContext::new(cwd, approval_policy, sandbox_policy, None)
// Diff messages should only include fields that changed between turns.
// Operating system is a static property of the host and should not be
// emitted as part of a per-turn diff.
let mut ec = EnvironmentContext::new(cwd, approval_policy, sandbox_policy, None);
Copy link
Contributor

@iceweasel-oai iceweasel-oai Nov 5, 2025

Choose a reason for hiding this comment

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

nit: this feels a bit awkward. Something like EnvironmentContext::init_without_os(...) would be nice

Copy link

@Waxime64 Waxime64 Nov 13, 2025

Choose a reason for hiding this comment

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

fn new_without_operating_system(
    cwd: Option<PathBuf>,
    approval_policy: Option<AskForApproval>,
    sandbox_policy: Option<SandboxPolicy>,
    shell: Option<Shell>,
) -> Self {
    let mut ec = Self::new(cwd, approval_policy, sandbox_policy, shell);
    ec.operating_system = None;
    ec
}

EnvironmentContext::new_without_operating_system(cwd, approval_policy, sandbox_policy, None)

ec.operating_system = None;
ec
}
}

Expand Down Expand Up @@ -141,10 +158,11 @@ impl EnvironmentContext {
/// <shell>...</shell>
/// </environment_context>
/// ```
pub fn serialize_to_xml(self) -> String {
pub fn serialize_to_xml(&self) -> String {
let mut lines = vec![ENVIRONMENT_CONTEXT_OPEN_TAG.to_string()];
if let Some(cwd) = self.cwd {
lines.push(format!(" <cwd>{}</cwd>", cwd.to_string_lossy()));
if let Some(cwd) = self.cwd.as_ref() {
let cwd = cwd.to_string_lossy();
lines.push(format!(" <cwd>{cwd}</cwd>"));
}
if let Some(approval_policy) = self.approval_policy {
lines.push(format!(
Expand All @@ -154,29 +172,44 @@ impl EnvironmentContext {
if let Some(sandbox_mode) = self.sandbox_mode {
lines.push(format!(" <sandbox_mode>{sandbox_mode}</sandbox_mode>"));
}
if let Some(network_access) = self.network_access {
if let Some(network_access) = self.network_access.as_ref() {
lines.push(format!(
" <network_access>{network_access}</network_access>"
));
}
if let Some(writable_roots) = self.writable_roots {
if let Some(writable_roots) = self.writable_roots.as_ref() {
lines.push(" <writable_roots>".to_string());
for writable_root in writable_roots {
lines.push(format!(
" <root>{}</root>",
writable_root.to_string_lossy()
));
let writable_root = writable_root.to_string_lossy();
lines.push(format!(" <root>{writable_root}</root>"));
}
lines.push(" </writable_roots>".to_string());
}
if let Some(shell) = self.shell
if let Some(shell) = self.shell.as_ref()
&& let Some(shell_name) = shell.name()
{
lines.push(format!(" <shell>{shell_name}</shell>"));
}
if let Some(operating_system) = self.operating_system.as_ref() {
lines.push(" <operating_system>".to_string());
let name = operating_system.name.as_str();
lines.push(format!(" <name>{name}</name>"));
let version = operating_system.version.as_str();
lines.push(format!(" <version>{version}</version>"));
if let Some(is_wsl) = operating_system.is_likely_windows_subsystem_for_linux {
lines.push(format!(
" <is_likely_windows_subsystem_for_linux>{is_wsl}</is_likely_windows_subsystem_for_linux>"
));
}
lines.push(" </operating_system>".to_string());
}
lines.push(ENVIRONMENT_CONTEXT_CLOSE_TAG.to_string());
lines.join("\n")
}

fn operating_system_info() -> Option<OperatingSystemInfo> {
operating_system_info_impl()
}
}

impl From<EnvironmentContext> for ResponseItem {
Expand All @@ -191,13 +224,106 @@ impl From<EnvironmentContext> for ResponseItem {
}
}

// Restrict Operating System Info to Windows and Linux inside WSL for now
#[cfg(target_os = "windows")]
fn operating_system_info_impl() -> Option<OperatingSystemInfo> {
let info = os_info::get();
Some(OperatingSystemInfo {
name: info.os_type().to_string(),
version: info.version().to_string(),
is_likely_windows_subsystem_for_linux: Some(has_wsl_env_markers()),
Copy link
Contributor

@iceweasel-oai iceweasel-oai Nov 5, 2025

Choose a reason for hiding this comment

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

is this right? If they are running WSL, wouldn't target_os not be windows?

Copy link
Collaborator Author

@dylan-hurd-oai dylan-hurd-oai Nov 5, 2025

Choose a reason for hiding this comment

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

You can technically execute codex.exe from within WSL, since WSL supports interop for executables. I think we should generally discourage this, since many things won't work, but it is possible.

})
}

#[cfg(all(unix, not(target_os = "macos")))]
fn operating_system_info_impl() -> Option<OperatingSystemInfo> {
let info = os_info::get();
match has_wsl_env_markers() {
true => Some(OperatingSystemInfo {
name: info.os_type().to_string(),
version: info.version().to_string(),
is_likely_windows_subsystem_for_linux: Some(true),
}),
false => None,
}
}

#[cfg(target_os = "macos")]
fn operating_system_info_impl() -> Option<OperatingSystemInfo> {
None
}

#[cfg(not(target_os = "macos"))]
fn has_wsl_env_markers() -> bool {
// Cache detection result since env vars are stable across process lifetime
// and this function may be called multiple times.
static CACHE: std::sync::OnceLock<bool> = std::sync::OnceLock::new();
*CACHE.get_or_init(|| {
std::env::var_os("WSL_INTEROP").is_some()
|| std::env::var_os("WSLENV").is_some()
|| std::env::var_os("WSL_DISTRO_NAME").is_some()
})
}

#[cfg(test)]
mod tests {
use crate::shell::BashShell;
use crate::shell::ZshShell;

use super::*;
use pretty_assertions::assert_eq;
fn expected_environment_context(mut body_lines: Vec<String>) -> String {
let mut lines = vec!["<environment_context>".to_string()];
lines.append(&mut body_lines);
if let Some(os) = EnvironmentContext::operating_system_info() {
lines.push(" <operating_system>".to_string());
lines.push(format!(" <name>{}</name>", os.name));
lines.push(format!(" <version>{}</version>", os.version));
if let Some(is_wsl) = os.is_likely_windows_subsystem_for_linux {
lines.push(format!(
" <is_likely_windows_subsystem_for_linux>{is_wsl}</is_likely_windows_subsystem_for_linux>"
));
}
lines.push(" </operating_system>".to_string());
}
lines.push("</environment_context>".to_string());
lines.join("\n")
}

#[cfg(target_os = "windows")]
#[test]
fn operating_system_info_on_windows_includes_os_details() {
let info = operating_system_info_impl().expect("expected Windows operating system info");
let os_details = os_info::get();

assert_eq!(info.name, os_details.os_type().to_string());
assert_eq!(info.version, os_details.version().to_string());
assert_eq!(
info.is_likely_windows_subsystem_for_linux,
Some(has_wsl_env_markers())
);
}

#[cfg(all(unix, not(target_os = "macos")))]
#[test]
fn operating_system_info_matches_wsl_detection_on_unix() {
let info = operating_system_info_impl();
let os_details = os_info::get();
if has_wsl_env_markers() {
let info = info.expect("expected WSL operating system info");
assert_eq!(info.name, os_details.os_type().to_string());
assert_eq!(info.version, os_details.version().to_string());
assert_eq!(info.is_likely_windows_subsystem_for_linux, Some(true));
Copy link
Collaborator

@pakrym-oai pakrym-oai Nov 4, 2025

Choose a reason for hiding this comment

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

why do we need both name and is_likely_windows_subsystem_for_linux ?

Copy link
Collaborator Author

@dylan-hurd-oai dylan-hurd-oai Nov 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Updated this - should be os_info.name, not "Windows Subsystem for Linux" 🤦

When running inside WSL, the os_info will return a linux distribution, e.g. Ubuntu. But it's still helpful to know that this is running inside WSL and the actual machine you're on is a Windows machine.

} else {
assert_eq!(info, None);
}
}

#[cfg(target_os = "macos")]
#[test]
fn operating_system_info_is_none_on_macos() {
assert_eq!(operating_system_info_impl(), None);
}

fn workspace_write_policy(writable_roots: Vec<&str>, network_access: bool) -> SandboxPolicy {
SandboxPolicy::WorkspaceWrite {
Expand All @@ -217,16 +343,16 @@ mod tests {
None,
);

let expected = r#"<environment_context>
<cwd>/repo</cwd>
<approval_policy>on-request</approval_policy>
<sandbox_mode>workspace-write</sandbox_mode>
<network_access>restricted</network_access>
<writable_roots>
<root>/repo</root>
<root>/tmp</root>
</writable_roots>
</environment_context>"#;
let expected = expected_environment_context(vec![
" <cwd>/repo</cwd>".to_string(),
" <approval_policy>on-request</approval_policy>".to_string(),
" <sandbox_mode>workspace-write</sandbox_mode>".to_string(),
" <network_access>restricted</network_access>".to_string(),
" <writable_roots>".to_string(),
" <root>/repo</root>".to_string(),
" <root>/tmp</root>".to_string(),
" </writable_roots>".to_string(),
]);

assert_eq!(context.serialize_to_xml(), expected);
}
Expand All @@ -240,11 +366,11 @@ mod tests {
None,
);

let expected = r#"<environment_context>
<approval_policy>never</approval_policy>
<sandbox_mode>read-only</sandbox_mode>
<network_access>restricted</network_access>
</environment_context>"#;
let expected = expected_environment_context(vec![
" <approval_policy>never</approval_policy>".to_string(),
" <sandbox_mode>read-only</sandbox_mode>".to_string(),
" <network_access>restricted</network_access>".to_string(),
]);

assert_eq!(context.serialize_to_xml(), expected);
}
Expand All @@ -258,11 +384,11 @@ mod tests {
None,
);

let expected = r#"<environment_context>
<approval_policy>on-failure</approval_policy>
<sandbox_mode>danger-full-access</sandbox_mode>
<network_access>enabled</network_access>
</environment_context>"#;
let expected = expected_environment_context(vec![
" <approval_policy>on-failure</approval_policy>".to_string(),
" <sandbox_mode>danger-full-access</sandbox_mode>".to_string(),
" <network_access>enabled</network_access>".to_string(),
]);

assert_eq!(context.serialize_to_xml(), expected);
}
Expand Down
66 changes: 44 additions & 22 deletions codex-rs/core/tests/suite/prompt_caching.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,53 @@ fn text_user_input(text: String) -> serde_json::Value {
})
}

#[allow(dead_code)]
fn has_wsl_env_markers() -> bool {
Copy link
Contributor

@iceweasel-oai iceweasel-oai Nov 5, 2025

Choose a reason for hiding this comment

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

would be nice to have one version of this (also this one doesn't cache the output?)

std::env::var_os("WSL_INTEROP").is_some()
|| std::env::var_os("WSLENV").is_some()
|| std::env::var_os("WSL_DISTRO_NAME").is_some()
}

fn operating_system_context_block() -> String {
#[cfg(target_os = "windows")]
{
let info = os_info::get();
let name = info.os_type().to_string();
let version = info.version().to_string();
let is_wsl = has_wsl_env_markers();
format!(
" <operating_system>\n <name>{name}</name>\n <version>{version}</version>\n <is_likely_windows_subsystem_for_linux>{is_wsl}</is_likely_windows_subsystem_for_linux>\n </operating_system>\n"
)
}

#[cfg(all(unix, not(target_os = "macos")))]
{
if has_wsl_env_markers() {
" <operating_system>\n <name>{name}</name>\n <version></version>\n <is_likely_windows_subsystem_for_linux>true</is_likely_windows_subsystem_for_linux>\n </operating_system>\n".to_string()
} else {
String::new()
}
}

#[cfg(target_os = "macos")]
{
String::new()
}
}

fn default_env_context_str(cwd: &str, shell: &Shell) -> String {
let shell_line = match shell.name() {
Some(name) => format!(" <shell>{name}</shell>\n"),
None => String::new(),
};
let os_block = operating_system_context_block();
format!(
r#"<environment_context>
<cwd>{}</cwd>
<cwd>{cwd}</cwd>
<approval_policy>on-request</approval_policy>
<sandbox_mode>read-only</sandbox_mode>
<network_access>restricted</network_access>
{}</environment_context>"#,
cwd,
match shell.name() {
Some(name) => format!(" <shell>{name}</shell>\n"),
None => String::new(),
}
{shell_line}{os_block}</environment_context>"#
)
}

Expand Down Expand Up @@ -341,22 +375,10 @@ async fn prefixes_context_and_instructions_once_and_consistently_across_requests

let shell = default_user_shell().await;

let expected_env_text = format!(
r#"<environment_context>
<cwd>{}</cwd>
<approval_policy>on-request</approval_policy>
<sandbox_mode>read-only</sandbox_mode>
<network_access>restricted</network_access>
{}</environment_context>"#,
cwd.path().to_string_lossy(),
match shell.name() {
Some(name) => format!(" <shell>{name}</shell>\n"),
None => String::new(),
}
);
let cwd_str = cwd.path().to_string_lossy().into_owned();
let expected_env_text = default_env_context_str(&cwd_str, &shell);
let expected_ui_text = format!(
"# AGENTS.md instructions for {}\n\n<INSTRUCTIONS>\nbe consistent and helpful\n</INSTRUCTIONS>",
cwd.path().to_string_lossy()
"# AGENTS.md instructions for {cwd_str}\n\n<INSTRUCTIONS>\nbe consistent and helpful\n</INSTRUCTIONS>"
);

let expected_env_msg = serde_json::json!({
Expand Down
Loading