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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 5 additions & 11 deletions codex-rs/core/src/codex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -765,19 +765,13 @@ impl Session {

let mut post_session_configured_events = Vec::<Event>::new();

for (alias, feature) in config.features.legacy_feature_usages() {
let canonical = feature.key();
let summary = format!("`{alias}` is deprecated. Use `[features].{canonical}` instead.");
let details = if alias == canonical {
None
} else {
Some(format!(
"Enable it with `--enable {canonical}` or `[features].{canonical}` in config.toml. See https://github.com/openai/codex/blob/main/docs/config.md#feature-flags for details."
))
};
for usage in config.features.legacy_feature_usages() {
post_session_configured_events.push(Event {
id: INITIAL_SUBMIT_ID.to_owned(),
msg: EventMsg::DeprecationNotice(DeprecationNoticeEvent { summary, details }),
msg: EventMsg::DeprecationNotice(DeprecationNoticeEvent {
summary: usage.summary.clone(),
details: usage.details.clone(),
}),
});
}
if crate::config::uses_deprecated_instructions_file(&config.config_layer_stack) {
Expand Down
66 changes: 60 additions & 6 deletions codex-rs/core/src/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ impl Feature {
pub struct LegacyFeatureUsage {
pub alias: String,
pub feature: Feature,
pub summary: String,
pub details: Option<String>,
}

/// Holds the effective set of enabled features.
Expand Down Expand Up @@ -204,9 +206,12 @@ impl Features {
}

pub fn record_legacy_usage_force(&mut self, alias: &str, feature: Feature) {
let (summary, details) = legacy_usage_notice(alias, feature);
self.legacy_usages.insert(LegacyFeatureUsage {
alias: alias.to_string(),
feature,
summary,
details,
});
}

Expand All @@ -217,10 +222,8 @@ impl Features {
self.record_legacy_usage_force(alias, feature);
}

pub fn legacy_feature_usages(&self) -> impl Iterator<Item = (&str, Feature)> + '_ {
self.legacy_usages
.iter()
.map(|usage| (usage.alias.as_str(), usage.feature))
pub fn legacy_feature_usages(&self) -> impl Iterator<Item = &LegacyFeatureUsage> + '_ {
self.legacy_usages.iter()
}

pub fn emit_metrics(&self, otel: &OtelManager) {
Expand All @@ -241,6 +244,21 @@ impl Features {
/// Apply a table of key -> bool toggles (e.g. from TOML).
pub fn apply_map(&mut self, m: &BTreeMap<String, bool>) {
for (k, v) in m {
match k.as_str() {
"web_search_request" => {
self.record_legacy_usage_force(
"features.web_search_request",
Feature::WebSearchRequest,
);
}
"web_search_cached" => {
self.record_legacy_usage_force(
"features.web_search_cached",
Feature::WebSearchCached,
);
}
_ => {}
}
match feature_for_key(k) {
Some(feat) => {
if k != feat.key() {
Expand Down Expand Up @@ -301,6 +319,42 @@ impl Features {
}
}

fn legacy_usage_notice(alias: &str, feature: Feature) -> (String, Option<String>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know how I feel about rendering inside core but this logic has to be somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea, we emit from core. i think having it next to the rest of the logic is nicer than hiding it in another module for now.

let canonical = feature.key();
match feature {
Feature::WebSearchRequest | Feature::WebSearchCached => {
let label = match alias {
"web_search" => "[features].web_search",
"tools.web_search" => "[tools].web_search",
"features.web_search_request" | "web_search_request" => {
"[features].web_search_request"
}
"features.web_search_cached" | "web_search_cached" => {
"[features].web_search_cached"
}
_ => alias,
};
let summary = format!("`{label}` is deprecated. Use `web_search` instead.");
(summary, Some(web_search_details().to_string()))
}
_ => {
let summary = format!("`{alias}` is deprecated. Use `[features].{canonical}` instead.");
let details = if alias == canonical {
None
} else {
Some(format!(
"Enable it with `--enable {canonical}` or `[features].{canonical}` in config.toml. See https://github.com/openai/codex/blob/main/docs/config.md#feature-flags for details."
))
};
(summary, details)
}
}
}

fn web_search_details() -> &'static str {
"Set `web_search` to `\"live\"`, `\"cached\"`, or `\"disabled\"` in config.toml."
}

/// Keys accepted in `[features]` tables.
fn feature_for_key(key: &str) -> Option<Feature> {
for spec in FEATURES {
Expand Down Expand Up @@ -349,13 +403,13 @@ pub const FEATURES: &[FeatureSpec] = &[
FeatureSpec {
id: Feature::WebSearchRequest,
key: "web_search_request",
stage: Stage::Stable,
stage: Stage::Deprecated,
default_enabled: false,
},
FeatureSpec {
id: Feature::WebSearchCached,
key: "web_search_cached",
stage: Stage::UnderDevelopment,
stage: Stage::Deprecated,
default_enabled: false,
},
// Experimental program. Rendered in the `/experimental` menu for users.
Expand Down
71 changes: 71 additions & 0 deletions codex-rs/core/tests/suite/deprecation_notice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ 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 std::collections::BTreeMap;
use toml::Value as TomlValue;

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
Expand Down Expand Up @@ -110,3 +111,73 @@ async fn emits_deprecation_notice_for_experimental_instructions_file() -> anyhow

Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn emits_deprecation_notice_for_web_search_feature_flags() -> anyhow::Result<()> {
skip_if_no_network!(Ok(()));

let server = start_mock_server().await;

let mut builder = test_codex().with_config(|config| {
let mut entries = BTreeMap::new();
entries.insert("web_search_request".to_string(), true);
config.features.apply_map(&entries);
});

let TestCodex { codex, .. } = builder.build(&server).await?;

let notice = wait_for_event_match(&codex, |event| match event {
EventMsg::DeprecationNotice(ev) if ev.summary.contains("[features].web_search_request") => {
Some(ev.clone())
}
_ => None,
})
.await;

let DeprecationNoticeEvent { summary, details } = notice;
assert_eq!(
summary,
"`[features].web_search_request` is deprecated. Use `web_search` instead.".to_string(),
);
assert_eq!(
details.as_deref(),
Some("Set `web_search` to `\"live\"`, `\"cached\"`, or `\"disabled\"` in config.toml."),
);

Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn emits_deprecation_notice_for_disabled_web_search_feature_flag() -> anyhow::Result<()> {
skip_if_no_network!(Ok(()));

let server = start_mock_server().await;

let mut builder = test_codex().with_config(|config| {
let mut entries = BTreeMap::new();
entries.insert("web_search_request".to_string(), false);
config.features.apply_map(&entries);
});

let TestCodex { codex, .. } = builder.build(&server).await?;

let notice = wait_for_event_match(&codex, |event| match event {
EventMsg::DeprecationNotice(ev) if ev.summary.contains("[features].web_search_request") => {
Some(ev.clone())
}
_ => None,
})
.await;

let DeprecationNoticeEvent { summary, details } = notice;
assert_eq!(
summary,
"`[features].web_search_request` is deprecated. Use `web_search` instead.".to_string(),
);
assert_eq!(
details.as_deref(),
Some("Set `web_search` to `\"live\"`, `\"cached\"`, or `\"disabled\"` in config.toml."),
);

Ok(())
}
Loading