feat: rename experimental_instructions_file to model_instructions_file (#9555)
A user who has `experimental_instructions_file` set will now see this: <img width="888" height="660" alt="image" src="https://github.com/user-attachments/assets/51c98312-eb9b-4881-81f1-bea6677e158d" /> And a `codex exec` would include this warning: <img width="888" height="660" alt="image" src="https://github.com/user-attachments/assets/a89f62be-1edf-4593-a75e-e0b4a762ed7d" />
This commit is contained in:
parent
3a0eeb8edf
commit
f4d55319d1
8 changed files with 141 additions and 29 deletions
|
|
@ -53,14 +53,6 @@
|
|||
"experimental_compact_prompt_file": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
},
|
||||
"experimental_instructions_file": {
|
||||
"description": "Legacy, now use features",
|
||||
"allOf": [
|
||||
{
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
}
|
||||
]
|
||||
},
|
||||
"experimental_use_freeform_apply_patch": {
|
||||
"type": "boolean"
|
||||
},
|
||||
|
|
@ -240,6 +232,14 @@
|
|||
"type": "integer",
|
||||
"format": "int64"
|
||||
},
|
||||
"model_instructions_file": {
|
||||
"description": "Optional path to a file containing model instructions that will override the built-in instructions for the selected model. Users are STRONGLY DISCOURAGED from using this field, as deviating from the instructions sanctioned by Codex will likely degrade model performance.",
|
||||
"allOf": [
|
||||
{
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
}
|
||||
]
|
||||
},
|
||||
"model_personality": {
|
||||
"description": "EXPERIMENTAL Optionally specify a personality for the model",
|
||||
"allOf": [
|
||||
|
|
@ -545,9 +545,6 @@
|
|||
"experimental_compact_prompt_file": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
},
|
||||
"experimental_instructions_file": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
},
|
||||
"experimental_use_freeform_apply_patch": {
|
||||
"type": "boolean"
|
||||
},
|
||||
|
|
@ -640,6 +637,14 @@
|
|||
"model": {
|
||||
"type": "string"
|
||||
},
|
||||
"model_instructions_file": {
|
||||
"description": "Optional path to a file containing model instructions.",
|
||||
"allOf": [
|
||||
{
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
}
|
||||
]
|
||||
},
|
||||
"model_personality": {
|
||||
"$ref": "#/definitions/Personality"
|
||||
},
|
||||
|
|
|
|||
|
|
@ -666,6 +666,19 @@ impl Session {
|
|||
msg: EventMsg::DeprecationNotice(DeprecationNoticeEvent { summary, details }),
|
||||
});
|
||||
}
|
||||
if crate::config::uses_deprecated_instructions_file(&config.config_layer_stack) {
|
||||
post_session_configured_events.push(Event {
|
||||
id: INITIAL_SUBMIT_ID.to_owned(),
|
||||
msg: EventMsg::DeprecationNotice(DeprecationNoticeEvent {
|
||||
summary: "`experimental_instructions_file` is deprecated and ignored. Use `model_instructions_file` instead."
|
||||
.to_string(),
|
||||
details: Some(
|
||||
"Move the setting to `model_instructions_file` in config.toml (or under a profile) to load instructions from a file."
|
||||
.to_string(),
|
||||
),
|
||||
}),
|
||||
});
|
||||
}
|
||||
maybe_push_chat_wire_api_deprecation(&config, &mut post_session_configured_events);
|
||||
|
||||
let auth = auth_manager.auth().await;
|
||||
|
|
|
|||
|
|
@ -820,6 +820,12 @@ pub struct ConfigToml {
|
|||
#[serde(default)]
|
||||
pub developer_instructions: Option<String>,
|
||||
|
||||
/// Optional path to a file containing model instructions that will override
|
||||
/// the built-in instructions for the selected model. Users are STRONGLY
|
||||
/// DISCOURAGED from using this field, as deviating from the instructions
|
||||
/// sanctioned by Codex will likely degrade model performance.
|
||||
pub model_instructions_file: Option<AbsolutePathBuf>,
|
||||
|
||||
/// Compact prompt used for history compaction.
|
||||
pub compact_prompt: Option<String>,
|
||||
|
||||
|
|
@ -965,6 +971,8 @@ pub struct ConfigToml {
|
|||
pub notice: Option<Notice>,
|
||||
|
||||
/// Legacy, now use features
|
||||
/// Deprecated: ignored. Use `model_instructions_file`.
|
||||
#[schemars(skip)]
|
||||
pub experimental_instructions_file: Option<AbsolutePathBuf>,
|
||||
pub experimental_compact_prompt_file: Option<AbsolutePathBuf>,
|
||||
pub experimental_use_unified_exec_tool: Option<bool>,
|
||||
|
|
@ -1432,14 +1440,12 @@ impl Config {
|
|||
// Load base instructions override from a file if specified. If the
|
||||
// path is relative, resolve it against the effective cwd so the
|
||||
// behaviour matches other path-like config values.
|
||||
let experimental_instructions_path = config_profile
|
||||
.experimental_instructions_file
|
||||
let model_instructions_path = config_profile
|
||||
.model_instructions_file
|
||||
.as_ref()
|
||||
.or(cfg.experimental_instructions_file.as_ref());
|
||||
let file_base_instructions = Self::try_read_non_empty_file(
|
||||
experimental_instructions_path,
|
||||
"experimental instructions file",
|
||||
)?;
|
||||
.or(cfg.model_instructions_file.as_ref());
|
||||
let file_base_instructions =
|
||||
Self::try_read_non_empty_file(model_instructions_path, "model instructions file")?;
|
||||
let base_instructions = base_instructions.or(file_base_instructions);
|
||||
let developer_instructions = developer_instructions.or(cfg.developer_instructions);
|
||||
|
||||
|
|
@ -1682,6 +1688,30 @@ impl Config {
|
|||
}
|
||||
}
|
||||
|
||||
pub(crate) fn uses_deprecated_instructions_file(config_layer_stack: &ConfigLayerStack) -> bool {
|
||||
config_layer_stack
|
||||
.layers_high_to_low()
|
||||
.into_iter()
|
||||
.any(|layer| toml_uses_deprecated_instructions_file(&layer.config))
|
||||
}
|
||||
|
||||
fn toml_uses_deprecated_instructions_file(value: &TomlValue) -> bool {
|
||||
let Some(table) = value.as_table() else {
|
||||
return false;
|
||||
};
|
||||
if table.contains_key("experimental_instructions_file") {
|
||||
return true;
|
||||
}
|
||||
let Some(profiles) = table.get("profiles").and_then(TomlValue::as_table) else {
|
||||
return false;
|
||||
};
|
||||
profiles.values().any(|profile| {
|
||||
profile.as_table().is_some_and(|profile_table| {
|
||||
profile_table.contains_key("experimental_instructions_file")
|
||||
})
|
||||
})
|
||||
}
|
||||
|
||||
/// Returns the path to the Codex configuration directory, which can be
|
||||
/// specified by the `CODEX_HOME` environment variable. If not set, defaults to
|
||||
/// `~/.codex`.
|
||||
|
|
|
|||
|
|
@ -27,6 +27,10 @@ pub struct ConfigProfile {
|
|||
pub model_verbosity: Option<Verbosity>,
|
||||
pub model_personality: Option<Personality>,
|
||||
pub chatgpt_base_url: Option<String>,
|
||||
/// Optional path to a file containing model instructions.
|
||||
pub model_instructions_file: Option<AbsolutePathBuf>,
|
||||
/// Deprecated: ignored. Use `model_instructions_file`.
|
||||
#[schemars(skip)]
|
||||
pub experimental_instructions_file: Option<AbsolutePathBuf>,
|
||||
pub experimental_compact_prompt_file: Option<AbsolutePathBuf>,
|
||||
pub include_apply_patch_tool: Option<bool>,
|
||||
|
|
|
|||
|
|
@ -656,7 +656,7 @@ mod unit_tests {
|
|||
let contents = r#"
|
||||
# This is a field recognized by config.toml that is an AbsolutePathBuf in
|
||||
# the ConfigToml struct.
|
||||
experimental_instructions_file = "./some_file.md"
|
||||
model_instructions_file = "./some_file.md"
|
||||
|
||||
# This is a field recognized by config.toml.
|
||||
model = "gpt-1000"
|
||||
|
|
@ -669,7 +669,7 @@ foo = "xyzzy"
|
|||
let normalized_toml_value = resolve_relative_paths_in_config_toml(user_config, base_dir)?;
|
||||
let mut expected_toml_value = toml::map::Map::new();
|
||||
expected_toml_value.insert(
|
||||
"experimental_instructions_file".to_string(),
|
||||
"model_instructions_file".to_string(),
|
||||
TomlValue::String(
|
||||
AbsolutePathBuf::resolve_path_against_base("./some_file.md", base_dir)?
|
||||
.as_path()
|
||||
|
|
|
|||
|
|
@ -438,10 +438,10 @@ async fn project_paths_resolve_relative_to_dot_codex_and_override_in_order() ->
|
|||
tokio::fs::write(project_root.join(".git"), "gitdir: here").await?;
|
||||
|
||||
let root_cfg = r#"
|
||||
experimental_instructions_file = "root.txt"
|
||||
model_instructions_file = "root.txt"
|
||||
"#;
|
||||
let nested_cfg = r#"
|
||||
experimental_instructions_file = "child.txt"
|
||||
model_instructions_file = "child.txt"
|
||||
"#;
|
||||
tokio::fs::write(project_root.join(".codex").join(CONFIG_TOML_FILE), root_cfg).await?;
|
||||
tokio::fs::write(nested.join(".codex").join(CONFIG_TOML_FILE), nested_cfg).await?;
|
||||
|
|
@ -591,7 +591,7 @@ async fn cli_overrides_with_relative_paths_do_not_break_trust_check() -> std::io
|
|||
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(&nested)?;
|
||||
let cli_overrides = vec![(
|
||||
"experimental_instructions_file".to_string(),
|
||||
"model_instructions_file".to_string(),
|
||||
TomlValue::String("relative.md".to_string()),
|
||||
)];
|
||||
|
||||
|
|
|
|||
|
|
@ -109,11 +109,11 @@ async fn chat_mode_stream_cli() {
|
|||
);
|
||||
}
|
||||
|
||||
/// Verify that passing `-c experimental_instructions_file=...` to the CLI
|
||||
/// Verify that passing `-c model_instructions_file=...` to the CLI
|
||||
/// overrides the built-in base instructions by inspecting the request body
|
||||
/// received by a mock OpenAI Responses endpoint.
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn exec_cli_applies_experimental_instructions_file() {
|
||||
async fn exec_cli_applies_model_instructions_file() {
|
||||
skip_if_no_network!();
|
||||
|
||||
// Start mock server which will capture the request and return a minimal
|
||||
|
|
@ -128,7 +128,7 @@ async fn exec_cli_applies_experimental_instructions_file() {
|
|||
// Create a temporary instructions file with a unique marker we can assert
|
||||
// appears in the outbound request payload.
|
||||
let custom = TempDir::new().unwrap();
|
||||
let marker = "cli-experimental-instructions-marker";
|
||||
let marker = "cli-model-instructions-file-marker";
|
||||
let custom_path = custom.path().join("instr.md");
|
||||
std::fs::write(&custom_path, marker).unwrap();
|
||||
let custom_path_str = custom_path.to_string_lossy().replace('\\', "/");
|
||||
|
|
@ -151,9 +151,7 @@ async fn exec_cli_applies_experimental_instructions_file() {
|
|||
.arg("-c")
|
||||
.arg("model_provider=\"mock\"")
|
||||
.arg("-c")
|
||||
.arg(format!(
|
||||
"experimental_instructions_file=\"{custom_path_str}\""
|
||||
))
|
||||
.arg(format!("model_instructions_file=\"{custom_path_str}\""))
|
||||
.arg("-C")
|
||||
.arg(&repo_root)
|
||||
.arg("hello?\n");
|
||||
|
|
|
|||
|
|
@ -1,15 +1,22 @@
|
|||
#![cfg(not(target_os = "windows"))]
|
||||
|
||||
use anyhow::Ok;
|
||||
use codex_app_server_protocol::ConfigLayerSource;
|
||||
use codex_core::config_loader::ConfigLayerEntry;
|
||||
use codex_core::config_loader::ConfigLayerStack;
|
||||
use codex_core::config_loader::ConfigRequirements;
|
||||
use codex_core::config_loader::ConfigRequirementsToml;
|
||||
use codex_core::features::Feature;
|
||||
use codex_core::protocol::DeprecationNoticeEvent;
|
||||
use codex_core::protocol::EventMsg;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use core_test_support::test_absolute_path;
|
||||
use core_test_support::test_codex::TestCodex;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use core_test_support::wait_for_event_match;
|
||||
use pretty_assertions::assert_eq;
|
||||
use toml::Value as TomlValue;
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn emits_deprecation_notice_for_legacy_feature_flag() -> anyhow::Result<()> {
|
||||
|
|
@ -48,3 +55,58 @@ async fn emits_deprecation_notice_for_legacy_feature_flag() -> anyhow::Result<()
|
|||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn emits_deprecation_notice_for_experimental_instructions_file() -> anyhow::Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
|
||||
let mut builder = test_codex().with_config(|config| {
|
||||
let mut table = toml::map::Map::new();
|
||||
table.insert(
|
||||
"experimental_instructions_file".to_string(),
|
||||
TomlValue::String("legacy.md".to_string()),
|
||||
);
|
||||
let config_layer = ConfigLayerEntry::new(
|
||||
ConfigLayerSource::User {
|
||||
file: test_absolute_path("/tmp/config.toml"),
|
||||
},
|
||||
TomlValue::Table(table),
|
||||
);
|
||||
let config_layer_stack = ConfigLayerStack::new(
|
||||
vec![config_layer],
|
||||
ConfigRequirements::default(),
|
||||
ConfigRequirementsToml::default(),
|
||||
)
|
||||
.expect("build config layer stack");
|
||||
config.config_layer_stack = config_layer_stack;
|
||||
});
|
||||
|
||||
let TestCodex { codex, .. } = builder.build(&server).await?;
|
||||
|
||||
let notice = wait_for_event_match(&codex, |event| match event {
|
||||
EventMsg::DeprecationNotice(ev)
|
||||
if ev.summary.contains("experimental_instructions_file") =>
|
||||
{
|
||||
Some(ev.clone())
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
.await;
|
||||
|
||||
let DeprecationNoticeEvent { summary, details } = notice;
|
||||
assert_eq!(
|
||||
summary,
|
||||
"`experimental_instructions_file` is deprecated and ignored. Use `model_instructions_file` instead."
|
||||
.to_string(),
|
||||
);
|
||||
assert_eq!(
|
||||
details.as_deref(),
|
||||
Some(
|
||||
"Move the setting to `model_instructions_file` in config.toml (or under a profile) to load instructions from a file."
|
||||
),
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue