mirror of
https://github.com/openai/codex.git
synced 2026-11-10 21:00:47 +00:00
Merge remote-tracking branch 'origin/main' into daniels-oai/approval-requirements-fallback-tests
This commit is contained in:
@@ -5,6 +5,7 @@
|
||||
</p>
|
||||
</br>
|
||||
If you want Codex in your code editor (VS Code, Cursor, Windsurf), <a href="https://developers.openai.com/codex/ide">install in your IDE.</a>
|
||||
</br>If you want the desktop app experience, run <code>codex app</code> or visit <a href="https://chatgpt.com/codex?app-landing-page=true">the Codex App page</a>.
|
||||
</br>If you are looking for the <em>cloud-based agent</em> from OpenAI, <strong>Codex Web</strong>, go to <a href="https://chatgpt.com/codex">chatgpt.com/codex</a>.</p>
|
||||
|
||||
---
|
||||
|
||||
@@ -41,6 +41,7 @@ fn preset_to_info(preset: &ModelPreset, priority: i32) -> ModelInfo {
|
||||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -89,6 +89,7 @@ async fn models_client_hits_models_endpoint() {
|
||||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
}],
|
||||
};
|
||||
|
||||
|
||||
@@ -233,6 +233,9 @@
|
||||
"memory_tool": {
|
||||
"type": "boolean"
|
||||
},
|
||||
"multi_agent": {
|
||||
"type": "boolean"
|
||||
},
|
||||
"personality": {
|
||||
"type": "boolean"
|
||||
},
|
||||
@@ -1363,6 +1366,9 @@
|
||||
"memory_tool": {
|
||||
"type": "boolean"
|
||||
},
|
||||
"multi_agent": {
|
||||
"type": "boolean"
|
||||
},
|
||||
"personality": {
|
||||
"type": "boolean"
|
||||
},
|
||||
|
||||
@@ -0,0 +1,2 @@
|
||||
model = "gpt-5.1-codex-mini"
|
||||
model_reasoning_effort = "medium"
|
||||
@@ -0,0 +1,26 @@
|
||||
version = 1
|
||||
|
||||
[agents.default]
|
||||
description = "Default agent."
|
||||
|
||||
[agents.worker]
|
||||
description = """Use for execution and production work.
|
||||
Typical tasks:
|
||||
- Implement part of a feature
|
||||
- Fix tests or bugs
|
||||
- Split large refactors into independent chunks
|
||||
Rules:
|
||||
- Explicitly assign **ownership** of the task (files / responsibility).
|
||||
- Always tell workers they are **not alone in the codebase**, and they should ignore edits made by others without touching them."""
|
||||
|
||||
[agents.explorer]
|
||||
description = """Use `explorer` for all codebase questions.
|
||||
Explorers are fast and authoritative.
|
||||
Always prefer them over manual search or file reading.
|
||||
Rules:
|
||||
- Ask explorers first and precisely.
|
||||
- Do not re-read or re-search code they cover.
|
||||
- Trust explorer results without verification.
|
||||
- Run explorers in parallel when useful.
|
||||
- Reuse existing explorers for related questions."""
|
||||
config_file = "explorer.toml"
|
||||
@@ -8,5 +8,4 @@ pub(crate) use control::AgentControl;
|
||||
pub(crate) use guards::MAX_THREAD_SPAWN_DEPTH;
|
||||
pub(crate) use guards::exceeds_thread_spawn_depth_limit;
|
||||
pub(crate) use guards::next_thread_spawn_depth;
|
||||
pub(crate) use role::AgentRole;
|
||||
pub(crate) use status::agent_status_from_event;
|
||||
|
||||
+639
-115
@@ -1,132 +1,656 @@
|
||||
use crate::config::Config;
|
||||
use crate::protocol::SandboxPolicy;
|
||||
use codex_protocol::openai_models::ReasoningEffort;
|
||||
use crate::config::ConfigOverrides;
|
||||
use crate::config::deserialize_config_toml_with_base;
|
||||
use crate::config::find_codex_home;
|
||||
use crate::config_loader::ConfigLayerEntry;
|
||||
use crate::config_loader::ConfigLayerStack;
|
||||
use crate::config_loader::ConfigLayerStackOrdering;
|
||||
use codex_app_server_protocol::ConfigLayerSource;
|
||||
use serde::Deserialize;
|
||||
use serde::Serialize;
|
||||
use std::collections::BTreeMap;
|
||||
use std::collections::BTreeSet;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::LazyLock;
|
||||
use toml::Value as TomlValue;
|
||||
|
||||
/// Base instructions for the orchestrator role.
|
||||
const ORCHESTRATOR_PROMPT: &str = include_str!("../../templates/agents/orchestrator.md");
|
||||
/// Default model override used.
|
||||
// TODO(jif) update when we have something smarter.
|
||||
const EXPLORER_MODEL: &str = "gpt-5.1-codex-mini";
|
||||
const BUILT_IN_AGENTS_CONFIG: &str = include_str!("builtins_agents_config.toml");
|
||||
const BUILT_IN_EXPLORER_CONFIG: &str = include_str!("builtins/explorer.toml");
|
||||
|
||||
/// Enumerated list of all supported agent roles.
|
||||
const ALL_ROLES: [AgentRole; 3] = [
|
||||
AgentRole::Default,
|
||||
AgentRole::Explorer,
|
||||
AgentRole::Worker,
|
||||
// TODO(jif) add when we have stable prompts + models
|
||||
// AgentRole::Orchestrator,
|
||||
];
|
||||
const AGENTS_CONFIG_FILENAME: &str = "agents_config.toml";
|
||||
const AGENTS_CONFIG_SCHEMA_VERSION: u32 = 1;
|
||||
const DEFAULT_ROLE_NAME: &str = "default";
|
||||
const AGENT_TYPE_UNAVAILABLE_ERROR: &str = "agent type is currently not available";
|
||||
|
||||
/// Hard-coded agent role selection used when spawning sub-agents.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "snake_case")]
|
||||
pub enum AgentRole {
|
||||
/// Inherit the parent agent's configuration unchanged.
|
||||
Default,
|
||||
/// Coordination-only agent that delegates to workers.
|
||||
Orchestrator,
|
||||
/// Task-executing agent with a fixed model override.
|
||||
Worker,
|
||||
/// Task-executing agent with a fixed model override.
|
||||
Explorer,
|
||||
#[derive(Debug, Clone, Default, Deserialize)]
|
||||
#[serde(deny_unknown_fields)]
|
||||
struct AgentsConfigToml {
|
||||
version: Option<u32>,
|
||||
#[serde(default)]
|
||||
agents: BTreeMap<String, AgentDeclarationToml>,
|
||||
}
|
||||
|
||||
/// Immutable profile data that drives per-agent configuration overrides.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
|
||||
pub struct AgentProfile {
|
||||
/// Optional base instructions override.
|
||||
pub base_instructions: Option<&'static str>,
|
||||
/// Optional model override.
|
||||
pub model: Option<&'static str>,
|
||||
/// Optional reasoning effort override.
|
||||
pub reasoning_effort: Option<ReasoningEffort>,
|
||||
/// Whether to force a read-only sandbox policy.
|
||||
pub read_only: bool,
|
||||
/// Description to include in the tool specs.
|
||||
pub description: &'static str,
|
||||
#[derive(Debug, Clone, Default, Deserialize)]
|
||||
#[serde(deny_unknown_fields)]
|
||||
struct AgentDeclarationToml {
|
||||
/// Human-facing role documentation used in spawn tool guidance.
|
||||
description: Option<String>,
|
||||
/// Path to a role-specific config layer.
|
||||
config_file: Option<PathBuf>,
|
||||
}
|
||||
|
||||
impl AgentRole {
|
||||
/// Returns the string values used by JSON schema enums.
|
||||
pub fn enum_values() -> Vec<String> {
|
||||
ALL_ROLES
|
||||
/// Applies a role config layer to a mutable config and preserves unspecified keys.
|
||||
pub(crate) async fn apply_role_to_config(
|
||||
config: &mut Config,
|
||||
role_name: Option<&str>,
|
||||
) -> Result<(), String> {
|
||||
let role_name = role_name.unwrap_or(DEFAULT_ROLE_NAME);
|
||||
let built_in_agents_config = built_in::configs();
|
||||
let user_agents_config =
|
||||
user_defined::config(config.codex_home.as_path()).unwrap_or_else(|err| {
|
||||
tracing::warn!(
|
||||
agent_type = role_name,
|
||||
error = %err,
|
||||
"failed to load user-defined agents config; falling back to built-in roles"
|
||||
);
|
||||
AgentsConfigToml::default()
|
||||
});
|
||||
|
||||
let agent_config = if let Some(role) = user_agents_config.agents.get(role_name) {
|
||||
if let Some(config_file) = &role.config_file {
|
||||
let content = tokio::fs::read_to_string(config_file)
|
||||
.await
|
||||
.map_err(|err| {
|
||||
tracing::warn!("failed to read user-defined role config_file: {err:?}");
|
||||
AGENT_TYPE_UNAVAILABLE_ERROR.to_string()
|
||||
})?;
|
||||
let parsed: TomlValue = toml::from_str(&content).map_err(|err| {
|
||||
tracing::warn!("failed to read user-defined role config_file: {err:?}");
|
||||
AGENT_TYPE_UNAVAILABLE_ERROR.to_string()
|
||||
})?;
|
||||
Some(parsed)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
} else if let Some(role) = built_in_agents_config.agents.get(role_name) {
|
||||
if let Some(config_file) = &role.config_file {
|
||||
let content = built_in::config_file(config_file).ok_or_else(|| {
|
||||
tracing::warn!("failed to read user-defined role config_file.");
|
||||
AGENT_TYPE_UNAVAILABLE_ERROR.to_string()
|
||||
})?;
|
||||
let parsed: TomlValue = toml::from_str(content).map_err(|err| {
|
||||
tracing::warn!("failed to read user-defined role config_file: {err:?}");
|
||||
AGENT_TYPE_UNAVAILABLE_ERROR.to_string()
|
||||
})?;
|
||||
Some(parsed)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
} else {
|
||||
return Err(format!("unknown agent_type '{role_name}'"));
|
||||
};
|
||||
|
||||
let Some(agent_config) = agent_config else {
|
||||
return Ok(());
|
||||
};
|
||||
|
||||
let original = config.clone();
|
||||
let original_stack = &original.config_layer_stack;
|
||||
let mut layers = original
|
||||
.config_layer_stack
|
||||
.get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, true)
|
||||
.into_iter()
|
||||
.cloned()
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
let role_layer = ConfigLayerEntry::new(ConfigLayerSource::SessionFlags, agent_config);
|
||||
let role_layer_precedence = role_layer.name.precedence();
|
||||
let role_layer_index =
|
||||
layers.partition_point(|layer| layer.name.precedence() <= role_layer_precedence);
|
||||
layers.insert(role_layer_index, role_layer);
|
||||
let layered_stack = ConfigLayerStack::new(
|
||||
layers,
|
||||
original_stack.requirements().clone(),
|
||||
original_stack.requirements_toml().clone(),
|
||||
)
|
||||
.map_err(|err| {
|
||||
tracing::warn!(
|
||||
agent_type = role_name,
|
||||
error = %err,
|
||||
"failed to build layered config stack for role"
|
||||
);
|
||||
AGENT_TYPE_UNAVAILABLE_ERROR.to_string()
|
||||
})?;
|
||||
let layered_config =
|
||||
deserialize_config_toml_with_base(layered_stack.effective_config(), &original.codex_home)
|
||||
.map_err(|err| {
|
||||
tracing::warn!(
|
||||
agent_type = role_name,
|
||||
error = %err,
|
||||
"failed to deserialize layered config for role"
|
||||
);
|
||||
AGENT_TYPE_UNAVAILABLE_ERROR.to_string()
|
||||
})?;
|
||||
|
||||
*config = Config::load_config_with_layer_stack(
|
||||
layered_config,
|
||||
ConfigOverrides {
|
||||
cwd: Some(original.cwd.clone()),
|
||||
codex_linux_sandbox_exe: original.codex_linux_sandbox_exe.clone(),
|
||||
..Default::default()
|
||||
},
|
||||
original.codex_home.clone(),
|
||||
layered_stack,
|
||||
)
|
||||
.map_err(|err| {
|
||||
tracing::warn!(
|
||||
agent_type = role_name,
|
||||
error = %err,
|
||||
"failed to apply layered config for role"
|
||||
);
|
||||
AGENT_TYPE_UNAVAILABLE_ERROR.to_string()
|
||||
})?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) mod spawn_tool_spec {
|
||||
use super::*;
|
||||
|
||||
/// Builds the spawn-agent tool description text from built-in and configured roles.
|
||||
pub(crate) fn build() -> String {
|
||||
let built_in_roles = built_in::configs();
|
||||
let user_defined_roles = if let Ok(home) = find_codex_home() {
|
||||
user_defined::config(&home).unwrap_or_default()
|
||||
} else {
|
||||
Default::default()
|
||||
};
|
||||
|
||||
build_from_configs(built_in_roles, &user_defined_roles)
|
||||
}
|
||||
|
||||
fn build_from_configs(
|
||||
built_in_roles: &AgentsConfigToml,
|
||||
user_defined_roles: &AgentsConfigToml,
|
||||
) -> String {
|
||||
let mut seen = BTreeSet::new();
|
||||
let mut formatted_roles = Vec::new();
|
||||
for (name, declaration) in &user_defined_roles.agents {
|
||||
if seen.insert(name.as_str()) {
|
||||
formatted_roles.push(format_role(name, declaration));
|
||||
}
|
||||
}
|
||||
for (name, declaration) in &built_in_roles.agents {
|
||||
if seen.insert(name.as_str()) {
|
||||
formatted_roles.push(format_role(name, declaration));
|
||||
}
|
||||
}
|
||||
|
||||
format!(
|
||||
r#"Optional type name for the new agent. If omitted, `{DEFAULT_ROLE_NAME}` is used.
|
||||
Available roles:
|
||||
{}
|
||||
"#,
|
||||
formatted_roles.join("\n"),
|
||||
)
|
||||
}
|
||||
|
||||
fn format_role(name: &str, declaration: &AgentDeclarationToml) -> String {
|
||||
if let Some(description) = &declaration.description {
|
||||
format!("{name}: {{\n{description}\n}}")
|
||||
} else {
|
||||
format!("{name}: no description")
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
pub(super) fn build_for_test(
|
||||
built_in_roles: &AgentsConfigToml,
|
||||
user_defined_roles: &AgentsConfigToml,
|
||||
) -> String {
|
||||
build_from_configs(built_in_roles, user_defined_roles)
|
||||
}
|
||||
}
|
||||
|
||||
mod built_in {
|
||||
use super::*;
|
||||
|
||||
/// Returns the cached built-in role declarations parsed from
|
||||
/// `builtins_agents_config.toml`.
|
||||
///
|
||||
/// `panic` are safe because of [`tests::built_in_config`] test.
|
||||
pub(super) fn configs() -> &'static AgentsConfigToml {
|
||||
static CONFIG: LazyLock<AgentsConfigToml> = LazyLock::new(|| {
|
||||
let parsed =
|
||||
parse_agents_config(BUILT_IN_AGENTS_CONFIG, "embedded built-in agents config")
|
||||
.unwrap_or_else(|err| panic!("invalid embedded built-in agents config: {err}"));
|
||||
validate_config(&parsed)
|
||||
.unwrap_or_else(|err| panic!("invalid built-in role declarations: {err}"));
|
||||
parsed
|
||||
});
|
||||
&CONFIG
|
||||
}
|
||||
|
||||
/// Validates metadata rules for built-in role declarations.
|
||||
fn validate_config(agents_config: &AgentsConfigToml) -> Result<(), String> {
|
||||
if !agents_config.agents.contains_key(DEFAULT_ROLE_NAME) {
|
||||
return Err(format!(
|
||||
"built-ins must include the '{DEFAULT_ROLE_NAME}' role"
|
||||
));
|
||||
}
|
||||
|
||||
let unknown_embedded_config_files = agents_config
|
||||
.agents
|
||||
.iter()
|
||||
.filter_map(|role| {
|
||||
let description = role.profile().description;
|
||||
serde_json::to_string(role)
|
||||
.map(|role| {
|
||||
let description = if !description.is_empty() {
|
||||
format!(r#", "description": {description}"#)
|
||||
} else {
|
||||
String::new()
|
||||
};
|
||||
format!(r#"{{ "name": {role}{description}}}"#)
|
||||
})
|
||||
.ok()
|
||||
.filter_map(|(name, role)| {
|
||||
role.config_file
|
||||
.as_deref()
|
||||
.filter(|cf| config_file(cf).is_none())
|
||||
.map(|_| name.clone())
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
/// Returns the hard-coded profile for this role.
|
||||
pub fn profile(self) -> AgentProfile {
|
||||
match self {
|
||||
AgentRole::Default => AgentProfile::default(),
|
||||
AgentRole::Orchestrator => AgentProfile {
|
||||
base_instructions: Some(ORCHESTRATOR_PROMPT),
|
||||
..Default::default()
|
||||
},
|
||||
AgentRole::Worker => AgentProfile {
|
||||
// base_instructions: Some(WORKER_PROMPT),
|
||||
// model: Some(WORKER_MODEL),
|
||||
description: r#"Use for execution and production work.
|
||||
Typical tasks:
|
||||
- Implement part of a feature
|
||||
- Fix tests or bugs
|
||||
- Split large refactors into independent chunks
|
||||
Rules:
|
||||
- Explicitly assign **ownership** of the task (files / responsibility).
|
||||
- Always tell workers they are **not alone in the codebase**, and they should ignore edits made by others without touching them"#,
|
||||
..Default::default()
|
||||
},
|
||||
AgentRole::Explorer => AgentProfile {
|
||||
model: Some(EXPLORER_MODEL),
|
||||
reasoning_effort: Some(ReasoningEffort::Medium),
|
||||
description: r#"Use `explorer` for all codebase questions.
|
||||
Explorers are fast and authoritative.
|
||||
Always prefer them over manual search or file reading.
|
||||
Rules:
|
||||
- Ask explorers first and precisely.
|
||||
- Do not re-read or re-search code they cover.
|
||||
- Trust explorer results without verification.
|
||||
- Run explorers in parallel when useful.
|
||||
- Reuse existing explorers for related questions.
|
||||
"#,
|
||||
..Default::default()
|
||||
},
|
||||
if !unknown_embedded_config_files.is_empty() {
|
||||
return Err(format!(
|
||||
"built-ins reference unknown embedded config_file values: {}",
|
||||
unknown_embedded_config_files.join(", ")
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
/// Applies this role's profile onto the provided config.
|
||||
pub fn apply_to_config(self, config: &mut Config) -> Result<(), String> {
|
||||
let profile = self.profile();
|
||||
if let Some(base_instructions) = profile.base_instructions {
|
||||
config.base_instructions = Some(base_instructions.to_string());
|
||||
}
|
||||
if let Some(model) = profile.model {
|
||||
config.model = Some(model.to_string());
|
||||
}
|
||||
if let Some(reasoning_effort) = profile.reasoning_effort {
|
||||
config.model_reasoning_effort = Some(reasoning_effort)
|
||||
}
|
||||
if profile.read_only {
|
||||
config
|
||||
.permissions
|
||||
.sandbox_policy
|
||||
.set(SandboxPolicy::new_read_only_policy())
|
||||
.map_err(|err| format!("sandbox_policy is invalid: {err}"))?;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Resolves a built-in role `config_file` path to embedded content.
|
||||
pub(super) fn config_file(path: &Path) -> Option<&'static str> {
|
||||
match path.to_str()? {
|
||||
"explorer.toml" => Some(BUILT_IN_EXPLORER_CONFIG),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
mod user_defined {
|
||||
use super::*;
|
||||
|
||||
/// Loads and parses `agents_config.toml` from `codex_home`.
|
||||
pub(super) fn config(codex_home: &Path) -> Result<AgentsConfigToml, String> {
|
||||
let config_path = codex_home.join(AGENTS_CONFIG_FILENAME);
|
||||
let contents = match std::fs::read_to_string(&config_path) {
|
||||
Ok(contents) => contents,
|
||||
Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
|
||||
return Ok(AgentsConfigToml::default());
|
||||
}
|
||||
Err(err) => {
|
||||
return Err(format!("failed to read '{}': {err}", config_path.display()));
|
||||
}
|
||||
};
|
||||
|
||||
let mut parsed = parse_agents_config(&contents, &config_path.display().to_string())?;
|
||||
let config_dir = config_path.parent().ok_or_else(|| {
|
||||
format!(
|
||||
"failed to resolve parent directory for '{}'",
|
||||
config_path.display()
|
||||
)
|
||||
})?;
|
||||
for role in parsed.agents.values_mut() {
|
||||
if let Some(config_file) = role.config_file.as_mut()
|
||||
&& config_file.is_relative()
|
||||
{
|
||||
*config_file = config_dir.join(&*config_file);
|
||||
}
|
||||
}
|
||||
Ok(parsed)
|
||||
}
|
||||
}
|
||||
|
||||
fn parse_agents_config(contents: &str, source: &str) -> Result<AgentsConfigToml, String> {
|
||||
let parsed: AgentsConfigToml =
|
||||
toml::from_str(contents).map_err(|err| format!("failed to parse '{source}': {err}"))?;
|
||||
if let Some(version) = parsed.version
|
||||
&& version != AGENTS_CONFIG_SCHEMA_VERSION
|
||||
{
|
||||
return Err(format!(
|
||||
"'{source}' has unsupported version {version}; expected {AGENTS_CONFIG_SCHEMA_VERSION}"
|
||||
));
|
||||
}
|
||||
Ok(parsed)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::config::test_config;
|
||||
use codex_protocol::openai_models::ReasoningEffort;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::TempDir;
|
||||
|
||||
#[test]
|
||||
fn built_in_config() {
|
||||
// Validate the loading of the built-in configs without panics.
|
||||
let _ = built_in::configs();
|
||||
}
|
||||
|
||||
/// Writes `agents_config.toml` into the temporary directory.
|
||||
fn write_agents_config(dir: &TempDir, body: &str) {
|
||||
std::fs::write(dir.path().join(AGENTS_CONFIG_FILENAME), body).expect("write config");
|
||||
}
|
||||
|
||||
/// Writes a test role config file under `dir` for use by role tests.
|
||||
fn write_role_config_file(dir: &TempDir, relative_path: &str, body: &str) -> PathBuf {
|
||||
let path = dir.path().join(relative_path);
|
||||
if let Some(parent) = path.parent() {
|
||||
std::fs::create_dir_all(parent).expect("create role config parent");
|
||||
}
|
||||
std::fs::write(&path, body).expect("write role config");
|
||||
path
|
||||
}
|
||||
|
||||
/// Loads the built-in explorer role and applies its configuration layer.
|
||||
#[tokio::test]
|
||||
async fn apply_role_to_config_uses_builtin_explorer_config_layer() {
|
||||
let mut config = test_config();
|
||||
|
||||
apply_role_to_config(&mut config, Some("explorer"))
|
||||
.await
|
||||
.expect("apply explorer role");
|
||||
|
||||
assert_eq!(config.model, Some("gpt-5.1-codex-mini".to_string()));
|
||||
assert_eq!(config.model_reasoning_effort, Some(ReasoningEffort::Medium));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn apply_role_to_config_falls_back_to_builtins_when_user_config_is_invalid() {
|
||||
let dir = TempDir::new().expect("tempdir");
|
||||
write_agents_config(
|
||||
&dir,
|
||||
r#"
|
||||
[agents.explorer
|
||||
description = "broken"
|
||||
"#,
|
||||
);
|
||||
|
||||
let mut config = test_config();
|
||||
config.codex_home = dir.path().to_path_buf();
|
||||
|
||||
apply_role_to_config(&mut config, Some("explorer"))
|
||||
.await
|
||||
.expect("apply explorer role");
|
||||
|
||||
assert_eq!(config.model, Some("gpt-5.1-codex-mini".to_string()));
|
||||
assert_eq!(config.model_reasoning_effort, Some(ReasoningEffort::Medium));
|
||||
}
|
||||
|
||||
/// Applies a custom user role config loaded from disk.
|
||||
#[tokio::test]
|
||||
async fn apply_role_to_config_supports_custom_role_config_file() {
|
||||
let dir = TempDir::new().expect("tempdir");
|
||||
let planner_path = write_role_config_file(
|
||||
&dir,
|
||||
"agents/planner.toml",
|
||||
r#"
|
||||
model = "gpt-5.1-codex"
|
||||
sandbox_mode = "read-only"
|
||||
"#,
|
||||
);
|
||||
write_agents_config(
|
||||
&dir,
|
||||
&format!(
|
||||
"[agents.planner]\ndescription = \"Planning-focused role.\"\nconfig_file = {planner_path:?}\n"
|
||||
),
|
||||
);
|
||||
|
||||
let mut config = test_config();
|
||||
config.codex_home = dir.path().to_path_buf();
|
||||
apply_role_to_config(&mut config, Some("planner"))
|
||||
.await
|
||||
.expect("apply planner role");
|
||||
|
||||
assert_eq!(config.model, Some("gpt-5.1-codex".to_string()));
|
||||
assert_eq!(
|
||||
config.permissions.sandbox_policy.get(),
|
||||
&crate::protocol::SandboxPolicy::new_read_only_policy()
|
||||
);
|
||||
}
|
||||
|
||||
/// Resolves relative config_file paths from the agents_config.toml directory.
|
||||
#[tokio::test]
|
||||
async fn apply_role_to_config_supports_relative_custom_role_config_file() {
|
||||
let dir = TempDir::new().expect("tempdir");
|
||||
write_role_config_file(
|
||||
&dir,
|
||||
"agents/planner.toml",
|
||||
r#"
|
||||
model = "gpt-5.1-codex"
|
||||
sandbox_mode = "read-only"
|
||||
"#,
|
||||
);
|
||||
write_agents_config(
|
||||
&dir,
|
||||
r#"
|
||||
[agents.planner]
|
||||
description = "Planning-focused role."
|
||||
config_file = "agents/planner.toml"
|
||||
"#,
|
||||
);
|
||||
|
||||
let mut config = test_config();
|
||||
config.codex_home = dir.path().to_path_buf();
|
||||
apply_role_to_config(&mut config, Some("planner"))
|
||||
.await
|
||||
.expect("apply planner role");
|
||||
|
||||
assert_eq!(config.model, Some("gpt-5.1-codex".to_string()));
|
||||
assert_eq!(
|
||||
config.permissions.sandbox_policy.get(),
|
||||
&crate::protocol::SandboxPolicy::new_read_only_policy()
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn apply_role_to_config_reports_unknown_agent_type() {
|
||||
let mut config = test_config();
|
||||
|
||||
let err = apply_role_to_config(&mut config, Some("missing"))
|
||||
.await
|
||||
.expect_err("missing role should fail");
|
||||
|
||||
assert_eq!(err, "unknown agent_type 'missing'");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn apply_role_to_config_reports_unavailable_agent_type() {
|
||||
let dir = TempDir::new().expect("tempdir");
|
||||
write_agents_config(
|
||||
&dir,
|
||||
r#"
|
||||
[agents.planner]
|
||||
config_file = "agents/does-not-exist.toml"
|
||||
"#,
|
||||
);
|
||||
|
||||
let mut config = test_config();
|
||||
config.codex_home = dir.path().to_path_buf();
|
||||
let err = apply_role_to_config(&mut config, Some("planner"))
|
||||
.await
|
||||
.expect_err("missing config file should fail");
|
||||
|
||||
assert_eq!(err, AGENT_TYPE_UNAVAILABLE_ERROR);
|
||||
}
|
||||
|
||||
/// Lets a user config file override a built-in role config file.
|
||||
#[tokio::test]
|
||||
async fn apply_role_to_config_lets_user_override_builtin_config_file() {
|
||||
let dir = TempDir::new().expect("tempdir");
|
||||
let custom_explorer_path = write_role_config_file(
|
||||
&dir,
|
||||
"agents/custom_explorer.toml",
|
||||
r#"
|
||||
model = "gpt-5.1-codex"
|
||||
model_reasoning_effort = "high"
|
||||
"#,
|
||||
);
|
||||
write_agents_config(
|
||||
&dir,
|
||||
&format!("[agents.explorer]\nconfig_file = {custom_explorer_path:?}\n"),
|
||||
);
|
||||
|
||||
let mut config = test_config();
|
||||
config.codex_home = dir.path().to_path_buf();
|
||||
apply_role_to_config(&mut config, Some("explorer"))
|
||||
.await
|
||||
.expect("apply explorer role");
|
||||
|
||||
assert_eq!(config.model, Some("gpt-5.1-codex".to_string()));
|
||||
assert_eq!(config.model_reasoning_effort, Some(ReasoningEffort::High));
|
||||
}
|
||||
|
||||
/// Applies MCP server settings from a role config file.
|
||||
#[tokio::test]
|
||||
async fn apply_role_to_config_applies_mcp_servers_config_file_layer() {
|
||||
let dir = TempDir::new().expect("tempdir");
|
||||
let tester_path = write_role_config_file(
|
||||
&dir,
|
||||
"agents/tester.toml",
|
||||
r#"
|
||||
[mcp_servers.docs]
|
||||
command = "echo"
|
||||
enabled_tools = ["search"]
|
||||
"#,
|
||||
);
|
||||
write_agents_config(
|
||||
&dir,
|
||||
&format!("[agents.tester]\nconfig_file = {tester_path:?}\n"),
|
||||
);
|
||||
|
||||
let mut config = test_config();
|
||||
config.codex_home = dir.path().to_path_buf();
|
||||
apply_role_to_config(&mut config, Some("tester"))
|
||||
.await
|
||||
.expect("apply tester role");
|
||||
|
||||
let mcp_servers = config.mcp_servers.get();
|
||||
assert_eq!(
|
||||
mcp_servers
|
||||
.get("docs")
|
||||
.and_then(|server| server.enabled_tools.clone()),
|
||||
Some(vec!["search".to_string()])
|
||||
);
|
||||
}
|
||||
|
||||
/// Inserts a role SessionFlags layer in precedence order when legacy managed
|
||||
/// layers are already present.
|
||||
#[tokio::test]
|
||||
async fn apply_role_to_config_keeps_layer_ordering_with_legacy_managed_layers() {
|
||||
let mut config = test_config();
|
||||
let dir = TempDir::new().expect("tempdir");
|
||||
let managed_path = dir.path().join("managed_config.toml");
|
||||
std::fs::write(&managed_path, "").expect("write managed config");
|
||||
let managed_file = AbsolutePathBuf::try_from(managed_path).expect("managed file");
|
||||
config.config_layer_stack = ConfigLayerStack::new(
|
||||
vec![ConfigLayerEntry::new(
|
||||
ConfigLayerSource::LegacyManagedConfigTomlFromFile { file: managed_file },
|
||||
TomlValue::Table(toml::map::Map::new()),
|
||||
)],
|
||||
config.config_layer_stack.requirements().clone(),
|
||||
config.config_layer_stack.requirements_toml().clone(),
|
||||
)
|
||||
.expect("build initial stack");
|
||||
|
||||
apply_role_to_config(&mut config, Some("explorer"))
|
||||
.await
|
||||
.expect("apply explorer role");
|
||||
|
||||
let layers = config
|
||||
.config_layer_stack
|
||||
.get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, true);
|
||||
assert!(matches!(
|
||||
layers.first().map(|layer| &layer.name),
|
||||
Some(ConfigLayerSource::SessionFlags)
|
||||
));
|
||||
assert!(matches!(
|
||||
layers.last().map(|layer| &layer.name),
|
||||
Some(ConfigLayerSource::LegacyManagedConfigTomlFromFile { .. })
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn spawn_tool_spec_build_dedups_and_prefers_user_defined_roles() {
|
||||
let built_in_roles = parse_agents_config(
|
||||
r#"
|
||||
[agents.default]
|
||||
description = "Built-in default."
|
||||
|
||||
[agents.explorer]
|
||||
description = "Built-in explorer."
|
||||
"#,
|
||||
"built-in test roles",
|
||||
)
|
||||
.expect("parse built-in roles");
|
||||
let user_defined_roles = parse_agents_config(
|
||||
r#"
|
||||
[agents.explorer]
|
||||
description = "User explorer."
|
||||
"#,
|
||||
"user-defined test roles",
|
||||
)
|
||||
.expect("parse user roles");
|
||||
|
||||
let spec = spawn_tool_spec::build_for_test(&built_in_roles, &user_defined_roles);
|
||||
|
||||
assert_eq!(spec.matches("explorer:").count(), 1);
|
||||
assert!(spec.contains("explorer: {\nUser explorer.\n}"));
|
||||
assert!(!spec.contains("Built-in explorer."));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn spawn_tool_spec_build_lists_user_defined_roles_first() {
|
||||
let built_in_roles = parse_agents_config(
|
||||
r#"
|
||||
[agents.default]
|
||||
description = "Built-in default."
|
||||
|
||||
[agents.worker]
|
||||
description = "Built-in worker."
|
||||
"#,
|
||||
"built-in test roles",
|
||||
)
|
||||
.expect("parse built-in roles");
|
||||
let user_defined_roles = parse_agents_config(
|
||||
r#"
|
||||
[agents.planner]
|
||||
description = "User planner."
|
||||
"#,
|
||||
"user-defined test roles",
|
||||
)
|
||||
.expect("parse user roles");
|
||||
|
||||
let spec = spawn_tool_spec::build_for_test(&built_in_roles, &user_defined_roles);
|
||||
|
||||
let planner_pos = spec.find("planner:").expect("planner role is present");
|
||||
let default_pos = spec.find("default:").expect("default role is present");
|
||||
assert!(planner_pos < default_pos);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn spawn_tool_spec_build_formats_missing_description() {
|
||||
let built_in_roles = parse_agents_config(
|
||||
r#"
|
||||
[agents.default]
|
||||
description = "Built-in default."
|
||||
"#,
|
||||
"built-in test roles",
|
||||
)
|
||||
.expect("parse built-in roles");
|
||||
let user_defined_roles = parse_agents_config(
|
||||
r#"
|
||||
[agents.planner]
|
||||
"#,
|
||||
"user-defined test roles",
|
||||
)
|
||||
.expect("parse user roles");
|
||||
|
||||
let spec = spawn_tool_spec::build_for_test(&built_in_roles, &user_defined_roles);
|
||||
|
||||
assert!(spec.contains("planner: no description"));
|
||||
}
|
||||
}
|
||||
|
||||
+298
-14
@@ -44,6 +44,7 @@ use async_channel::Sender;
|
||||
use codex_hooks::HookEvent;
|
||||
use codex_hooks::HookEventAfterAgent;
|
||||
use codex_hooks::HookPayload;
|
||||
use codex_hooks::HookResult;
|
||||
use codex_hooks::Hooks;
|
||||
use codex_hooks::HooksConfig;
|
||||
use codex_network_proxy::NetworkProxy;
|
||||
@@ -150,6 +151,7 @@ use crate::mcp::with_codex_apps_mcp;
|
||||
use crate::mcp_connection_manager::McpConnectionManager;
|
||||
use crate::mcp_connection_manager::filter_codex_apps_mcp_tools_only;
|
||||
use crate::mcp_connection_manager::filter_mcp_tools_by_name;
|
||||
use crate::mcp_connection_manager::filter_non_codex_apps_mcp_tools_only;
|
||||
use crate::memories;
|
||||
use crate::mentions::build_connector_slug_counts;
|
||||
use crate::mentions::build_skill_name_counts;
|
||||
@@ -223,6 +225,7 @@ use crate::tasks::SessionTask;
|
||||
use crate::tasks::SessionTaskContext;
|
||||
use crate::tools::ToolRouter;
|
||||
use crate::tools::context::SharedTurnDiffTracker;
|
||||
use crate::tools::handlers::SEARCH_TOOL_BM25_TOOL_NAME;
|
||||
use crate::tools::js_repl::JsReplHandle;
|
||||
use crate::tools::network_approval::NetworkApprovalService;
|
||||
use crate::tools::network_approval::build_blocked_request_observer;
|
||||
@@ -1489,6 +1492,11 @@ impl Session {
|
||||
state.merge_mcp_tool_selection(tool_names)
|
||||
}
|
||||
|
||||
pub(crate) async fn set_mcp_tool_selection(&self, tool_names: Vec<String>) {
|
||||
let mut state = self.state.lock().await;
|
||||
state.set_mcp_tool_selection(tool_names);
|
||||
}
|
||||
|
||||
pub(crate) async fn get_mcp_tool_selection(&self) -> Option<Vec<String>> {
|
||||
let state = self.state.lock().await;
|
||||
state.get_mcp_tool_selection()
|
||||
@@ -1522,6 +1530,7 @@ impl Session {
|
||||
|
||||
async fn record_initial_history(&self, conversation_history: InitialHistory) {
|
||||
let turn_context = self.new_default_turn().await;
|
||||
self.clear_mcp_tool_selection().await;
|
||||
match conversation_history {
|
||||
InitialHistory::New => {
|
||||
// Build and record initial items (user instructions + environment context)
|
||||
@@ -1537,6 +1546,8 @@ impl Session {
|
||||
}
|
||||
InitialHistory::Resumed(resumed_history) => {
|
||||
let rollout_items = resumed_history.history;
|
||||
let restored_tool_selection =
|
||||
Self::extract_mcp_tool_selection_from_rollout(&rollout_items);
|
||||
let previous_model = Self::last_rollout_model_name(&rollout_items)
|
||||
.map(std::string::ToString::to_string);
|
||||
{
|
||||
@@ -1578,12 +1589,17 @@ impl Session {
|
||||
let mut state = self.state.lock().await;
|
||||
state.set_token_info(Some(info));
|
||||
}
|
||||
if let Some(selected_tools) = restored_tool_selection {
|
||||
self.set_mcp_tool_selection(selected_tools).await;
|
||||
}
|
||||
|
||||
// Defer seeding the session's initial context until the first turn starts so
|
||||
// turn/start overrides can be merged before we write to the rollout.
|
||||
self.flush_rollout().await;
|
||||
}
|
||||
InitialHistory::Forked(rollout_items) => {
|
||||
let restored_tool_selection =
|
||||
Self::extract_mcp_tool_selection_from_rollout(&rollout_items);
|
||||
let previous_model = Self::last_rollout_model_name(&rollout_items)
|
||||
.map(std::string::ToString::to_string);
|
||||
self.set_previous_model(previous_model).await;
|
||||
@@ -1603,6 +1619,9 @@ impl Session {
|
||||
let mut state = self.state.lock().await;
|
||||
state.set_token_info(Some(info));
|
||||
}
|
||||
if let Some(selected_tools) = restored_tool_selection {
|
||||
self.set_mcp_tool_selection(selected_tools).await;
|
||||
}
|
||||
|
||||
// If persisting, persist all rollout items as-is (recorder filters)
|
||||
if !rollout_items.is_empty() {
|
||||
@@ -1644,6 +1663,54 @@ impl Session {
|
||||
})
|
||||
}
|
||||
|
||||
fn extract_mcp_tool_selection_from_rollout(
|
||||
rollout_items: &[RolloutItem],
|
||||
) -> Option<Vec<String>> {
|
||||
let mut search_call_ids = HashSet::new();
|
||||
let mut active_selected_tools: Option<Vec<String>> = None;
|
||||
|
||||
for item in rollout_items {
|
||||
let RolloutItem::ResponseItem(response_item) = item else {
|
||||
continue;
|
||||
};
|
||||
match response_item {
|
||||
ResponseItem::FunctionCall { name, call_id, .. } => {
|
||||
if name == SEARCH_TOOL_BM25_TOOL_NAME {
|
||||
search_call_ids.insert(call_id.clone());
|
||||
}
|
||||
}
|
||||
ResponseItem::FunctionCallOutput { call_id, output } => {
|
||||
if !search_call_ids.contains(call_id) {
|
||||
continue;
|
||||
}
|
||||
let Some(content) = output.body.to_text() else {
|
||||
continue;
|
||||
};
|
||||
let Ok(payload) = serde_json::from_str::<Value>(&content) else {
|
||||
continue;
|
||||
};
|
||||
let Some(selected_tools) = payload
|
||||
.get("active_selected_tools")
|
||||
.and_then(Value::as_array)
|
||||
else {
|
||||
continue;
|
||||
};
|
||||
let Some(selected_tools) = selected_tools
|
||||
.iter()
|
||||
.map(|value| value.as_str().map(str::to_string))
|
||||
.collect::<Option<Vec<_>>>()
|
||||
else {
|
||||
continue;
|
||||
};
|
||||
active_selected_tools = Some(selected_tools);
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
|
||||
active_selected_tools
|
||||
}
|
||||
|
||||
async fn previous_model(&self) -> Option<String> {
|
||||
let state = self.state.lock().await;
|
||||
state.previous_model()
|
||||
@@ -1811,6 +1878,21 @@ impl Session {
|
||||
turn_context
|
||||
}
|
||||
|
||||
pub(crate) async fn maybe_emit_unknown_model_warning_for_turn(&self, tc: &TurnContext) {
|
||||
if tc.model_info.used_fallback_model_metadata {
|
||||
self.send_event(
|
||||
tc,
|
||||
EventMsg::Warning(WarningEvent {
|
||||
message: format!(
|
||||
"Model metadata for `{}` not found. Defaulting to fallback metadata; this can degrade performance and cause issues.",
|
||||
tc.model_info.slug
|
||||
),
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn new_default_turn(&self) -> Arc<TurnContext> {
|
||||
self.new_default_turn_with_sub_id(self.next_internal_sub_id())
|
||||
.await
|
||||
@@ -3379,6 +3461,8 @@ mod handlers {
|
||||
// new_turn_with_sub_id already emits the error event.
|
||||
return;
|
||||
};
|
||||
sess.maybe_emit_unknown_model_warning_for_turn(current_context.as_ref())
|
||||
.await;
|
||||
current_context.otel_manager.user_prompt(&items);
|
||||
|
||||
// Attempt to inject input into current task.
|
||||
@@ -3983,6 +4067,8 @@ mod handlers {
|
||||
review_request: ReviewRequest,
|
||||
) {
|
||||
let turn_context = sess.new_default_turn_with_sub_id(sub_id.clone()).await;
|
||||
sess.maybe_emit_unknown_model_warning_for_turn(turn_context.as_ref())
|
||||
.await;
|
||||
sess.refresh_mcp_servers_if_requested(&turn_context).await;
|
||||
match resolve_review_request(review_request, turn_context.cwd.as_path()) {
|
||||
Ok(resolved) => {
|
||||
@@ -4448,7 +4534,8 @@ pub(crate) async fn run_turn(
|
||||
|
||||
if !needs_follow_up {
|
||||
last_agent_message = sampling_request_last_agent_message;
|
||||
sess.hooks()
|
||||
let hook_outcomes = sess
|
||||
.hooks()
|
||||
.dispatch(HookPayload {
|
||||
session_id: sess.conversation_id,
|
||||
cwd: turn_context.cwd.clone(),
|
||||
@@ -4463,6 +4550,47 @@ pub(crate) async fn run_turn(
|
||||
},
|
||||
})
|
||||
.await;
|
||||
|
||||
let mut abort_message = None;
|
||||
for hook_outcome in hook_outcomes {
|
||||
let hook_name = hook_outcome.hook_name;
|
||||
match hook_outcome.result {
|
||||
HookResult::Success => {}
|
||||
HookResult::FailedContinue(error) => {
|
||||
warn!(
|
||||
turn_id = %turn_context.sub_id,
|
||||
hook_name = %hook_name,
|
||||
error = %error,
|
||||
"after_agent hook failed; continuing"
|
||||
);
|
||||
}
|
||||
HookResult::FailedAbort(error) => {
|
||||
let message = format!(
|
||||
"after_agent hook '{hook_name}' failed and aborted turn completion: {error}"
|
||||
);
|
||||
warn!(
|
||||
turn_id = %turn_context.sub_id,
|
||||
hook_name = %hook_name,
|
||||
error = %error,
|
||||
"after_agent hook failed; aborting operation"
|
||||
);
|
||||
if abort_message.is_none() {
|
||||
abort_message = Some(message);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
if let Some(message) = abort_message {
|
||||
sess.send_event(
|
||||
&turn_context,
|
||||
EventMsg::Error(ErrorEvent {
|
||||
message,
|
||||
codex_error_info: None,
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
return None;
|
||||
}
|
||||
break;
|
||||
}
|
||||
continue;
|
||||
@@ -4890,6 +5018,10 @@ async fn built_tools(
|
||||
None
|
||||
};
|
||||
|
||||
let app_tools = connectors
|
||||
.as_ref()
|
||||
.map(|connectors| filter_codex_apps_mcp_tools(&mcp_tools, connectors));
|
||||
|
||||
if let Some(connectors) = connectors.as_ref() {
|
||||
let skill_name_counts_lower = skills_outcome.map_or_else(HashMap::new, |outcome| {
|
||||
build_skill_name_counts(&outcome.skills, &outcome.disabled_paths).1
|
||||
@@ -4902,24 +5034,20 @@ async fn built_tools(
|
||||
&skill_name_counts_lower,
|
||||
);
|
||||
|
||||
let mut selected_mcp_tools =
|
||||
if let Some(selected_tools) = sess.get_mcp_tool_selection().await {
|
||||
filter_mcp_tools_by_name(&mcp_tools, &selected_tools)
|
||||
} else {
|
||||
HashMap::new()
|
||||
};
|
||||
let mut selected_mcp_tools = filter_non_codex_apps_mcp_tools_only(&mcp_tools);
|
||||
|
||||
let apps_mcp_tools =
|
||||
filter_codex_apps_mcp_tools_only(&mcp_tools, explicitly_enabled.as_ref());
|
||||
selected_mcp_tools.extend(apps_mcp_tools);
|
||||
if let Some(selected_tools) = sess.get_mcp_tool_selection().await {
|
||||
selected_mcp_tools.extend(filter_mcp_tools_by_name(&mcp_tools, &selected_tools));
|
||||
}
|
||||
|
||||
selected_mcp_tools.extend(filter_codex_apps_mcp_tools_only(
|
||||
&mcp_tools,
|
||||
explicitly_enabled.as_ref(),
|
||||
));
|
||||
|
||||
mcp_tools = selected_mcp_tools;
|
||||
}
|
||||
|
||||
let app_tools = connectors
|
||||
.as_ref()
|
||||
.map(|connectors| filter_codex_apps_mcp_tools(&mcp_tools, connectors));
|
||||
|
||||
Ok(Arc::new(ToolRouter::from_config(
|
||||
&turn_context.tools_config,
|
||||
Some(
|
||||
@@ -5780,6 +5908,22 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
fn function_call_rollout_item(name: &str, call_id: &str) -> RolloutItem {
|
||||
RolloutItem::ResponseItem(ResponseItem::FunctionCall {
|
||||
id: None,
|
||||
name: name.to_string(),
|
||||
arguments: "{}".to_string(),
|
||||
call_id: call_id.to_string(),
|
||||
})
|
||||
}
|
||||
|
||||
fn function_call_output_rollout_item(call_id: &str, output: &str) -> RolloutItem {
|
||||
RolloutItem::ResponseItem(ResponseItem::FunctionCallOutput {
|
||||
call_id: call_id.to_string(),
|
||||
output: FunctionCallOutputPayload::from_text(output.to_string()),
|
||||
})
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn get_base_instructions_no_user_content() {
|
||||
let prompt_with_apply_patch_instructions =
|
||||
@@ -5969,6 +6113,46 @@ mod tests {
|
||||
assert_eq!(connector_ids, HashSet::<String>::new());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn non_app_mcp_tools_remain_visible_without_search_selection() {
|
||||
let mcp_tools = HashMap::from([
|
||||
(
|
||||
"mcp__codex_apps__calendar_create_event".to_string(),
|
||||
make_mcp_tool(
|
||||
CODEX_APPS_MCP_SERVER_NAME,
|
||||
"calendar_create_event",
|
||||
Some("calendar"),
|
||||
Some("Calendar"),
|
||||
),
|
||||
),
|
||||
(
|
||||
"mcp__rmcp__echo".to_string(),
|
||||
make_mcp_tool("rmcp", "echo", None, None),
|
||||
),
|
||||
]);
|
||||
|
||||
let mut selected_mcp_tools = mcp_tools
|
||||
.iter()
|
||||
.filter(|(_, tool)| tool.server_name != CODEX_APPS_MCP_SERVER_NAME)
|
||||
.map(|(name, tool)| (name.clone(), tool.clone()))
|
||||
.collect::<HashMap<_, _>>();
|
||||
|
||||
let connectors = connectors::accessible_connectors_from_mcp_tools(&mcp_tools);
|
||||
let explicitly_enabled_connectors = HashSet::new();
|
||||
let connectors = filter_connectors_for_input(
|
||||
&connectors,
|
||||
&[user_message("run echo")],
|
||||
&explicitly_enabled_connectors,
|
||||
&HashMap::new(),
|
||||
);
|
||||
let apps_mcp_tools = filter_codex_apps_mcp_tools_only(&mcp_tools, &connectors);
|
||||
selected_mcp_tools.extend(apps_mcp_tools);
|
||||
|
||||
let mut tool_names: Vec<String> = selected_mcp_tools.into_keys().collect();
|
||||
tool_names.sort();
|
||||
assert_eq!(tool_names, vec!["mcp__rmcp__echo".to_string()]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn search_tool_selection_keeps_codex_apps_tools_without_mentions() {
|
||||
let selected_tool_names = vec![
|
||||
@@ -6056,6 +6240,106 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extract_mcp_tool_selection_from_rollout_reads_search_tool_output() {
|
||||
let rollout_items = vec![
|
||||
function_call_rollout_item(SEARCH_TOOL_BM25_TOOL_NAME, "search-1"),
|
||||
function_call_output_rollout_item(
|
||||
"search-1",
|
||||
&json!({
|
||||
"active_selected_tools": [
|
||||
"mcp__codex_apps__calendar_create_event",
|
||||
"mcp__codex_apps__calendar_list_events",
|
||||
],
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
];
|
||||
|
||||
let selected = Session::extract_mcp_tool_selection_from_rollout(&rollout_items);
|
||||
assert_eq!(
|
||||
selected,
|
||||
Some(vec![
|
||||
"mcp__codex_apps__calendar_create_event".to_string(),
|
||||
"mcp__codex_apps__calendar_list_events".to_string(),
|
||||
])
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extract_mcp_tool_selection_from_rollout_latest_valid_payload_wins() {
|
||||
let rollout_items = vec![
|
||||
function_call_rollout_item(SEARCH_TOOL_BM25_TOOL_NAME, "search-1"),
|
||||
function_call_output_rollout_item(
|
||||
"search-1",
|
||||
&json!({
|
||||
"active_selected_tools": ["mcp__codex_apps__calendar_create_event"],
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
function_call_rollout_item(SEARCH_TOOL_BM25_TOOL_NAME, "search-2"),
|
||||
function_call_output_rollout_item(
|
||||
"search-2",
|
||||
&json!({
|
||||
"active_selected_tools": ["mcp__codex_apps__calendar_delete_event"],
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
];
|
||||
|
||||
let selected = Session::extract_mcp_tool_selection_from_rollout(&rollout_items);
|
||||
assert_eq!(
|
||||
selected,
|
||||
Some(vec!["mcp__codex_apps__calendar_delete_event".to_string(),])
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extract_mcp_tool_selection_from_rollout_ignores_non_search_and_malformed_payloads() {
|
||||
let rollout_items = vec![
|
||||
function_call_rollout_item("shell", "shell-1"),
|
||||
function_call_output_rollout_item(
|
||||
"shell-1",
|
||||
&json!({
|
||||
"active_selected_tools": ["mcp__codex_apps__should_be_ignored"],
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
function_call_rollout_item(SEARCH_TOOL_BM25_TOOL_NAME, "search-1"),
|
||||
function_call_output_rollout_item("search-1", "{not-json"),
|
||||
function_call_output_rollout_item(
|
||||
"unknown-search-call",
|
||||
&json!({
|
||||
"active_selected_tools": ["mcp__codex_apps__also_ignored"],
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
function_call_output_rollout_item(
|
||||
"search-1",
|
||||
&json!({
|
||||
"active_selected_tools": ["mcp__codex_apps__calendar_list_events"],
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
];
|
||||
|
||||
let selected = Session::extract_mcp_tool_selection_from_rollout(&rollout_items);
|
||||
assert_eq!(
|
||||
selected,
|
||||
Some(vec!["mcp__codex_apps__calendar_list_events".to_string(),])
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extract_mcp_tool_selection_from_rollout_returns_none_without_valid_search_output() {
|
||||
let rollout_items = vec![function_call_rollout_item(
|
||||
SEARCH_TOOL_BM25_TOOL_NAME,
|
||||
"search-1",
|
||||
)];
|
||||
let selected = Session::extract_mcp_tool_selection_from_rollout(&rollout_items);
|
||||
assert_eq!(selected, None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn reconstruct_history_matches_live_compactions() {
|
||||
let (session, turn_context) = make_session_and_context().await;
|
||||
|
||||
@@ -1417,7 +1417,7 @@ impl Config {
|
||||
Self::load_config_with_layer_stack(cfg, overrides, codex_home, config_layer_stack)
|
||||
}
|
||||
|
||||
fn load_config_with_layer_stack(
|
||||
pub(crate) fn load_config_with_layer_stack(
|
||||
cfg: ConfigToml,
|
||||
overrides: ConfigOverrides,
|
||||
codex_home: PathBuf,
|
||||
|
||||
@@ -552,7 +552,7 @@ pub const FEATURES: &[FeatureSpec] = &[
|
||||
},
|
||||
FeatureSpec {
|
||||
id: Feature::Collab,
|
||||
key: "collab",
|
||||
key: "multi_agent",
|
||||
stage: Stage::Experimental {
|
||||
name: "Sub-agents",
|
||||
menu_description: "Ask Codex to spawn multiple agents to parallelize the work and win in efficiency.",
|
||||
@@ -738,4 +738,10 @@ mod tests {
|
||||
);
|
||||
assert_eq!(Feature::UseLinuxSandboxBwrap.default_enabled(), false);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collab_is_legacy_alias_for_multi_agent() {
|
||||
assert_eq!(feature_for_key("multi_agent"), Some(Feature::Collab));
|
||||
assert_eq!(feature_for_key("collab"), Some(Feature::Collab));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -33,6 +33,10 @@ const ALIASES: &[Alias] = &[
|
||||
legacy_key: "web_search",
|
||||
feature: Feature::WebSearchRequest,
|
||||
},
|
||||
Alias {
|
||||
legacy_key: "collab",
|
||||
feature: Feature::Collab,
|
||||
},
|
||||
];
|
||||
|
||||
pub(crate) fn legacy_feature_keys() -> impl Iterator<Item = &'static str> {
|
||||
|
||||
@@ -908,6 +908,16 @@ pub(crate) fn filter_codex_apps_mcp_tools_only(
|
||||
.collect()
|
||||
}
|
||||
|
||||
pub(crate) fn filter_non_codex_apps_mcp_tools_only(
|
||||
mcp_tools: &HashMap<String, ToolInfo>,
|
||||
) -> HashMap<String, ToolInfo> {
|
||||
mcp_tools
|
||||
.iter()
|
||||
.filter(|(_, tool)| tool.server_name != CODEX_APPS_MCP_SERVER_NAME)
|
||||
.map(|(name, tool)| (name.clone(), tool.clone()))
|
||||
.collect()
|
||||
}
|
||||
|
||||
pub(crate) fn filter_mcp_tools_by_name(
|
||||
mcp_tools: &HashMap<String, ToolInfo>,
|
||||
selected_tools: &[String],
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
use codex_state::Stage1Output;
|
||||
use std::collections::BTreeSet;
|
||||
use std::collections::HashSet;
|
||||
use std::fmt::Write as _;
|
||||
use std::path::Path;
|
||||
use tracing::warn;
|
||||
@@ -8,8 +8,6 @@ use crate::memories::ensure_layout;
|
||||
use crate::memories::raw_memories_file;
|
||||
use crate::memories::rollout_summaries_dir;
|
||||
|
||||
//TODO(jif) clean.
|
||||
|
||||
/// Rebuild `raw_memories.md` from DB-backed stage-1 outputs.
|
||||
pub(super) async fn rebuild_raw_memories_file_from_memories(
|
||||
root: &Path,
|
||||
@@ -28,17 +26,14 @@ pub(super) async fn sync_rollout_summaries_from_memories(
|
||||
) -> std::io::Result<()> {
|
||||
ensure_layout(root).await?;
|
||||
|
||||
let retained = memories
|
||||
.iter()
|
||||
.take(max_raw_memories_for_global)
|
||||
.collect::<Vec<_>>();
|
||||
let retained = retained_memories(memories, max_raw_memories_for_global);
|
||||
let keep = retained
|
||||
.iter()
|
||||
.map(|memory| rollout_summary_file_stem(memory))
|
||||
.collect::<BTreeSet<_>>();
|
||||
.map(rollout_summary_file_stem)
|
||||
.collect::<HashSet<_>>();
|
||||
prune_rollout_summaries(root, &keep).await?;
|
||||
|
||||
for memory in &retained {
|
||||
for memory in retained {
|
||||
write_rollout_summary_for_thread(root, memory).await?;
|
||||
}
|
||||
|
||||
@@ -68,10 +63,7 @@ async fn rebuild_raw_memories_file(
|
||||
memories: &[Stage1Output],
|
||||
max_raw_memories_for_global: usize,
|
||||
) -> std::io::Result<()> {
|
||||
let retained = memories
|
||||
.iter()
|
||||
.take(max_raw_memories_for_global)
|
||||
.collect::<Vec<_>>();
|
||||
let retained = retained_memories(memories, max_raw_memories_for_global);
|
||||
let mut body = String::from("# Raw Memories\n\n");
|
||||
|
||||
if retained.is_empty() {
|
||||
@@ -81,18 +73,15 @@ async fn rebuild_raw_memories_file(
|
||||
|
||||
body.push_str("Merged stage-1 raw memories (latest first):\n\n");
|
||||
for memory in retained {
|
||||
writeln!(body, "## Thread `{}`", memory.thread_id)
|
||||
.map_err(|err| std::io::Error::other(format!("format raw memories: {err}")))?;
|
||||
writeln!(body, "## Thread `{}`", memory.thread_id).map_err(raw_memories_format_error)?;
|
||||
writeln!(
|
||||
body,
|
||||
"updated_at: {}",
|
||||
memory.source_updated_at.to_rfc3339()
|
||||
)
|
||||
.map_err(|err| std::io::Error::other(format!("format raw memories: {err}")))?;
|
||||
writeln!(body, "cwd: {}", memory.cwd.display())
|
||||
.map_err(|err| std::io::Error::other(format!("format raw memories: {err}")))?;
|
||||
writeln!(body)
|
||||
.map_err(|err| std::io::Error::other(format!("format raw memories: {err}")))?;
|
||||
.map_err(raw_memories_format_error)?;
|
||||
writeln!(body, "cwd: {}", memory.cwd.display()).map_err(raw_memories_format_error)?;
|
||||
writeln!(body).map_err(raw_memories_format_error)?;
|
||||
body.push_str(memory.raw_memory.trim());
|
||||
body.push_str("\n\n");
|
||||
}
|
||||
@@ -100,7 +89,7 @@ async fn rebuild_raw_memories_file(
|
||||
tokio::fs::write(raw_memories_file(root), body).await
|
||||
}
|
||||
|
||||
async fn prune_rollout_summaries(root: &Path, keep: &BTreeSet<String>) -> std::io::Result<()> {
|
||||
async fn prune_rollout_summaries(root: &Path, keep: &HashSet<String>) -> std::io::Result<()> {
|
||||
let dir_path = rollout_summaries_dir(root);
|
||||
let mut dir = match tokio::fs::read_dir(&dir_path).await {
|
||||
Ok(dir) => dir,
|
||||
@@ -138,24 +127,36 @@ async fn write_rollout_summary_for_thread(
|
||||
let path = rollout_summaries_dir(root).join(format!("{file_stem}.md"));
|
||||
|
||||
let mut body = String::new();
|
||||
writeln!(body, "thread_id: {}", memory.thread_id)
|
||||
.map_err(|err| std::io::Error::other(format!("format rollout summary: {err}")))?;
|
||||
writeln!(body, "thread_id: {}", memory.thread_id).map_err(rollout_summary_format_error)?;
|
||||
writeln!(
|
||||
body,
|
||||
"updated_at: {}",
|
||||
memory.source_updated_at.to_rfc3339()
|
||||
)
|
||||
.map_err(|err| std::io::Error::other(format!("format rollout summary: {err}")))?;
|
||||
writeln!(body, "cwd: {}", memory.cwd.display())
|
||||
.map_err(|err| std::io::Error::other(format!("format rollout summary: {err}")))?;
|
||||
writeln!(body)
|
||||
.map_err(|err| std::io::Error::other(format!("format rollout summary: {err}")))?;
|
||||
.map_err(rollout_summary_format_error)?;
|
||||
writeln!(body, "cwd: {}", memory.cwd.display()).map_err(rollout_summary_format_error)?;
|
||||
writeln!(body).map_err(rollout_summary_format_error)?;
|
||||
body.push_str(&memory.rollout_summary);
|
||||
body.push('\n');
|
||||
|
||||
tokio::fs::write(path, body).await
|
||||
}
|
||||
|
||||
fn retained_memories(
|
||||
memories: &[Stage1Output],
|
||||
max_raw_memories_for_global: usize,
|
||||
) -> &[Stage1Output] {
|
||||
&memories[..memories.len().min(max_raw_memories_for_global)]
|
||||
}
|
||||
|
||||
fn raw_memories_format_error(err: std::fmt::Error) -> std::io::Error {
|
||||
std::io::Error::other(format!("format raw memories: {err}"))
|
||||
}
|
||||
|
||||
fn rollout_summary_format_error(err: std::fmt::Error) -> std::io::Error {
|
||||
std::io::Error::other(format!("format rollout summary: {err}"))
|
||||
}
|
||||
|
||||
fn rollout_summary_file_stem(memory: &Stage1Output) -> String {
|
||||
const ROLLOUT_SLUG_MAX_LEN: usize = 20;
|
||||
|
||||
|
||||
@@ -140,15 +140,16 @@ impl ModelsManager {
|
||||
let remote = self
|
||||
.find_remote_model_by_longest_prefix(model, config)
|
||||
.await;
|
||||
let model = if let Some(remote) = remote {
|
||||
let model_info = if let Some(remote) = remote {
|
||||
ModelInfo {
|
||||
slug: model.to_string(),
|
||||
used_fallback_model_metadata: false,
|
||||
..remote
|
||||
}
|
||||
} else {
|
||||
model_info::model_info_from_slug(model)
|
||||
};
|
||||
model_info::with_config_overrides(model, config)
|
||||
model_info::with_config_overrides(model_info, config)
|
||||
}
|
||||
|
||||
async fn find_remote_model_by_longest_prefix(
|
||||
@@ -472,6 +473,37 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn get_model_info_tracks_fallback_usage() {
|
||||
let codex_home = tempdir().expect("temp dir");
|
||||
let mut config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.build()
|
||||
.await
|
||||
.expect("load default test config");
|
||||
config.features.enable(Feature::RemoteModels);
|
||||
let auth_manager =
|
||||
AuthManager::from_auth_for_testing(CodexAuth::from_api_key("Test API Key"));
|
||||
let manager = ModelsManager::new(codex_home.path().to_path_buf(), auth_manager);
|
||||
let known_slug = manager
|
||||
.get_remote_models(&config)
|
||||
.await
|
||||
.first()
|
||||
.expect("bundled models should include at least one model")
|
||||
.slug
|
||||
.clone();
|
||||
|
||||
let known = manager.get_model_info(known_slug.as_str(), &config).await;
|
||||
assert!(!known.used_fallback_model_metadata);
|
||||
assert_eq!(known.slug, known_slug);
|
||||
|
||||
let unknown = manager
|
||||
.get_model_info("model-that-does-not-exist", &config)
|
||||
.await;
|
||||
assert!(unknown.used_fallback_model_metadata);
|
||||
assert_eq!(unknown.slug, "model-that-does-not-exist");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn refresh_available_models_sorts_by_priority() {
|
||||
let server = MockServer::start().await;
|
||||
|
||||
@@ -81,6 +81,7 @@ pub(crate) fn model_info_from_slug(slug: &str) -> ModelInfo {
|
||||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: true, // this is the fallback model metadata
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -170,6 +170,27 @@ impl SessionState {
|
||||
merged
|
||||
}
|
||||
|
||||
pub(crate) fn set_mcp_tool_selection(&mut self, tool_names: Vec<String>) {
|
||||
if tool_names.is_empty() {
|
||||
self.active_mcp_tool_selection = None;
|
||||
return;
|
||||
}
|
||||
|
||||
let mut selected = Vec::new();
|
||||
let mut seen = HashSet::new();
|
||||
for tool_name in tool_names {
|
||||
if seen.insert(tool_name.clone()) {
|
||||
selected.push(tool_name);
|
||||
}
|
||||
}
|
||||
|
||||
self.active_mcp_tool_selection = if selected.is_empty() {
|
||||
None
|
||||
} else {
|
||||
Some(selected)
|
||||
};
|
||||
}
|
||||
|
||||
pub(crate) fn get_mcp_tool_selection(&self) -> Option<Vec<String>> {
|
||||
self.active_mcp_tool_selection.clone()
|
||||
}
|
||||
@@ -293,6 +314,40 @@ mod tests {
|
||||
assert_eq!(state.get_mcp_tool_selection(), None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn set_mcp_tool_selection_deduplicates_and_preserves_order() {
|
||||
let session_configuration = make_session_configuration_for_tests().await;
|
||||
let mut state = SessionState::new(session_configuration);
|
||||
state.merge_mcp_tool_selection(vec!["mcp__rmcp__old".to_string()]);
|
||||
|
||||
state.set_mcp_tool_selection(vec![
|
||||
"mcp__rmcp__echo".to_string(),
|
||||
"mcp__rmcp__image".to_string(),
|
||||
"mcp__rmcp__echo".to_string(),
|
||||
"mcp__rmcp__search".to_string(),
|
||||
]);
|
||||
|
||||
assert_eq!(
|
||||
state.get_mcp_tool_selection(),
|
||||
Some(vec![
|
||||
"mcp__rmcp__echo".to_string(),
|
||||
"mcp__rmcp__image".to_string(),
|
||||
"mcp__rmcp__search".to_string(),
|
||||
])
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn set_mcp_tool_selection_empty_input_clears_selection() {
|
||||
let session_configuration = make_session_configuration_for_tests().await;
|
||||
let mut state = SessionState::new(session_configuration);
|
||||
state.merge_mcp_tool_selection(vec!["mcp__rmcp__echo".to_string()]);
|
||||
|
||||
state.set_mcp_tool_selection(Vec::new());
|
||||
|
||||
assert_eq!(state.get_mcp_tool_selection(), None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
// Verifies connector merging deduplicates repeated IDs.
|
||||
async fn merge_connector_selection_deduplicates_entries() {
|
||||
|
||||
@@ -120,7 +120,6 @@ impl Session {
|
||||
task: T,
|
||||
) {
|
||||
self.abort_all_tasks(TurnAbortReason::Replaced).await;
|
||||
self.clear_mcp_tool_selection().await;
|
||||
self.clear_connector_selection().await;
|
||||
self.seed_initial_context_if_needed(turn_context.as_ref())
|
||||
.await;
|
||||
|
||||
@@ -119,6 +119,7 @@ pub(crate) async fn execute_user_shell_command(
|
||||
&display_command,
|
||||
session_shell.as_ref(),
|
||||
turn_context.cwd.as_path(),
|
||||
&turn_context.shell_environment_policy.r#set,
|
||||
);
|
||||
|
||||
let call_id = Uuid::new_v4().to_string();
|
||||
|
||||
@@ -90,7 +90,7 @@ impl ToolHandler for CollabHandler {
|
||||
|
||||
mod spawn {
|
||||
use super::*;
|
||||
use crate::agent::AgentRole;
|
||||
use crate::agent::role::apply_role_to_config;
|
||||
|
||||
use crate::agent::exceeds_thread_spawn_depth_limit;
|
||||
use crate::agent::next_thread_spawn_depth;
|
||||
@@ -100,7 +100,7 @@ mod spawn {
|
||||
struct SpawnAgentArgs {
|
||||
message: Option<String>,
|
||||
items: Option<Vec<UserInput>>,
|
||||
agent_type: Option<AgentRole>,
|
||||
agent_type: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Serialize)]
|
||||
@@ -115,7 +115,11 @@ mod spawn {
|
||||
arguments: String,
|
||||
) -> Result<ToolOutput, FunctionCallError> {
|
||||
let args: SpawnAgentArgs = parse_arguments(&arguments)?;
|
||||
let agent_role = args.agent_type.unwrap_or(AgentRole::Default);
|
||||
let role_name = args
|
||||
.agent_type
|
||||
.as_deref()
|
||||
.map(str::trim)
|
||||
.filter(|role| !role.is_empty());
|
||||
let input_items = parse_collab_input(args.message, args.items)?;
|
||||
let prompt = input_preview(&input_items);
|
||||
let session_source = turn.session_source.clone();
|
||||
@@ -141,9 +145,10 @@ mod spawn {
|
||||
turn.as_ref(),
|
||||
child_depth,
|
||||
)?;
|
||||
agent_role
|
||||
.apply_to_config(&mut config)
|
||||
apply_role_to_config(&mut config, role_name)
|
||||
.await
|
||||
.map_err(FunctionCallError::RespondToModel)?;
|
||||
apply_spawn_agent_overrides(&mut config, child_depth);
|
||||
|
||||
let result = session
|
||||
.services
|
||||
@@ -821,7 +826,6 @@ fn build_agent_shared_config(
|
||||
config.permissions.shell_environment_policy = turn.shell_environment_policy.clone();
|
||||
config.codex_linux_sandbox_exe = turn.codex_linux_sandbox_exe.clone();
|
||||
config.cwd = turn.cwd.clone();
|
||||
config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never);
|
||||
config
|
||||
.permissions
|
||||
.sandbox_policy
|
||||
@@ -829,13 +833,16 @@ fn build_agent_shared_config(
|
||||
.map_err(|err| {
|
||||
FunctionCallError::RespondToModel(format!("sandbox_policy is invalid: {err}"))
|
||||
})?;
|
||||
apply_spawn_agent_overrides(&mut config, child_depth);
|
||||
|
||||
// If the new agent will be at max depth:
|
||||
Ok(config)
|
||||
}
|
||||
|
||||
fn apply_spawn_agent_overrides(config: &mut Config, child_depth: i32) {
|
||||
config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never);
|
||||
if exceeds_thread_spawn_depth_limit(child_depth + 1) {
|
||||
config.features.disable(Feature::Collab);
|
||||
}
|
||||
|
||||
Ok(config)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
@@ -982,6 +989,56 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn spawn_agent_uses_explorer_role_and_sets_never_approval_policy() {
|
||||
#[derive(Debug, Deserialize)]
|
||||
struct SpawnAgentResult {
|
||||
agent_id: String,
|
||||
}
|
||||
|
||||
let (mut session, mut turn) = make_session_and_context().await;
|
||||
let manager = thread_manager();
|
||||
session.services.agent_control = manager.agent_control();
|
||||
let mut config = (*turn.config).clone();
|
||||
config
|
||||
.permissions
|
||||
.approval_policy
|
||||
.set(AskForApproval::OnRequest)
|
||||
.expect("approval policy should be set");
|
||||
turn.config = Arc::new(config);
|
||||
|
||||
let invocation = invocation(
|
||||
Arc::new(session),
|
||||
Arc::new(turn),
|
||||
"spawn_agent",
|
||||
function_payload(json!({
|
||||
"message": "inspect this repo",
|
||||
"agent_type": "explorer"
|
||||
})),
|
||||
);
|
||||
let output = CollabHandler
|
||||
.handle(invocation)
|
||||
.await
|
||||
.expect("spawn_agent should succeed");
|
||||
let ToolOutput::Function {
|
||||
body: FunctionCallOutputBody::Text(content),
|
||||
..
|
||||
} = output
|
||||
else {
|
||||
panic!("expected function output");
|
||||
};
|
||||
let result: SpawnAgentResult =
|
||||
serde_json::from_str(&content).expect("spawn_agent result should be json");
|
||||
let agent_id = agent_id(&result.agent_id).expect("agent_id should be valid");
|
||||
let snapshot = manager
|
||||
.get_thread(agent_id)
|
||||
.await
|
||||
.expect("spawned agent thread should exist")
|
||||
.config_snapshot()
|
||||
.await;
|
||||
assert_eq!(snapshot.approval_policy, AskForApproval::Never);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn spawn_agent_errors_when_manager_dropped() {
|
||||
let (session, turn) = make_session_and_context().await;
|
||||
|
||||
@@ -33,6 +33,7 @@ pub use read_file::ReadFileHandler;
|
||||
pub use request_user_input::RequestUserInputHandler;
|
||||
pub(crate) use request_user_input::request_user_input_tool_description;
|
||||
pub(crate) use search_tool_bm25::DEFAULT_LIMIT as SEARCH_TOOL_BM25_DEFAULT_LIMIT;
|
||||
pub(crate) use search_tool_bm25::SEARCH_TOOL_BM25_TOOL_NAME;
|
||||
pub use search_tool_bm25::SearchToolBm25Handler;
|
||||
pub use shell::ShellCommandHandler;
|
||||
pub use shell::ShellHandler;
|
||||
|
||||
@@ -10,7 +10,6 @@ use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
|
||||
use crate::connectors;
|
||||
use crate::features::Feature;
|
||||
use crate::function_tool::FunctionCallError;
|
||||
use crate::mcp::CODEX_APPS_MCP_SERVER_NAME;
|
||||
use crate::mcp_connection_manager::ToolInfo;
|
||||
@@ -23,6 +22,7 @@ use crate::tools::registry::ToolKind;
|
||||
|
||||
pub struct SearchToolBm25Handler;
|
||||
|
||||
pub(crate) const SEARCH_TOOL_BM25_TOOL_NAME: &str = "search_tool_bm25";
|
||||
pub(crate) const DEFAULT_LIMIT: usize = 8;
|
||||
|
||||
fn default_limit() -> usize {
|
||||
@@ -42,7 +42,6 @@ struct ToolEntry {
|
||||
server_name: String,
|
||||
title: Option<String>,
|
||||
description: Option<String>,
|
||||
connector_id: Option<String>,
|
||||
connector_name: Option<String>,
|
||||
input_keys: Vec<String>,
|
||||
search_text: String,
|
||||
@@ -66,7 +65,6 @@ impl ToolEntry {
|
||||
.tool
|
||||
.description
|
||||
.map(|description| description.to_string()),
|
||||
connector_id: info.connector_id,
|
||||
connector_name: info.connector_name,
|
||||
input_keys,
|
||||
search_text,
|
||||
@@ -91,9 +89,9 @@ impl ToolHandler for SearchToolBm25Handler {
|
||||
let arguments = match payload {
|
||||
ToolPayload::Function { arguments } => arguments,
|
||||
_ => {
|
||||
return Err(FunctionCallError::Fatal(
|
||||
"search_tool_bm25 handler received unsupported payload".to_string(),
|
||||
));
|
||||
return Err(FunctionCallError::Fatal(format!(
|
||||
"{SEARCH_TOOL_BM25_TOOL_NAME} handler received unsupported payload"
|
||||
)));
|
||||
}
|
||||
};
|
||||
|
||||
@@ -120,15 +118,12 @@ impl ToolHandler for SearchToolBm25Handler {
|
||||
.await
|
||||
.list_all_tools()
|
||||
.await;
|
||||
let mcp_tools = if turn.config.features.enabled(Feature::Apps) {
|
||||
let connectors = connectors::with_app_enabled_state(
|
||||
connectors::accessible_connectors_from_mcp_tools(&mcp_tools),
|
||||
&turn.config,
|
||||
);
|
||||
filter_codex_apps_mcp_tools(mcp_tools, &connectors)
|
||||
} else {
|
||||
mcp_tools
|
||||
};
|
||||
|
||||
let connectors = connectors::with_app_enabled_state(
|
||||
connectors::accessible_connectors_from_mcp_tools(&mcp_tools),
|
||||
&turn.config,
|
||||
);
|
||||
let mcp_tools = filter_codex_apps_mcp_tools(mcp_tools, &connectors);
|
||||
|
||||
let mut entries: Vec<ToolEntry> = mcp_tools
|
||||
.into_iter()
|
||||
@@ -172,7 +167,6 @@ impl ToolHandler for SearchToolBm25Handler {
|
||||
"server": entry.server_name.clone(),
|
||||
"title": entry.title.clone(),
|
||||
"description": entry.description.clone(),
|
||||
"connector_id": entry.connector_id.clone(),
|
||||
"connector_name": entry.connector_name.clone(),
|
||||
"input_keys": entry.input_keys.clone(),
|
||||
"score": result.score,
|
||||
@@ -243,12 +237,6 @@ fn build_search_text(name: &str, info: &ToolInfo, input_keys: &[String]) -> Stri
|
||||
parts.push(connector_name.to_string());
|
||||
}
|
||||
|
||||
if let Some(connector_id) = info.connector_id.as_deref()
|
||||
&& !connector_id.trim().is_empty()
|
||||
{
|
||||
parts.push(connector_id.to_string());
|
||||
}
|
||||
|
||||
if !input_keys.is_empty() {
|
||||
parts.extend(input_keys.iter().cloned());
|
||||
}
|
||||
|
||||
@@ -254,7 +254,14 @@ impl ShellHandler {
|
||||
let mut exec_params = exec_params;
|
||||
let dependency_env = session.dependency_env().await;
|
||||
if !dependency_env.is_empty() {
|
||||
exec_params.env.extend(dependency_env);
|
||||
exec_params.env.extend(dependency_env.clone());
|
||||
}
|
||||
|
||||
let mut explicit_env_overrides = turn.shell_environment_policy.r#set.clone();
|
||||
for key in dependency_env.keys() {
|
||||
if let Some(value) = exec_params.env.get(key) {
|
||||
explicit_env_overrides.insert(key.clone(), value.clone());
|
||||
}
|
||||
}
|
||||
|
||||
// Approval policy guard for explicit escalation in non-OnRequest modes.
|
||||
@@ -315,6 +322,7 @@ impl ShellHandler {
|
||||
cwd: exec_params.cwd.clone(),
|
||||
timeout_ms: exec_params.expiration.timeout_ms(),
|
||||
env: exec_params.env.clone(),
|
||||
explicit_env_overrides,
|
||||
network: exec_params.network.clone(),
|
||||
sandbox_permissions: exec_params.sandbox_permissions,
|
||||
justification: exec_params.justification.clone(),
|
||||
|
||||
@@ -15,6 +15,7 @@ use async_trait::async_trait;
|
||||
use codex_hooks::HookEvent;
|
||||
use codex_hooks::HookEventAfterToolUse;
|
||||
use codex_hooks::HookPayload;
|
||||
use codex_hooks::HookResult;
|
||||
use codex_hooks::HookToolInput;
|
||||
use codex_hooks::HookToolInputLocalShell;
|
||||
use codex_hooks::HookToolKind;
|
||||
@@ -172,7 +173,7 @@ impl ToolRegistry {
|
||||
Ok((preview, success)) => (preview.clone(), *success),
|
||||
Err(err) => (err.to_string(), false),
|
||||
};
|
||||
dispatch_after_tool_use_hook(AfterToolUseHookDispatch {
|
||||
let hook_abort_error = dispatch_after_tool_use_hook(AfterToolUseHookDispatch {
|
||||
invocation: &invocation,
|
||||
output_preview,
|
||||
success,
|
||||
@@ -182,6 +183,10 @@ impl ToolRegistry {
|
||||
})
|
||||
.await;
|
||||
|
||||
if let Some(err) = hook_abort_error {
|
||||
return Err(err);
|
||||
}
|
||||
|
||||
match result {
|
||||
Ok(_) => {
|
||||
let mut guard = output_cell.lock().await;
|
||||
@@ -339,12 +344,14 @@ struct AfterToolUseHookDispatch<'a> {
|
||||
mutating: bool,
|
||||
}
|
||||
|
||||
async fn dispatch_after_tool_use_hook(dispatch: AfterToolUseHookDispatch<'_>) {
|
||||
async fn dispatch_after_tool_use_hook(
|
||||
dispatch: AfterToolUseHookDispatch<'_>,
|
||||
) -> Option<FunctionCallError> {
|
||||
let AfterToolUseHookDispatch { invocation, .. } = dispatch;
|
||||
let session = invocation.session.as_ref();
|
||||
let turn = invocation.turn.as_ref();
|
||||
let tool_input = HookToolInput::from(&invocation.payload);
|
||||
session
|
||||
let hook_outcomes = session
|
||||
.hooks()
|
||||
.dispatch(HookPayload {
|
||||
session_id: session.conversation_id,
|
||||
@@ -373,4 +380,34 @@ async fn dispatch_after_tool_use_hook(dispatch: AfterToolUseHookDispatch<'_>) {
|
||||
},
|
||||
})
|
||||
.await;
|
||||
|
||||
for hook_outcome in hook_outcomes {
|
||||
let hook_name = hook_outcome.hook_name;
|
||||
match hook_outcome.result {
|
||||
HookResult::Success => {}
|
||||
HookResult::FailedContinue(error) => {
|
||||
warn!(
|
||||
call_id = %invocation.call_id,
|
||||
tool_name = %invocation.tool_name,
|
||||
hook_name = %hook_name,
|
||||
error = %error,
|
||||
"after_tool_use hook failed; continuing"
|
||||
);
|
||||
}
|
||||
HookResult::FailedAbort(error) => {
|
||||
warn!(
|
||||
call_id = %invocation.call_id,
|
||||
tool_name = %invocation.tool_name,
|
||||
hook_name = %hook_name,
|
||||
error = %error,
|
||||
"after_tool_use hook failed; aborting operation"
|
||||
);
|
||||
return Some(FunctionCallError::Fatal(format!(
|
||||
"after_tool_use hook '{hook_name}' failed and aborted operation: {error}"
|
||||
)));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
None
|
||||
}
|
||||
|
||||
@@ -57,7 +57,12 @@ pub(crate) fn maybe_wrap_shell_lc_with_snapshot(
|
||||
command: &[String],
|
||||
session_shell: &Shell,
|
||||
cwd: &Path,
|
||||
explicit_env_overrides: &HashMap<String, String>,
|
||||
) -> Vec<String> {
|
||||
if cfg!(windows) {
|
||||
return command.to_vec();
|
||||
}
|
||||
|
||||
let Some(snapshot) = session_shell.shell_snapshot() else {
|
||||
return command.to_vec();
|
||||
};
|
||||
@@ -95,24 +100,78 @@ pub(crate) fn maybe_wrap_shell_lc_with_snapshot(
|
||||
.iter()
|
||||
.map(|arg| format!(" '{}'", shell_single_quote(arg)))
|
||||
.collect::<String>();
|
||||
let rewritten_script = format!(
|
||||
"if . '{snapshot_path}' >/dev/null 2>&1; then :; fi; exec '{original_shell}' -c '{original_script}'{trailing_args}"
|
||||
);
|
||||
let (override_captures, override_exports) = build_override_exports(explicit_env_overrides);
|
||||
let rewritten_script = if override_exports.is_empty() {
|
||||
format!(
|
||||
"if . '{snapshot_path}' >/dev/null 2>&1; then :; fi\n\nexec '{original_shell}' -c '{original_script}'{trailing_args}"
|
||||
)
|
||||
} else {
|
||||
format!(
|
||||
"{override_captures}\n\nif . '{snapshot_path}' >/dev/null 2>&1; then :; fi\n\n{override_exports}\n\nexec '{original_shell}' -c '{original_script}'{trailing_args}"
|
||||
)
|
||||
};
|
||||
|
||||
vec![shell_path.to_string(), "-c".to_string(), rewritten_script]
|
||||
}
|
||||
|
||||
fn build_override_exports(explicit_env_overrides: &HashMap<String, String>) -> (String, String) {
|
||||
let mut keys = explicit_env_overrides
|
||||
.keys()
|
||||
.filter(|key| is_valid_shell_variable_name(key))
|
||||
.collect::<Vec<_>>();
|
||||
keys.sort_unstable();
|
||||
|
||||
if keys.is_empty() {
|
||||
return (String::new(), String::new());
|
||||
}
|
||||
|
||||
let captures = keys
|
||||
.iter()
|
||||
.enumerate()
|
||||
.map(|(idx, key)| {
|
||||
format!(
|
||||
"__CODEX_SNAPSHOT_OVERRIDE_SET_{idx}=\"${{{key}+x}}\"\n__CODEX_SNAPSHOT_OVERRIDE_{idx}=\"${{{key}-}}\""
|
||||
)
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n");
|
||||
let restores = keys
|
||||
.iter()
|
||||
.enumerate()
|
||||
.map(|(idx, key)| {
|
||||
format!(
|
||||
"if [ -n \"${{__CODEX_SNAPSHOT_OVERRIDE_SET_{idx}}}\" ]; then export {key}=\"${{__CODEX_SNAPSHOT_OVERRIDE_{idx}}}\"; else unset {key}; fi"
|
||||
)
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n");
|
||||
|
||||
(captures, restores)
|
||||
}
|
||||
|
||||
fn is_valid_shell_variable_name(name: &str) -> bool {
|
||||
let mut chars = name.chars();
|
||||
let Some(first) = chars.next() else {
|
||||
return false;
|
||||
};
|
||||
if !(first == '_' || first.is_ascii_alphabetic()) {
|
||||
return false;
|
||||
}
|
||||
chars.all(|c| c == '_' || c.is_ascii_alphanumeric())
|
||||
}
|
||||
|
||||
fn shell_single_quote(input: &str) -> String {
|
||||
input.replace('\'', r#"'"'"'"#)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[cfg(all(test, unix))]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::shell::ShellType;
|
||||
use crate::shell_snapshot::ShellSnapshot;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::path::PathBuf;
|
||||
use std::process::Command;
|
||||
use std::sync::Arc;
|
||||
use tempfile::tempdir;
|
||||
use tokio::sync::watch;
|
||||
@@ -151,7 +210,12 @@ mod tests {
|
||||
"echo hello".to_string(),
|
||||
];
|
||||
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(&command, &session_shell, dir.path());
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(
|
||||
&command,
|
||||
&session_shell,
|
||||
dir.path(),
|
||||
&HashMap::new(),
|
||||
);
|
||||
|
||||
assert_eq!(rewritten[0], "/bin/zsh");
|
||||
assert_eq!(rewritten[1], "-c");
|
||||
@@ -176,7 +240,12 @@ mod tests {
|
||||
"echo 'hello'".to_string(),
|
||||
];
|
||||
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(&command, &session_shell, dir.path());
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(
|
||||
&command,
|
||||
&session_shell,
|
||||
dir.path(),
|
||||
&HashMap::new(),
|
||||
);
|
||||
|
||||
assert!(rewritten[2].contains(r#"exec '/bin/bash' -c 'echo '"'"'hello'"'"''"#));
|
||||
}
|
||||
@@ -198,7 +267,12 @@ mod tests {
|
||||
"echo hello".to_string(),
|
||||
];
|
||||
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(&command, &session_shell, dir.path());
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(
|
||||
&command,
|
||||
&session_shell,
|
||||
dir.path(),
|
||||
&HashMap::new(),
|
||||
);
|
||||
|
||||
assert_eq!(rewritten[0], "/bin/bash");
|
||||
assert_eq!(rewritten[1], "-c");
|
||||
@@ -223,7 +297,12 @@ mod tests {
|
||||
"echo hello".to_string(),
|
||||
];
|
||||
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(&command, &session_shell, dir.path());
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(
|
||||
&command,
|
||||
&session_shell,
|
||||
dir.path(),
|
||||
&HashMap::new(),
|
||||
);
|
||||
|
||||
assert_eq!(rewritten[0], "/bin/sh");
|
||||
assert_eq!(rewritten[1], "-c");
|
||||
@@ -250,7 +329,12 @@ mod tests {
|
||||
"arg1".to_string(),
|
||||
];
|
||||
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(&command, &session_shell, dir.path());
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(
|
||||
&command,
|
||||
&session_shell,
|
||||
dir.path(),
|
||||
&HashMap::new(),
|
||||
);
|
||||
|
||||
assert!(
|
||||
rewritten[2].contains(
|
||||
@@ -276,7 +360,12 @@ mod tests {
|
||||
"echo hello".to_string(),
|
||||
];
|
||||
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(&command, &session_shell, &command_cwd);
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(
|
||||
&command,
|
||||
&session_shell,
|
||||
&command_cwd,
|
||||
&HashMap::new(),
|
||||
);
|
||||
|
||||
assert_eq!(rewritten, command);
|
||||
}
|
||||
@@ -299,11 +388,214 @@ mod tests {
|
||||
];
|
||||
let command_cwd = dir.path().join(".");
|
||||
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(&command, &session_shell, &command_cwd);
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(
|
||||
&command,
|
||||
&session_shell,
|
||||
&command_cwd,
|
||||
&HashMap::new(),
|
||||
);
|
||||
|
||||
assert_eq!(rewritten[0], "/bin/zsh");
|
||||
assert_eq!(rewritten[1], "-c");
|
||||
assert!(rewritten[2].contains("if . '"));
|
||||
assert!(rewritten[2].contains("exec '/bin/bash' -c 'echo hello'"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn maybe_wrap_shell_lc_with_snapshot_restores_explicit_override_precedence() {
|
||||
let dir = tempdir().expect("create temp dir");
|
||||
let snapshot_path = dir.path().join("snapshot.sh");
|
||||
std::fs::write(
|
||||
&snapshot_path,
|
||||
"# Snapshot file\nexport TEST_ENV_SNAPSHOT=global\nexport SNAPSHOT_ONLY=from_snapshot\n",
|
||||
)
|
||||
.expect("write snapshot");
|
||||
let session_shell = shell_with_snapshot(
|
||||
ShellType::Bash,
|
||||
"/bin/bash",
|
||||
snapshot_path,
|
||||
dir.path().to_path_buf(),
|
||||
);
|
||||
let command = vec![
|
||||
"/bin/bash".to_string(),
|
||||
"-lc".to_string(),
|
||||
"printf '%s|%s' \"$TEST_ENV_SNAPSHOT\" \"${SNAPSHOT_ONLY-unset}\"".to_string(),
|
||||
];
|
||||
let explicit_env_overrides =
|
||||
HashMap::from([("TEST_ENV_SNAPSHOT".to_string(), "worktree".to_string())]);
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(
|
||||
&command,
|
||||
&session_shell,
|
||||
dir.path(),
|
||||
&explicit_env_overrides,
|
||||
);
|
||||
let output = Command::new(&rewritten[0])
|
||||
.args(&rewritten[1..])
|
||||
.env("TEST_ENV_SNAPSHOT", "worktree")
|
||||
.output()
|
||||
.expect("run rewritten command");
|
||||
|
||||
assert!(output.status.success(), "command failed: {output:?}");
|
||||
assert_eq!(
|
||||
String::from_utf8_lossy(&output.stdout),
|
||||
"worktree|from_snapshot"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn maybe_wrap_shell_lc_with_snapshot_keeps_snapshot_path_without_override() {
|
||||
let dir = tempdir().expect("create temp dir");
|
||||
let snapshot_path = dir.path().join("snapshot.sh");
|
||||
std::fs::write(
|
||||
&snapshot_path,
|
||||
"# Snapshot file\nexport PATH='/snapshot/bin'\n",
|
||||
)
|
||||
.expect("write snapshot");
|
||||
let session_shell = shell_with_snapshot(
|
||||
ShellType::Bash,
|
||||
"/bin/bash",
|
||||
snapshot_path,
|
||||
dir.path().to_path_buf(),
|
||||
);
|
||||
let command = vec![
|
||||
"/bin/bash".to_string(),
|
||||
"-lc".to_string(),
|
||||
"printf '%s' \"$PATH\"".to_string(),
|
||||
];
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(
|
||||
&command,
|
||||
&session_shell,
|
||||
dir.path(),
|
||||
&HashMap::new(),
|
||||
);
|
||||
let output = Command::new(&rewritten[0])
|
||||
.args(&rewritten[1..])
|
||||
.output()
|
||||
.expect("run rewritten command");
|
||||
|
||||
assert!(output.status.success(), "command failed: {output:?}");
|
||||
assert_eq!(String::from_utf8_lossy(&output.stdout), "/snapshot/bin");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn maybe_wrap_shell_lc_with_snapshot_applies_explicit_path_override() {
|
||||
let dir = tempdir().expect("create temp dir");
|
||||
let snapshot_path = dir.path().join("snapshot.sh");
|
||||
std::fs::write(
|
||||
&snapshot_path,
|
||||
"# Snapshot file\nexport PATH='/snapshot/bin'\n",
|
||||
)
|
||||
.expect("write snapshot");
|
||||
let session_shell = shell_with_snapshot(
|
||||
ShellType::Bash,
|
||||
"/bin/bash",
|
||||
snapshot_path,
|
||||
dir.path().to_path_buf(),
|
||||
);
|
||||
let command = vec![
|
||||
"/bin/bash".to_string(),
|
||||
"-lc".to_string(),
|
||||
"printf '%s' \"$PATH\"".to_string(),
|
||||
];
|
||||
let explicit_env_overrides =
|
||||
HashMap::from([("PATH".to_string(), "/worktree/bin".to_string())]);
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(
|
||||
&command,
|
||||
&session_shell,
|
||||
dir.path(),
|
||||
&explicit_env_overrides,
|
||||
);
|
||||
let output = Command::new(&rewritten[0])
|
||||
.args(&rewritten[1..])
|
||||
.env("PATH", "/worktree/bin")
|
||||
.output()
|
||||
.expect("run rewritten command");
|
||||
|
||||
assert!(output.status.success(), "command failed: {output:?}");
|
||||
assert_eq!(String::from_utf8_lossy(&output.stdout), "/worktree/bin");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn maybe_wrap_shell_lc_with_snapshot_does_not_embed_override_values_in_argv() {
|
||||
let dir = tempdir().expect("create temp dir");
|
||||
let snapshot_path = dir.path().join("snapshot.sh");
|
||||
std::fs::write(
|
||||
&snapshot_path,
|
||||
"# Snapshot file\nexport OPENAI_API_KEY='snapshot-value'\n",
|
||||
)
|
||||
.expect("write snapshot");
|
||||
let session_shell = shell_with_snapshot(
|
||||
ShellType::Bash,
|
||||
"/bin/bash",
|
||||
snapshot_path,
|
||||
dir.path().to_path_buf(),
|
||||
);
|
||||
let command = vec![
|
||||
"/bin/bash".to_string(),
|
||||
"-lc".to_string(),
|
||||
"printf '%s' \"$OPENAI_API_KEY\"".to_string(),
|
||||
];
|
||||
let explicit_env_overrides = HashMap::from([(
|
||||
"OPENAI_API_KEY".to_string(),
|
||||
"super-secret-value".to_string(),
|
||||
)]);
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(
|
||||
&command,
|
||||
&session_shell,
|
||||
dir.path(),
|
||||
&explicit_env_overrides,
|
||||
);
|
||||
|
||||
assert!(!rewritten[2].contains("super-secret-value"));
|
||||
let output = Command::new(&rewritten[0])
|
||||
.args(&rewritten[1..])
|
||||
.env("OPENAI_API_KEY", "super-secret-value")
|
||||
.output()
|
||||
.expect("run rewritten command");
|
||||
assert!(output.status.success(), "command failed: {output:?}");
|
||||
assert_eq!(
|
||||
String::from_utf8_lossy(&output.stdout),
|
||||
"super-secret-value"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn maybe_wrap_shell_lc_with_snapshot_preserves_unset_override_variables() {
|
||||
let dir = tempdir().expect("create temp dir");
|
||||
let snapshot_path = dir.path().join("snapshot.sh");
|
||||
std::fs::write(
|
||||
&snapshot_path,
|
||||
"# Snapshot file\nexport CODEX_TEST_UNSET_OVERRIDE='snapshot-value'\n",
|
||||
)
|
||||
.expect("write snapshot");
|
||||
let session_shell = shell_with_snapshot(
|
||||
ShellType::Bash,
|
||||
"/bin/bash",
|
||||
snapshot_path,
|
||||
dir.path().to_path_buf(),
|
||||
);
|
||||
let command = vec![
|
||||
"/bin/bash".to_string(),
|
||||
"-lc".to_string(),
|
||||
"if [ \"${CODEX_TEST_UNSET_OVERRIDE+x}\" = x ]; then printf 'set:%s' \"$CODEX_TEST_UNSET_OVERRIDE\"; else printf 'unset'; fi".to_string(),
|
||||
];
|
||||
let explicit_env_overrides = HashMap::from([(
|
||||
"CODEX_TEST_UNSET_OVERRIDE".to_string(),
|
||||
"worktree-value".to_string(),
|
||||
)]);
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(
|
||||
&command,
|
||||
&session_shell,
|
||||
dir.path(),
|
||||
&explicit_env_overrides,
|
||||
);
|
||||
|
||||
let output = Command::new(&rewritten[0])
|
||||
.args(&rewritten[1..])
|
||||
.env_remove("CODEX_TEST_UNSET_OVERRIDE")
|
||||
.output()
|
||||
.expect("run rewritten command");
|
||||
assert!(output.status.success(), "command failed: {output:?}");
|
||||
assert_eq!(String::from_utf8_lossy(&output.stdout), "unset");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -37,6 +37,7 @@ pub struct ShellRequest {
|
||||
pub cwd: PathBuf,
|
||||
pub timeout_ms: Option<u64>,
|
||||
pub env: std::collections::HashMap<String, String>,
|
||||
pub explicit_env_overrides: std::collections::HashMap<String, String>,
|
||||
pub network: Option<NetworkProxy>,
|
||||
pub sandbox_permissions: SandboxPermissions,
|
||||
pub justification: Option<String>,
|
||||
@@ -166,8 +167,12 @@ impl ToolRuntime<ShellRequest, ExecToolCallOutput> for ShellRuntime {
|
||||
) -> Result<ExecToolCallOutput, ToolError> {
|
||||
let base_command = &req.command;
|
||||
let session_shell = ctx.session.user_shell();
|
||||
let command =
|
||||
maybe_wrap_shell_lc_with_snapshot(base_command, session_shell.as_ref(), &req.cwd);
|
||||
let command = maybe_wrap_shell_lc_with_snapshot(
|
||||
base_command,
|
||||
session_shell.as_ref(),
|
||||
&req.cwd,
|
||||
&req.explicit_env_overrides,
|
||||
);
|
||||
let command = if matches!(session_shell.shell_type, ShellType::PowerShell)
|
||||
&& ctx.session.features().enabled(Feature::PowershellUtf8)
|
||||
{
|
||||
|
||||
@@ -41,6 +41,7 @@ pub struct UnifiedExecRequest {
|
||||
pub command: Vec<String>,
|
||||
pub cwd: PathBuf,
|
||||
pub env: HashMap<String, String>,
|
||||
pub explicit_env_overrides: HashMap<String, String>,
|
||||
pub network: Option<NetworkProxy>,
|
||||
pub tty: bool,
|
||||
pub sandbox_permissions: SandboxPermissions,
|
||||
@@ -170,8 +171,12 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
) -> Result<UnifiedExecProcess, ToolError> {
|
||||
let base_command = &req.command;
|
||||
let session_shell = ctx.session.user_shell();
|
||||
let command =
|
||||
maybe_wrap_shell_lc_with_snapshot(base_command, session_shell.as_ref(), &req.cwd);
|
||||
let command = maybe_wrap_shell_lc_with_snapshot(
|
||||
base_command,
|
||||
session_shell.as_ref(),
|
||||
&req.cwd,
|
||||
&req.explicit_env_overrides,
|
||||
);
|
||||
let command = if matches!(session_shell.shell_type, ShellType::PowerShell)
|
||||
&& ctx.session.features().enabled(Feature::PowershellUtf8)
|
||||
{
|
||||
|
||||
@@ -1,4 +1,3 @@
|
||||
use crate::agent::AgentRole;
|
||||
use crate::client_common::tools::FreeformTool;
|
||||
use crate::client_common::tools::FreeformToolFormat;
|
||||
use crate::client_common::tools::ResponsesApiTool;
|
||||
@@ -8,6 +7,7 @@ use crate::features::Features;
|
||||
use crate::mcp_connection_manager::ToolInfo;
|
||||
use crate::tools::handlers::PLAN_TOOL;
|
||||
use crate::tools::handlers::SEARCH_TOOL_BM25_DEFAULT_LIMIT;
|
||||
use crate::tools::handlers::SEARCH_TOOL_BM25_TOOL_NAME;
|
||||
use crate::tools::handlers::apply_patch::create_apply_patch_freeform_tool;
|
||||
use crate::tools::handlers::apply_patch::create_apply_patch_json_tool;
|
||||
use crate::tools::handlers::collab::DEFAULT_WAIT_TIMEOUT_MS;
|
||||
@@ -541,10 +541,7 @@ fn create_spawn_agent_tool() -> ToolSpec {
|
||||
(
|
||||
"agent_type".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some(format!(
|
||||
"Optional agent type ({}). Use an explicit type when delegating.",
|
||||
AgentRole::enum_values().join(", ")
|
||||
)),
|
||||
description: Some(crate::agent::role::spawn_tool_spec::build()),
|
||||
},
|
||||
),
|
||||
]);
|
||||
@@ -913,7 +910,7 @@ fn create_search_tool_bm25_tool(app_tools: &HashMap<String, ToolInfo>) -> ToolSp
|
||||
SEARCH_TOOL_BM25_DESCRIPTION_TEMPLATE.replace("{{app_names}}", app_names.as_str());
|
||||
|
||||
ToolSpec::Function(ResponsesApiTool {
|
||||
name: "search_tool_bm25".to_string(),
|
||||
name: SEARCH_TOOL_BM25_TOOL_NAME.to_string(),
|
||||
description,
|
||||
strict: false,
|
||||
parameters: JsonSchema::Object {
|
||||
@@ -1507,7 +1504,7 @@ pub(crate) fn build_specs(
|
||||
&& let Some(app_tools) = app_tools
|
||||
{
|
||||
builder.push_spec_with_parallel_support(create_search_tool_bm25_tool(&app_tools), true);
|
||||
builder.register_handler("search_tool_bm25", search_tool_handler);
|
||||
builder.register_handler(SEARCH_TOOL_BM25_TOOL_NAME, search_tool_handler);
|
||||
}
|
||||
|
||||
if let Some(apply_patch_tool_type) = &config.apply_patch_tool_type {
|
||||
@@ -2579,7 +2576,7 @@ mod tests {
|
||||
)
|
||||
.build();
|
||||
|
||||
let search_tool = find_tool(&tools, "search_tool_bm25");
|
||||
let search_tool = find_tool(&tools, SEARCH_TOOL_BM25_TOOL_NAME);
|
||||
let ToolSpec::Function(ResponsesApiTool { description, .. }) = &search_tool.spec else {
|
||||
panic!("expected function tool");
|
||||
};
|
||||
|
||||
@@ -622,6 +622,7 @@ impl UnifiedExecProcessManager {
|
||||
command: request.command.clone(),
|
||||
cwd,
|
||||
env,
|
||||
explicit_env_overrides: context.turn.shell_environment_policy.r#set.clone(),
|
||||
network: request.network.clone(),
|
||||
tty: request.tty,
|
||||
sandbox_permissions: request.sandbox_permissions,
|
||||
|
||||
@@ -2,7 +2,7 @@
|
||||
|
||||
Searches over apps tool metadata with BM25 and exposes matching tools for the next model call.
|
||||
|
||||
When `search_tool_bm25` is available, apps tools (`mcp__codex_apps__...`) are hidden until you search for them.
|
||||
MCP tools of the apps ({{app_names}}) are hidden until you search for them with this tool (`search_tool_bm25`).
|
||||
|
||||
Follow this workflow:
|
||||
|
||||
@@ -10,8 +10,8 @@ Follow this workflow:
|
||||
- `query` (required): focused terms that describe the capability you need.
|
||||
- `limit` (optional): maximum number of tools to return (default `8`).
|
||||
2. Use the returned `tools` list to decide which Apps tools are relevant.
|
||||
3. Matching tools are added to available `tools` and available for the remainder of the current turn.
|
||||
4. Repeated searches in the same turn are additive: new matches are unioned into `tools`.
|
||||
3. Matching tools are added to available `tools` and available for the remainder of the current session/thread.
|
||||
4. Repeated searches in the same session/thread are additive: new matches are unioned into `tools`.
|
||||
|
||||
Notes:
|
||||
- Core tools remain available without searching.
|
||||
@@ -23,8 +23,6 @@ Notes:
|
||||
- `title`
|
||||
- `description`
|
||||
- `connector_name`
|
||||
- `connector_id`
|
||||
- input schema property keys (`input_keys`)
|
||||
- Use `search_tool_bm25` when the user asks to work with one of apps ({{app_names}}) and the exact tool name is not already known.
|
||||
- If the needed app is already explicit in the prompt (for example an `apps://...` mention) or already present in the current `tools` list, you can call that tool directly.
|
||||
- Do not use `search_tool_bm25` for non-apps/local tasks (filesystem, repo search, or shell-only workflows).
|
||||
- Do not use `search_tool_bm25` for non-apps/local tasks (filesystem, repo search, or shell-only workflows) or anything not related to {{app_names}}.
|
||||
|
||||
@@ -0,0 +1,158 @@
|
||||
use anyhow::Result;
|
||||
use serde_json::Value;
|
||||
use serde_json::json;
|
||||
use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
use wiremock::Request;
|
||||
use wiremock::Respond;
|
||||
use wiremock::ResponseTemplate;
|
||||
use wiremock::matchers::method;
|
||||
use wiremock::matchers::path;
|
||||
use wiremock::matchers::path_regex;
|
||||
|
||||
const CONNECTOR_ID: &str = "calendar";
|
||||
const CONNECTOR_NAME: &str = "Calendar";
|
||||
const PROTOCOL_VERSION: &str = "2025-11-25";
|
||||
const SERVER_NAME: &str = "codex-apps-test";
|
||||
const SERVER_VERSION: &str = "1.0.0";
|
||||
|
||||
#[derive(Clone)]
|
||||
pub struct AppsTestServer {
|
||||
pub chatgpt_base_url: String,
|
||||
}
|
||||
|
||||
impl AppsTestServer {
|
||||
pub async fn mount(server: &MockServer) -> Result<Self> {
|
||||
mount_oauth_metadata(server).await;
|
||||
mount_streamable_http_json_rpc(server).await;
|
||||
Ok(Self {
|
||||
chatgpt_base_url: server.uri(),
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
async fn mount_oauth_metadata(server: &MockServer) {
|
||||
Mock::given(method("GET"))
|
||||
.and(path("/.well-known/oauth-authorization-server/mcp"))
|
||||
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
|
||||
"authorization_endpoint": format!("{}/oauth/authorize", server.uri()),
|
||||
"token_endpoint": format!("{}/oauth/token", server.uri()),
|
||||
"scopes_supported": [""],
|
||||
})))
|
||||
.mount(server)
|
||||
.await;
|
||||
}
|
||||
|
||||
async fn mount_streamable_http_json_rpc(server: &MockServer) {
|
||||
Mock::given(method("POST"))
|
||||
.and(path_regex("^/api/codex/apps/?$"))
|
||||
.respond_with(CodexAppsJsonRpcResponder)
|
||||
.mount(server)
|
||||
.await;
|
||||
}
|
||||
|
||||
struct CodexAppsJsonRpcResponder;
|
||||
|
||||
impl Respond for CodexAppsJsonRpcResponder {
|
||||
fn respond(&self, request: &Request) -> ResponseTemplate {
|
||||
let body: Value = match serde_json::from_slice(&request.body) {
|
||||
Ok(body) => body,
|
||||
Err(error) => {
|
||||
return ResponseTemplate::new(400).set_body_json(json!({
|
||||
"error": format!("invalid JSON-RPC body: {error}"),
|
||||
}));
|
||||
}
|
||||
};
|
||||
|
||||
let Some(method) = body.get("method").and_then(Value::as_str) else {
|
||||
return ResponseTemplate::new(400).set_body_json(json!({
|
||||
"error": "missing method in JSON-RPC request",
|
||||
}));
|
||||
};
|
||||
|
||||
match method {
|
||||
"initialize" => {
|
||||
let id = body.get("id").cloned().unwrap_or(Value::Null);
|
||||
let protocol_version = body
|
||||
.pointer("/params/protocolVersion")
|
||||
.and_then(Value::as_str)
|
||||
.unwrap_or(PROTOCOL_VERSION);
|
||||
ResponseTemplate::new(200).set_body_json(json!({
|
||||
"jsonrpc": "2.0",
|
||||
"id": id,
|
||||
"result": {
|
||||
"protocolVersion": protocol_version,
|
||||
"capabilities": {
|
||||
"tools": {
|
||||
"listChanged": true
|
||||
}
|
||||
},
|
||||
"serverInfo": {
|
||||
"name": SERVER_NAME,
|
||||
"version": SERVER_VERSION
|
||||
}
|
||||
}
|
||||
}))
|
||||
}
|
||||
"notifications/initialized" => ResponseTemplate::new(202),
|
||||
"tools/list" => {
|
||||
let id = body.get("id").cloned().unwrap_or(Value::Null);
|
||||
ResponseTemplate::new(200).set_body_json(json!({
|
||||
"jsonrpc": "2.0",
|
||||
"id": id,
|
||||
"result": {
|
||||
"tools": [
|
||||
{
|
||||
"name": "calendar_create_event",
|
||||
"description": "Create a calendar event.",
|
||||
"inputSchema": {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"title": { "type": "string" },
|
||||
"starts_at": { "type": "string" },
|
||||
"timezone": { "type": "string" }
|
||||
},
|
||||
"required": ["title", "starts_at"],
|
||||
"additionalProperties": false
|
||||
},
|
||||
"_meta": {
|
||||
"connector_id": CONNECTOR_ID,
|
||||
"connector_name": CONNECTOR_NAME
|
||||
}
|
||||
},
|
||||
{
|
||||
"name": "calendar_list_events",
|
||||
"description": "List calendar events.",
|
||||
"inputSchema": {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"query": { "type": "string" },
|
||||
"limit": { "type": "integer" }
|
||||
},
|
||||
"additionalProperties": false
|
||||
},
|
||||
"_meta": {
|
||||
"connector_id": CONNECTOR_ID,
|
||||
"connector_name": CONNECTOR_NAME
|
||||
}
|
||||
}
|
||||
],
|
||||
"nextCursor": null
|
||||
}
|
||||
}))
|
||||
}
|
||||
method if method.starts_with("notifications/") => ResponseTemplate::new(202),
|
||||
_ => {
|
||||
let id = body.get("id").cloned().unwrap_or(Value::Null);
|
||||
ResponseTemplate::new(200).set_body_json(json!({
|
||||
"jsonrpc": "2.0",
|
||||
"id": id,
|
||||
"error": {
|
||||
"code": -32601,
|
||||
"message": format!("method not found: {method}")
|
||||
}
|
||||
}))
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -12,6 +12,7 @@ use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use regex_lite::Regex;
|
||||
use std::path::PathBuf;
|
||||
|
||||
pub mod apps_test_server;
|
||||
pub mod context_snapshot;
|
||||
pub mod process;
|
||||
pub mod responses;
|
||||
@@ -25,6 +26,26 @@ fn enable_deterministic_unified_exec_process_ids_for_tests() {
|
||||
codex_core::test_support::set_deterministic_process_ids(true);
|
||||
}
|
||||
|
||||
#[ctor]
|
||||
fn configure_insta_workspace_root_for_snapshot_tests() {
|
||||
if std::env::var_os("INSTA_WORKSPACE_ROOT").is_some() {
|
||||
return;
|
||||
}
|
||||
|
||||
let workspace_root = codex_utils_cargo_bin::repo_root()
|
||||
.ok()
|
||||
.map(|root| root.join("codex-rs"));
|
||||
|
||||
if let Some(workspace_root) = workspace_root
|
||||
&& let Ok(workspace_root) = workspace_root.canonicalize()
|
||||
{
|
||||
// Safety: this ctor runs at process startup before test threads begin.
|
||||
unsafe {
|
||||
std::env::set_var("INSTA_WORKSPACE_ROOT", workspace_root);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[track_caller]
|
||||
pub fn assert_regex_match<'s>(pattern: &str, actual: &'s str) -> regex_lite::Captures<'s> {
|
||||
let regex = Regex::new(pattern).unwrap_or_else(|err| {
|
||||
|
||||
@@ -34,6 +34,7 @@ use codex_protocol::models::ReasoningItemReasoningSummary;
|
||||
use codex_protocol::models::WebSearchAction;
|
||||
use codex_protocol::openai_models::ReasoningEffort;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use core_test_support::apps_test_server::AppsTestServer;
|
||||
use core_test_support::load_default_config_for_test;
|
||||
use core_test_support::responses::ev_completed;
|
||||
use core_test_support::responses::ev_completed_with_tokens;
|
||||
@@ -671,6 +672,10 @@ async fn includes_user_instructions_message_in_request() {
|
||||
async fn includes_apps_guidance_as_developer_message_when_enabled() {
|
||||
skip_if_no_network!();
|
||||
let server = MockServer::start().await;
|
||||
let apps_server = AppsTestServer::mount(&server)
|
||||
.await
|
||||
.expect("mount apps MCP mock");
|
||||
let apps_base_url = apps_server.chatgpt_base_url.clone();
|
||||
|
||||
let resp_mock = mount_sse_once(
|
||||
&server,
|
||||
@@ -680,8 +685,10 @@ async fn includes_apps_guidance_as_developer_message_when_enabled() {
|
||||
|
||||
let mut builder = test_codex()
|
||||
.with_auth(CodexAuth::from_api_key("Test API Key"))
|
||||
.with_config(|config| {
|
||||
.with_config(move |config| {
|
||||
config.features.enable(Feature::Apps);
|
||||
config.features.disable(Feature::AppsMcpGateway);
|
||||
config.chatgpt_base_url = apps_base_url;
|
||||
});
|
||||
let codex = builder
|
||||
.build(&server)
|
||||
|
||||
@@ -228,6 +228,7 @@ async fn model_change_from_image_to_text_strips_prior_image_content() -> Result<
|
||||
supported_in_api: true,
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
priority: 1,
|
||||
upgrade: None,
|
||||
base_instructions: "base instructions".to_string(),
|
||||
@@ -385,6 +386,7 @@ async fn model_switch_to_smaller_model_updates_token_context_window() -> Result<
|
||||
supported_in_api: true,
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
priority: 1,
|
||||
upgrade: None,
|
||||
base_instructions: "base instructions".to_string(),
|
||||
|
||||
@@ -352,5 +352,6 @@ fn test_remote_model(slug: &str, priority: i32) -> ModelInfo {
|
||||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -616,6 +616,7 @@ async fn ignores_remote_personality_if_remote_models_disabled() -> anyhow::Resul
|
||||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
};
|
||||
|
||||
let _models_mock = mount_models_once(
|
||||
@@ -733,6 +734,7 @@ async fn remote_model_friendly_personality_instructions_with_feature() -> anyhow
|
||||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
};
|
||||
|
||||
let _models_mock = mount_models_once(
|
||||
@@ -845,6 +847,7 @@ async fn user_turn_personality_remote_model_template_includes_update_message() -
|
||||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
};
|
||||
|
||||
let _models_mock = mount_models_once(
|
||||
|
||||
@@ -225,6 +225,7 @@ async fn remote_models_remote_model_uses_unified_exec() -> Result<()> {
|
||||
supported_in_api: true,
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
priority: 1,
|
||||
upgrade: None,
|
||||
base_instructions: "base instructions".to_string(),
|
||||
@@ -464,6 +465,7 @@ async fn remote_models_apply_remote_base_instructions() -> Result<()> {
|
||||
supported_in_api: true,
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
priority: 1,
|
||||
upgrade: None,
|
||||
base_instructions: remote_base.to_string(),
|
||||
@@ -948,6 +950,7 @@ fn test_remote_model_with_policy(
|
||||
supported_in_api: true,
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
priority,
|
||||
upgrade: None,
|
||||
base_instructions: "base instructions".to_string(),
|
||||
|
||||
@@ -74,7 +74,14 @@ async fn emits_warning_when_resumed_model_differs() {
|
||||
.expect("resume conversation");
|
||||
|
||||
// Assert: a Warning event is emitted describing the model mismatch.
|
||||
let warning = wait_for_event(&conversation, |ev| matches!(ev, EventMsg::Warning(_))).await;
|
||||
let warning = wait_for_event(&conversation, |ev| {
|
||||
matches!(
|
||||
ev,
|
||||
EventMsg::Warning(WarningEvent { message })
|
||||
if message.contains("previous-model") && message.contains("current-model")
|
||||
)
|
||||
})
|
||||
.await;
|
||||
let EventMsg::Warning(WarningEvent { message }) = warning else {
|
||||
panic!("expected warning event");
|
||||
};
|
||||
|
||||
@@ -410,6 +410,7 @@ async fn stdio_image_responses_are_sanitized_for_text_only_model() -> anyhow::Re
|
||||
experimental_supported_tools: Vec::new(),
|
||||
input_modalities: vec![InputModality::Text],
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
}],
|
||||
},
|
||||
)
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
@@ -20,6 +20,7 @@ use core_test_support::wait_for_event;
|
||||
use core_test_support::wait_for_event_match;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
use std::collections::HashMap;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use tokio::fs;
|
||||
@@ -36,6 +37,17 @@ struct SnapshotRun {
|
||||
codex_home: PathBuf,
|
||||
}
|
||||
|
||||
const POLICY_PATH_FOR_TEST: &str = "/codex/policy/path";
|
||||
const SNAPSHOT_PATH_FOR_TEST: &str = "/codex/snapshot/path";
|
||||
const SNAPSHOT_MARKER_VAR: &str = "CODEX_SNAPSHOT_POLICY_MARKER";
|
||||
const SNAPSHOT_MARKER_VALUE: &str = "from_snapshot";
|
||||
const POLICY_SUCCESS_OUTPUT: &str = "policy-after-snapshot";
|
||||
|
||||
#[derive(Debug, Default)]
|
||||
struct SnapshotRunOptions {
|
||||
shell_environment_set: HashMap<String, String>,
|
||||
}
|
||||
|
||||
async fn wait_for_snapshot(codex_home: &Path) -> Result<PathBuf> {
|
||||
let snapshot_dir = codex_home.join("shell_snapshots");
|
||||
let deadline = Instant::now() + Duration::from_secs(5);
|
||||
@@ -54,12 +66,57 @@ async fn wait_for_snapshot(codex_home: &Path) -> Result<PathBuf> {
|
||||
}
|
||||
}
|
||||
|
||||
async fn wait_for_file_contents(path: &Path) -> Result<String> {
|
||||
let deadline = Instant::now() + Duration::from_secs(5);
|
||||
loop {
|
||||
match fs::read_to_string(path).await {
|
||||
Ok(contents) => return Ok(contents),
|
||||
Err(err) if err.kind() == std::io::ErrorKind::NotFound => {}
|
||||
Err(err) => return Err(err.into()),
|
||||
}
|
||||
|
||||
if Instant::now() >= deadline {
|
||||
anyhow::bail!("timed out waiting for file {}", path.display());
|
||||
}
|
||||
|
||||
sleep(Duration::from_millis(25)).await;
|
||||
}
|
||||
}
|
||||
|
||||
fn policy_set_path_for_test() -> HashMap<String, String> {
|
||||
HashMap::from([("PATH".to_string(), POLICY_PATH_FOR_TEST.to_string())])
|
||||
}
|
||||
|
||||
fn snapshot_override_content_for_policy_test() -> String {
|
||||
format!(
|
||||
"# Snapshot file\nexport PATH='{SNAPSHOT_PATH_FOR_TEST}'\nexport {SNAPSHOT_MARKER_VAR}='{SNAPSHOT_MARKER_VALUE}'\n"
|
||||
)
|
||||
}
|
||||
|
||||
fn command_asserting_policy_after_snapshot() -> String {
|
||||
format!(
|
||||
"if [ \"${{{SNAPSHOT_MARKER_VAR}:-}}\" = \"{SNAPSHOT_MARKER_VALUE}\" ] && [ \"$PATH\" != \"{SNAPSHOT_PATH_FOR_TEST}\" ]; then case \":$PATH:\" in *\":{POLICY_PATH_FOR_TEST}:\"*) printf \"{POLICY_SUCCESS_OUTPUT}\" ;; *) printf \"path=%s marker=%s\" \"$PATH\" \"${{{SNAPSHOT_MARKER_VAR}:-missing}}\" ;; esac; else printf \"path=%s marker=%s\" \"$PATH\" \"${{{SNAPSHOT_MARKER_VAR}:-missing}}\"; fi"
|
||||
)
|
||||
}
|
||||
|
||||
#[allow(clippy::expect_used)]
|
||||
async fn run_snapshot_command(command: &str) -> Result<SnapshotRun> {
|
||||
let builder = test_codex().with_config(|config| {
|
||||
run_snapshot_command_with_options(command, SnapshotRunOptions::default()).await
|
||||
}
|
||||
|
||||
#[allow(clippy::expect_used)]
|
||||
async fn run_snapshot_command_with_options(
|
||||
command: &str,
|
||||
options: SnapshotRunOptions,
|
||||
) -> Result<SnapshotRun> {
|
||||
let SnapshotRunOptions {
|
||||
shell_environment_set,
|
||||
} = options;
|
||||
let builder = test_codex().with_config(move |config| {
|
||||
config.use_experimental_unified_exec_tool = true;
|
||||
config.features.enable(Feature::UnifiedExec);
|
||||
config.features.enable(Feature::ShellSnapshot);
|
||||
config.permissions.shell_environment_policy.r#set = shell_environment_set;
|
||||
});
|
||||
let harness = TestCodexHarness::with_builder(builder).await?;
|
||||
let args = json!({
|
||||
@@ -132,8 +189,20 @@ async fn run_snapshot_command(command: &str) -> Result<SnapshotRun> {
|
||||
|
||||
#[allow(clippy::expect_used)]
|
||||
async fn run_shell_command_snapshot(command: &str) -> Result<SnapshotRun> {
|
||||
let builder = test_codex().with_config(|config| {
|
||||
run_shell_command_snapshot_with_options(command, SnapshotRunOptions::default()).await
|
||||
}
|
||||
|
||||
#[allow(clippy::expect_used)]
|
||||
async fn run_shell_command_snapshot_with_options(
|
||||
command: &str,
|
||||
options: SnapshotRunOptions,
|
||||
) -> Result<SnapshotRun> {
|
||||
let SnapshotRunOptions {
|
||||
shell_environment_set,
|
||||
} = options;
|
||||
let builder = test_codex().with_config(move |config| {
|
||||
config.features.enable(Feature::ShellSnapshot);
|
||||
config.permissions.shell_environment_policy.r#set = shell_environment_set;
|
||||
});
|
||||
let harness = TestCodexHarness::with_builder(builder).await?;
|
||||
let args = json!({
|
||||
@@ -204,6 +273,64 @@ async fn run_shell_command_snapshot(command: &str) -> Result<SnapshotRun> {
|
||||
})
|
||||
}
|
||||
|
||||
#[allow(clippy::expect_used)]
|
||||
async fn run_tool_turn_on_harness(
|
||||
harness: &TestCodexHarness,
|
||||
prompt: &str,
|
||||
call_id: &str,
|
||||
tool_name: &str,
|
||||
args: serde_json::Value,
|
||||
) -> Result<ExecCommandEndEvent> {
|
||||
let responses = vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_function_call(call_id, tool_name, &serde_json::to_string(&args)?),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-2"),
|
||||
ev_assistant_message("msg-1", "done"),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
];
|
||||
mount_sse_sequence(harness.server(), responses).await;
|
||||
|
||||
let test = harness.test();
|
||||
let codex = test.codex.clone();
|
||||
let session_model = test.session_configured.model.clone();
|
||||
let cwd = test.cwd_path().to_path_buf();
|
||||
codex
|
||||
.submit(Op::UserTurn {
|
||||
items: vec![UserInput::Text {
|
||||
text: prompt.into(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
final_output_json_schema: None,
|
||||
cwd,
|
||||
approval_policy: AskForApproval::Never,
|
||||
sandbox_policy: SandboxPolicy::DangerFullAccess,
|
||||
model: session_model,
|
||||
effort: None,
|
||||
summary: ReasoningSummary::Auto,
|
||||
collaboration_mode: None,
|
||||
personality: None,
|
||||
})
|
||||
.await?;
|
||||
|
||||
wait_for_event_match(&codex, |ev| match ev {
|
||||
EventMsg::ExecCommandBegin(ev) if ev.call_id == call_id => Some(ev.clone()),
|
||||
_ => None,
|
||||
})
|
||||
.await;
|
||||
let end = wait_for_event_match(&codex, |ev| match ev {
|
||||
EventMsg::ExecCommandEnd(ev) if ev.call_id == call_id => Some(ev.clone()),
|
||||
_ => None,
|
||||
})
|
||||
.await;
|
||||
wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await;
|
||||
Ok(end)
|
||||
}
|
||||
|
||||
fn normalize_newlines(text: &str) -> String {
|
||||
text.replace("\r\n", "\n")
|
||||
}
|
||||
@@ -260,6 +387,100 @@ async fn linux_shell_command_uses_shell_snapshot() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg_attr(target_os = "windows", ignore)]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn shell_command_snapshot_preserves_shell_environment_policy_set() -> Result<()> {
|
||||
let builder = test_codex().with_config(|config| {
|
||||
config.features.enable(Feature::ShellSnapshot);
|
||||
config.permissions.shell_environment_policy.r#set = policy_set_path_for_test();
|
||||
});
|
||||
let harness = TestCodexHarness::with_builder(builder).await?;
|
||||
let codex_home = harness.test().home.path().to_path_buf();
|
||||
run_tool_turn_on_harness(
|
||||
&harness,
|
||||
"warm up shell snapshot",
|
||||
"shell-snapshot-policy-warmup",
|
||||
"shell_command",
|
||||
json!({
|
||||
"command": "printf warmup",
|
||||
"timeout_ms": 1_000,
|
||||
}),
|
||||
)
|
||||
.await?;
|
||||
let snapshot_path = wait_for_snapshot(&codex_home).await?;
|
||||
fs::write(&snapshot_path, snapshot_override_content_for_policy_test()).await?;
|
||||
|
||||
let command = command_asserting_policy_after_snapshot();
|
||||
let end = run_tool_turn_on_harness(
|
||||
&harness,
|
||||
"verify shell policy after snapshot",
|
||||
"shell-snapshot-policy-assert",
|
||||
"shell_command",
|
||||
json!({
|
||||
"command": command,
|
||||
"timeout_ms": 1_000,
|
||||
}),
|
||||
)
|
||||
.await?;
|
||||
|
||||
assert_eq!(
|
||||
normalize_newlines(&end.stdout).trim(),
|
||||
POLICY_SUCCESS_OUTPUT
|
||||
);
|
||||
assert_eq!(end.exit_code, 0);
|
||||
assert!(snapshot_path.starts_with(codex_home));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg_attr(not(target_os = "linux"), ignore)]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn linux_unified_exec_snapshot_preserves_shell_environment_policy_set() -> Result<()> {
|
||||
let builder = test_codex().with_config(|config| {
|
||||
config.use_experimental_unified_exec_tool = true;
|
||||
config.features.enable(Feature::UnifiedExec);
|
||||
config.features.enable(Feature::ShellSnapshot);
|
||||
config.permissions.shell_environment_policy.r#set = policy_set_path_for_test();
|
||||
});
|
||||
let harness = TestCodexHarness::with_builder(builder).await?;
|
||||
let codex_home = harness.test().home.path().to_path_buf();
|
||||
run_tool_turn_on_harness(
|
||||
&harness,
|
||||
"warm up unified exec shell snapshot",
|
||||
"shell-snapshot-policy-warmup-exec",
|
||||
"exec_command",
|
||||
json!({
|
||||
"cmd": "printf warmup",
|
||||
"yield_time_ms": 1_000,
|
||||
}),
|
||||
)
|
||||
.await?;
|
||||
let snapshot_path = wait_for_snapshot(&codex_home).await?;
|
||||
fs::write(&snapshot_path, snapshot_override_content_for_policy_test()).await?;
|
||||
|
||||
let command = command_asserting_policy_after_snapshot();
|
||||
let end = run_tool_turn_on_harness(
|
||||
&harness,
|
||||
"verify unified exec policy after snapshot",
|
||||
"shell-snapshot-policy-assert-exec",
|
||||
"exec_command",
|
||||
json!({
|
||||
"cmd": command,
|
||||
"yield_time_ms": 1_000,
|
||||
}),
|
||||
)
|
||||
.await?;
|
||||
|
||||
assert_eq!(
|
||||
normalize_newlines(&end.stdout).trim(),
|
||||
POLICY_SUCCESS_OUTPUT
|
||||
);
|
||||
assert_eq!(end.exit_code, 0);
|
||||
assert!(snapshot_path.starts_with(codex_home));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg_attr(target_os = "windows", ignore)]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn shell_command_snapshot_still_intercepts_apply_patch() -> Result<()> {
|
||||
@@ -316,7 +537,10 @@ async fn shell_command_snapshot_still_intercepts_apply_patch() -> Result<()> {
|
||||
|
||||
wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await;
|
||||
|
||||
assert_eq!(fs::read_to_string(&target).await?, "hello from snapshot\n");
|
||||
assert_eq!(
|
||||
wait_for_file_contents(&target).await?,
|
||||
"hello from snapshot\n"
|
||||
);
|
||||
|
||||
let snapshot_path = wait_for_snapshot(&codex_home).await?;
|
||||
let snapshot_content = fs::read_to_string(&snapshot_path).await?;
|
||||
|
||||
@@ -671,6 +671,7 @@ async fn view_image_tool_returns_unsupported_message_for_text_only_model() -> an
|
||||
supported_in_api: true,
|
||||
input_modalities: vec![InputModality::Text],
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
priority: 1,
|
||||
upgrade: None,
|
||||
base_instructions: "base instructions".to_string(),
|
||||
|
||||
@@ -9,8 +9,9 @@ pub use types::Hook;
|
||||
pub use types::HookEvent;
|
||||
pub use types::HookEventAfterAgent;
|
||||
pub use types::HookEventAfterToolUse;
|
||||
pub use types::HookOutcome;
|
||||
pub use types::HookPayload;
|
||||
pub use types::HookResponse;
|
||||
pub use types::HookResult;
|
||||
pub use types::HookToolInput;
|
||||
pub use types::HookToolInputLocalShell;
|
||||
pub use types::HookToolKind;
|
||||
|
||||
+125
-23
@@ -2,8 +2,8 @@ use tokio::process::Command;
|
||||
|
||||
use crate::types::Hook;
|
||||
use crate::types::HookEvent;
|
||||
use crate::types::HookOutcome;
|
||||
use crate::types::HookPayload;
|
||||
use crate::types::HookResponse;
|
||||
|
||||
#[derive(Default, Clone)]
|
||||
pub struct HooksConfig {
|
||||
@@ -45,14 +45,19 @@ impl Hooks {
|
||||
}
|
||||
}
|
||||
|
||||
pub async fn dispatch(&self, hook_payload: HookPayload) {
|
||||
// TODO(gt): support interrupting program execution by returning a result here.
|
||||
for hook in self.hooks_for_event(&hook_payload.hook_event) {
|
||||
pub async fn dispatch(&self, hook_payload: HookPayload) -> Vec<HookResponse> {
|
||||
let hooks = self.hooks_for_event(&hook_payload.hook_event);
|
||||
let mut outcomes = Vec::with_capacity(hooks.len());
|
||||
for hook in hooks {
|
||||
let outcome = hook.execute(&hook_payload).await;
|
||||
if matches!(outcome, HookOutcome::Stop) {
|
||||
let should_abort_operation = outcome.result.should_abort_operation();
|
||||
outcomes.push(outcome);
|
||||
if should_abort_operation {
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
outcomes
|
||||
}
|
||||
}
|
||||
|
||||
@@ -88,6 +93,7 @@ mod tests {
|
||||
use super::*;
|
||||
use crate::types::HookEventAfterAgent;
|
||||
use crate::types::HookEventAfterToolUse;
|
||||
use crate::types::HookResult;
|
||||
use crate::types::HookToolInput;
|
||||
use crate::types::HookToolKind;
|
||||
|
||||
@@ -113,14 +119,50 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
fn counting_hook(calls: &Arc<AtomicUsize>, outcome: HookOutcome) -> Hook {
|
||||
fn counting_success_hook(calls: &Arc<AtomicUsize>, name: &str) -> Hook {
|
||||
let hook_name = name.to_string();
|
||||
let calls = Arc::clone(calls);
|
||||
Hook {
|
||||
name: hook_name,
|
||||
func: Arc::new(move |_| {
|
||||
let calls = Arc::clone(&calls);
|
||||
Box::pin(async move {
|
||||
calls.fetch_add(1, Ordering::SeqCst);
|
||||
outcome
|
||||
HookResult::Success
|
||||
})
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
fn failing_continue_hook(calls: &Arc<AtomicUsize>, name: &str, message: &str) -> Hook {
|
||||
let hook_name = name.to_string();
|
||||
let message = message.to_string();
|
||||
let calls = Arc::clone(calls);
|
||||
Hook {
|
||||
name: hook_name,
|
||||
func: Arc::new(move |_| {
|
||||
let calls = Arc::clone(&calls);
|
||||
let message = message.clone();
|
||||
Box::pin(async move {
|
||||
calls.fetch_add(1, Ordering::SeqCst);
|
||||
HookResult::FailedContinue(std::io::Error::other(message).into())
|
||||
})
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
fn failing_abort_hook(calls: &Arc<AtomicUsize>, name: &str, message: &str) -> Hook {
|
||||
let hook_name = name.to_string();
|
||||
let message = message.to_string();
|
||||
let calls = Arc::clone(calls);
|
||||
Hook {
|
||||
name: hook_name,
|
||||
func: Arc::new(move |_| {
|
||||
let calls = Arc::clone(&calls);
|
||||
let message = message.clone();
|
||||
Box::pin(async move {
|
||||
calls.fetch_add(1, Ordering::SeqCst);
|
||||
HookResult::FailedAbort(std::io::Error::other(message).into())
|
||||
})
|
||||
}),
|
||||
}
|
||||
@@ -212,11 +254,14 @@ mod tests {
|
||||
async fn dispatch_executes_hook() {
|
||||
let calls = Arc::new(AtomicUsize::new(0));
|
||||
let hooks = Hooks {
|
||||
after_agent: vec![counting_hook(&calls, HookOutcome::Continue)],
|
||||
after_agent: vec![counting_success_hook(&calls, "counting")],
|
||||
..Hooks::default()
|
||||
};
|
||||
|
||||
hooks.dispatch(hook_payload("1")).await;
|
||||
let outcomes = hooks.dispatch(hook_payload("1")).await;
|
||||
assert_eq!(outcomes.len(), 1);
|
||||
assert_eq!(outcomes[0].hook_name, "counting");
|
||||
assert!(matches!(outcomes[0].result, HookResult::Success));
|
||||
assert_eq!(calls.load(Ordering::SeqCst), 1);
|
||||
}
|
||||
|
||||
@@ -224,7 +269,8 @@ mod tests {
|
||||
async fn default_hook_is_noop_and_continues() {
|
||||
let payload = hook_payload("d");
|
||||
let outcome = Hook::default().execute(&payload).await;
|
||||
assert_eq!(outcome, HookOutcome::Continue);
|
||||
assert_eq!(outcome.hook_name, "default");
|
||||
assert!(matches!(outcome.result, HookResult::Success));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -232,28 +278,36 @@ mod tests {
|
||||
let calls = Arc::new(AtomicUsize::new(0));
|
||||
let hooks = Hooks {
|
||||
after_agent: vec![
|
||||
counting_hook(&calls, HookOutcome::Continue),
|
||||
counting_hook(&calls, HookOutcome::Continue),
|
||||
counting_success_hook(&calls, "counting-1"),
|
||||
counting_success_hook(&calls, "counting-2"),
|
||||
],
|
||||
..Hooks::default()
|
||||
};
|
||||
|
||||
hooks.dispatch(hook_payload("2")).await;
|
||||
let outcomes = hooks.dispatch(hook_payload("2")).await;
|
||||
assert_eq!(outcomes.len(), 2);
|
||||
assert_eq!(outcomes[0].hook_name, "counting-1");
|
||||
assert_eq!(outcomes[1].hook_name, "counting-2");
|
||||
assert!(matches!(outcomes[0].result, HookResult::Success));
|
||||
assert!(matches!(outcomes[1].result, HookResult::Success));
|
||||
assert_eq!(calls.load(Ordering::SeqCst), 2);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn dispatch_stops_when_hook_returns_stop() {
|
||||
async fn dispatch_stops_when_hook_requests_abort() {
|
||||
let calls = Arc::new(AtomicUsize::new(0));
|
||||
let hooks = Hooks {
|
||||
after_agent: vec![
|
||||
counting_hook(&calls, HookOutcome::Stop),
|
||||
counting_hook(&calls, HookOutcome::Continue),
|
||||
failing_abort_hook(&calls, "abort", "hook failed"),
|
||||
counting_success_hook(&calls, "counting"),
|
||||
],
|
||||
..Hooks::default()
|
||||
};
|
||||
|
||||
hooks.dispatch(hook_payload("3")).await;
|
||||
let outcomes = hooks.dispatch(hook_payload("3")).await;
|
||||
assert_eq!(outcomes.len(), 1);
|
||||
assert_eq!(outcomes[0].hook_name, "abort");
|
||||
assert!(matches!(outcomes[0].result, HookResult::FailedAbort(_)));
|
||||
assert_eq!(calls.load(Ordering::SeqCst), 1);
|
||||
}
|
||||
|
||||
@@ -261,11 +315,53 @@ mod tests {
|
||||
async fn dispatch_executes_after_tool_use_hooks() {
|
||||
let calls = Arc::new(AtomicUsize::new(0));
|
||||
let hooks = Hooks {
|
||||
after_tool_use: vec![counting_hook(&calls, HookOutcome::Continue)],
|
||||
after_tool_use: vec![counting_success_hook(&calls, "counting")],
|
||||
..Hooks::default()
|
||||
};
|
||||
|
||||
hooks.dispatch(after_tool_use_payload("p")).await;
|
||||
let outcomes = hooks.dispatch(after_tool_use_payload("p")).await;
|
||||
assert_eq!(outcomes.len(), 1);
|
||||
assert_eq!(outcomes[0].hook_name, "counting");
|
||||
assert!(matches!(outcomes[0].result, HookResult::Success));
|
||||
assert_eq!(calls.load(Ordering::SeqCst), 1);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn dispatch_continues_after_continueable_failure() {
|
||||
let calls = Arc::new(AtomicUsize::new(0));
|
||||
let hooks = Hooks {
|
||||
after_agent: vec![
|
||||
failing_continue_hook(&calls, "failing", "hook failed"),
|
||||
counting_success_hook(&calls, "counting"),
|
||||
],
|
||||
..Hooks::default()
|
||||
};
|
||||
|
||||
let outcomes = hooks.dispatch(hook_payload("err")).await;
|
||||
assert_eq!(outcomes.len(), 2);
|
||||
assert_eq!(outcomes[0].hook_name, "failing");
|
||||
assert!(matches!(outcomes[0].result, HookResult::FailedContinue(_)));
|
||||
assert_eq!(outcomes[1].hook_name, "counting");
|
||||
assert!(matches!(outcomes[1].result, HookResult::Success));
|
||||
assert_eq!(calls.load(Ordering::SeqCst), 2);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn dispatch_returns_after_tool_use_failure_outcome() {
|
||||
let calls = Arc::new(AtomicUsize::new(0));
|
||||
let hooks = Hooks {
|
||||
after_tool_use: vec![failing_continue_hook(
|
||||
&calls,
|
||||
"failing",
|
||||
"after_tool_use hook failed",
|
||||
)],
|
||||
..Hooks::default()
|
||||
};
|
||||
|
||||
let outcomes = hooks.dispatch(after_tool_use_payload("err-tool")).await;
|
||||
assert_eq!(outcomes.len(), 1);
|
||||
assert_eq!(outcomes[0].hook_name, "failing");
|
||||
assert!(matches!(outcomes[0].result, HookResult::FailedContinue(_)));
|
||||
assert_eq!(calls.load(Ordering::SeqCst), 1);
|
||||
}
|
||||
|
||||
@@ -276,6 +372,7 @@ mod tests {
|
||||
let payload_path = temp_dir.path().join("payload.json");
|
||||
let payload_path_arg = payload_path.to_string_lossy().into_owned();
|
||||
let hook = Hook {
|
||||
name: "write_payload".to_string(),
|
||||
func: Arc::new(move |payload: &HookPayload| {
|
||||
let payload_path_arg = payload_path_arg.clone();
|
||||
Box::pin(async move {
|
||||
@@ -290,7 +387,7 @@ mod tests {
|
||||
])
|
||||
.expect("build command");
|
||||
command.status().await.expect("run hook command");
|
||||
HookOutcome::Continue
|
||||
HookResult::Success
|
||||
})
|
||||
}),
|
||||
};
|
||||
@@ -302,7 +399,9 @@ mod tests {
|
||||
after_agent: vec![hook],
|
||||
..Hooks::default()
|
||||
};
|
||||
hooks.dispatch(payload).await;
|
||||
let outcomes = hooks.dispatch(payload).await;
|
||||
assert_eq!(outcomes.len(), 1);
|
||||
assert!(matches!(outcomes[0].result, HookResult::Success));
|
||||
|
||||
let contents = timeout(Duration::from_secs(2), async {
|
||||
loop {
|
||||
@@ -330,6 +429,7 @@ mod tests {
|
||||
fs::write(&script_path, "[IO.File]::WriteAllText($args[0], $args[1])")?;
|
||||
let script_path_arg = script_path.to_string_lossy().into_owned();
|
||||
let hook = Hook {
|
||||
name: "write_payload".to_string(),
|
||||
func: Arc::new(move |payload: &HookPayload| {
|
||||
let payload_path_arg = payload_path_arg.clone();
|
||||
let script_path_arg = script_path_arg.clone();
|
||||
@@ -348,7 +448,7 @@ mod tests {
|
||||
])
|
||||
.expect("build command");
|
||||
command.status().await.expect("run hook command");
|
||||
HookOutcome::Continue
|
||||
HookResult::Success
|
||||
})
|
||||
}),
|
||||
};
|
||||
@@ -360,7 +460,9 @@ mod tests {
|
||||
after_agent: vec![hook],
|
||||
..Hooks::default()
|
||||
};
|
||||
hooks.dispatch(payload).await;
|
||||
let outcomes = hooks.dispatch(payload).await;
|
||||
assert_eq!(outcomes.len(), 1);
|
||||
assert!(matches!(outcomes[0].result, HookResult::Success));
|
||||
|
||||
let contents = timeout(Duration::from_secs(2), async {
|
||||
loop {
|
||||
|
||||
+33
-11
@@ -10,24 +10,53 @@ use futures::future::BoxFuture;
|
||||
use serde::Serialize;
|
||||
use serde::Serializer;
|
||||
|
||||
pub type HookFn = Arc<dyn for<'a> Fn(&'a HookPayload) -> BoxFuture<'a, HookOutcome> + Send + Sync>;
|
||||
pub type HookFn = Arc<dyn for<'a> Fn(&'a HookPayload) -> BoxFuture<'a, HookResult> + Send + Sync>;
|
||||
|
||||
#[derive(Debug)]
|
||||
pub enum HookResult {
|
||||
/// Success: hook completed successfully.
|
||||
Success,
|
||||
/// FailedContinue: hook failed, but other subsequent hooks should still execute and the
|
||||
/// operation should continue.
|
||||
FailedContinue(Box<dyn std::error::Error + Send + Sync + 'static>),
|
||||
/// FailedAbort: hook failed, other subsequent hooks should not execute, and the operation
|
||||
/// should be aborted.
|
||||
FailedAbort(Box<dyn std::error::Error + Send + Sync + 'static>),
|
||||
}
|
||||
|
||||
impl HookResult {
|
||||
pub fn should_abort_operation(&self) -> bool {
|
||||
matches!(self, Self::FailedAbort(_))
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
pub struct HookResponse {
|
||||
pub hook_name: String,
|
||||
pub result: HookResult,
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
pub struct Hook {
|
||||
pub name: String,
|
||||
pub func: HookFn,
|
||||
}
|
||||
|
||||
impl Default for Hook {
|
||||
fn default() -> Self {
|
||||
Self {
|
||||
func: Arc::new(|_| Box::pin(async { HookOutcome::Continue })),
|
||||
name: "default".to_string(),
|
||||
func: Arc::new(|_| Box::pin(async { HookResult::Success })),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl Hook {
|
||||
pub async fn execute(&self, payload: &HookPayload) -> HookOutcome {
|
||||
(self.func)(payload).await
|
||||
pub async fn execute(&self, payload: &HookPayload) -> HookResponse {
|
||||
HookResponse {
|
||||
hook_name: self.name.clone(),
|
||||
result: (self.func)(payload).await,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -126,13 +155,6 @@ pub enum HookEvent {
|
||||
},
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub enum HookOutcome {
|
||||
Continue,
|
||||
#[allow(dead_code)]
|
||||
Stop,
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use std::path::PathBuf;
|
||||
|
||||
@@ -6,8 +6,8 @@ use serde::Serialize;
|
||||
|
||||
use crate::Hook;
|
||||
use crate::HookEvent;
|
||||
use crate::HookOutcome;
|
||||
use crate::HookPayload;
|
||||
use crate::HookResult;
|
||||
use crate::command_from_argv;
|
||||
|
||||
/// Legacy notify payload appended as the final argv argument for backward compatibility.
|
||||
@@ -48,12 +48,13 @@ pub fn legacy_notify_json(hook_event: &HookEvent, cwd: &Path) -> Result<String,
|
||||
pub fn notify_hook(argv: Vec<String>) -> Hook {
|
||||
let argv = Arc::new(argv);
|
||||
Hook {
|
||||
name: "legacy_notify".to_string(),
|
||||
func: Arc::new(move |payload: &HookPayload| {
|
||||
let argv = Arc::clone(&argv);
|
||||
Box::pin(async move {
|
||||
let mut command = match command_from_argv(&argv) {
|
||||
Some(command) => command,
|
||||
None => return HookOutcome::Continue,
|
||||
None => return HookResult::Success,
|
||||
};
|
||||
if let Ok(notify_payload) = legacy_notify_json(&payload.hook_event, &payload.cwd) {
|
||||
command.arg(notify_payload);
|
||||
@@ -65,8 +66,10 @@ pub fn notify_hook(argv: Vec<String>) -> Hook {
|
||||
.stdout(Stdio::null())
|
||||
.stderr(Stdio::null());
|
||||
|
||||
let _ = command.spawn();
|
||||
HookOutcome::Continue
|
||||
match command.spawn() {
|
||||
Ok(_) => HookResult::Success,
|
||||
Err(err) => HookResult::FailedContinue(err.into()),
|
||||
}
|
||||
})
|
||||
}),
|
||||
}
|
||||
|
||||
@@ -253,6 +253,11 @@ pub struct ModelInfo {
|
||||
/// When true, this model should use websocket transport even when websocket features are off.
|
||||
#[serde(default)]
|
||||
pub prefer_websockets: bool,
|
||||
/// Internal-only marker set by core when a model slug resolved to fallback metadata.
|
||||
#[serde(default, skip_serializing, skip_deserializing)]
|
||||
#[schemars(skip)]
|
||||
#[ts(skip)]
|
||||
pub used_fallback_model_metadata: bool,
|
||||
}
|
||||
|
||||
impl ModelInfo {
|
||||
@@ -517,6 +522,7 @@ mod tests {
|
||||
experimental_supported_tools: vec![],
|
||||
input_modalities: default_input_modalities(),
|
||||
prefer_websockets: false,
|
||||
used_fallback_model_metadata: false,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -170,20 +170,28 @@ def codex_rust_crate(
|
||||
cargo_env["CARGO_BIN_EXE_" + binary] = "$(rlocationpath %s)" % binary_label
|
||||
|
||||
for test in native.glob(["tests/*.rs"], allow_empty = True):
|
||||
test_name = name + "-" + test.removeprefix("tests/").removesuffix(".rs").replace("/", "-")
|
||||
test_file_stem = test.removeprefix("tests/").removesuffix(".rs")
|
||||
test_crate_name = test_file_stem.replace("-", "_")
|
||||
test_name = name + "-" + test_file_stem.replace("/", "-")
|
||||
if not test_name.endswith("-test"):
|
||||
test_name += "-test"
|
||||
|
||||
rust_test(
|
||||
name = test_name,
|
||||
crate_name = test_crate_name,
|
||||
crate_root = test,
|
||||
srcs = [test],
|
||||
data = native.glob(["tests/**"], allow_empty = True) + sanitized_binaries + test_data_extra,
|
||||
compile_data = native.glob(["tests/**"], allow_empty = True) + integration_compile_data_extra,
|
||||
deps = maybe_lib + deps + dev_deps + integration_deps_extra,
|
||||
proc_macro_deps = proc_macro_deps + proc_macro_dev_deps,
|
||||
rustc_flags = rustc_flags_extra,
|
||||
# Keep `file!()` paths Cargo-like (`core/tests/...`) instead of
|
||||
# Bazel workspace-prefixed (`codex-rs/core/tests/...`) for snapshot parity.
|
||||
rustc_flags = rustc_flags_extra + ["--remap-path-prefix=codex-rs="],
|
||||
rustc_env = rustc_env,
|
||||
env = test_env | cargo_env,
|
||||
# Important: do not merge `test_env` here. Its unit-test-only
|
||||
# `INSTA_WORKSPACE_ROOT="."` can point integration tests at the
|
||||
# runfiles cwd and cause false `.snap.new` churn on Linux.
|
||||
env = cargo_env,
|
||||
tags = test_tags,
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user